LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Possible mem leak in copy_process()
@ 2008-04-16 21:45 Jesper Juhl
  2008-04-17  4:17 ` Arnd Bergmann
  2008-04-17 11:57 ` Oleg Nesterov
  0 siblings, 2 replies; 9+ messages in thread
From: Jesper Juhl @ 2008-04-16 21:45 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: linux-kernel, Jesper Juhl


Hi Ingo,

I was just browsing through Coverity Scan results for the kernel and 
noticed one thing it spotted that looks valid but which I have no idea how 
to fix. 
The code is, as far as I can see, written by you so I thought I'd forward 
the info from Coverity to you in the hope that perhaps you'd know what to 
do about it :-)

It's a memory leak in copy_process() - this is what coverity reports : 


1004 	static struct task_struct *copy_process(unsigned long clone_flags,
1005 						unsigned long stack_start,
1006 						struct pt_regs *regs,
1007 						unsigned long stack_size,
1008 						int __user *child_tidptr,
1009 						struct pid *pid)
1010 	{
<...snip...>
1198 			retval = -ENOMEM;

Event alloc_fn: Called allocation function "alloc_pid" [model]
Event var_assign: Assigned variable "pid" to storage returned from "alloc_pid"
Also see events: [var_assign][leaked_storage][pass_arg]

1199 			pid = alloc_pid(task_active_pid_ns(p));

At conditional (1): "pid == 0" taking false path

1200 			if (!pid)
1201 				goto bad_fork_cleanup_io;
1202 	

At conditional (2): "clone_flags & 536870912 != 0" taking true path

1203 			if (clone_flags & CLONE_NEWPID) {
1204 				retval = pid_ns_prepare_proc(task_active_pid_ns(p));

At conditional (3): "retval < 0" taking false path

1205 				if (retval < 0)
1206 					goto bad_fork_free_pid;
1207 			}
1208 		}
1209 	

Event pass_arg: Variable "pid" not freed or pointed-to in function "pid_nr" [model]
Also see events: [alloc_fn][var_assign][leaked_storage]

1210 		p->pid = pid_nr(pid);
1211 		p->tgid = p->pid;

At conditional (4): "clone_flags & 65536 != 0" taking true path

1212 		if (clone_flags & CLONE_THREAD)
1213 			p->tgid = current->tgid;
1214 	

At conditional (5): "clone_flags & 16777216 != 0" taking true path

1215 		p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL;
1216 		/*
1217 		 * Clear TID on mm_release()?
1218 		 */

At conditional (6): "clone_flags & 2097152 != 0" taking true path

1219 		p->clear_child_tid = (clone_flags & CLONE_CHILD_CLEARTID) ? child_tidptr: NULL;
1220 	#ifdef CONFIG_FUTEX
1221 		p->robust_list = NULL;
1222 	#ifdef CONFIG_COMPAT
1223 		p->compat_robust_list = NULL;
1224 	#endif
1225 		INIT_LIST_HEAD(&p->pi_state_list);
1226 		p->pi_state_cache = NULL;
1227 	#endif
1228 		/*
1229 		 * sigaltstack should be cleared when sharing the same VM
1230 		 */

At conditional (7): "clone_flags & 16640 == 256" taking true path

1231 		if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM)
1232 			p->sas_ss_sp = p->sas_ss_size = 0;
1233 	
1234 		/*
1235 		 * Syscall tracing should be turned off in the child regardless
1236 		 * of CLONE_PTRACE.
1237 		 */
1238 		clear_tsk_thread_flag(p, TIF_SYSCALL_TRACE);
1239 	#ifdef TIF_SYSCALL_EMU
1240 		clear_tsk_thread_flag(p, TIF_SYSCALL_EMU);
1241 	#endif
1242 		clear_all_latency_tracing(p);
1243 	
1244 		/* Our parent execution domain becomes current domain
1245 		   These must match for thread signalling to apply */
1246 		p->parent_exec_id = p->self_exec_id;
1247 	
1248 		/* ok, now we should be set up.. */

At conditional (8): "clone_flags & 65536 != 0" taking true path

1249 		p->exit_signal = (clone_flags & CLONE_THREAD) ? -1 : (clone_flags & CSIGNAL);
1250 		p->pdeath_signal = 0;
1251 		p->exit_state = 0;
1252 	
1253 		/*
1254 		 * Ok, make it visible to the rest of the system.
1255 		 * We dont wake it up yet.
1256 		 */
1257 		p->group_leader = p;
1258 		INIT_LIST_HEAD(&p->thread_group);
1259 		INIT_LIST_HEAD(&p->ptrace_children);
1260 		INIT_LIST_HEAD(&p->ptrace_list);
1261 	
1262 		/* Now that the task is set up, run cgroup callbacks if
1263 		 * necessary. We need to run them before the task is visible
1264 		 * on the tasklist. */
1265 		cgroup_fork_callbacks(p);
1266 		cgroup_callbacks_done = 1;
1267 	
1268 		/* Need tasklist lock for parent etc handling! */
1269 		write_lock_irq(&tasklist_lock);
1270 	
1271 		/*
1272 		 * The task hasn't been attached yet, so its cpus_allowed mask will
1273 		 * not be changed, nor will its assigned CPU.
1274 		 *
1275 		 * The cpus_allowed mask of the parent may have changed after it was
1276 		 * copied first time - so re-copy it here, then check the child's CPU
1277 		 * to ensure it is on a valid CPU (and if not, just force it back to
1278 		 * parent's CPU). This avoids alot of nasty races.
1279 		 */
1280 		p->cpus_allowed = current->cpus_allowed;
1281 		p->rt.nr_cpus_allowed = current->rt.nr_cpus_allowed;

At conditional (9): "0" taking false path
At conditional (10): "((0) ? constant_test_bit : (variable_test_bit)) == 0" taking true path

1282 		if (unlikely(!cpu_isset(task_cpu(p), p->cpus_allowed) ||
1283 				!cpu_online(task_cpu(p))))

At conditional (11): "4 == 4" taking true path

1284 			set_task_cpu(p, smp_processor_id());
1285 	
1286 		/* CLONE_PARENT re-uses the old parent */

At conditional (12): "clone_flags & 98304 != 0" taking true path

1287 		if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
1288 			p->real_parent = current->real_parent;
1289 		else
1290 			p->real_parent = current;
1291 		p->parent = p->real_parent;
1292 	
1293 		spin_lock(&current->sighand->siglock);
1294 	
1295 		/*
1296 		 * Process group and session signals need to be delivered to just the
1297 		 * parent before the fork or both the parent and the child after the
1298 		 * fork. Restart if a signal comes in before we add the new process to
1299 		 * it's process group.
1300 		 * A fatal signal pending means that current will exit, so the new
1301 		 * thread can't slip out of an OOM kill (or normal SIGKILL).
1302 	 	 */
1303 		recalc_sigpending();

At conditional (13): "signal_pending != 0" taking false path

1304 		if (signal_pending(current)) {
1305 			spin_unlock(&current->sighand->siglock);
1306 			write_unlock_irq(&tasklist_lock);
1307 			retval = -ERESTARTNOINTR;
1308 			goto bad_fork_free_pid;
1309 		}
1310 	

At conditional (14): "clone_flags & 65536 != 0" taking true path

1311 		if (clone_flags & CLONE_THREAD) {
1312 			p->group_leader = current->group_leader;
1313 			list_add_tail_rcu(&p->thread_group, &p->group_leader->thread_group);
1314 	

At conditional (15): "((get_current)->signal)->it_virt_expires != 0" 
taking true path

1315 			if (!cputime_eq(current->signal->it_virt_expires,
1316 					cputime_zero) ||
1317 			    !cputime_eq(current->signal->it_prof_expires,
1318 					cputime_zero) ||
1319 			    current->signal->rlim[RLIMIT_CPU].rlim_cur != RLIM_INFINITY ||
1320 			    !list_empty(&current->signal->cpu_timers[0]) ||
1321 			    !list_empty(&current->signal->cpu_timers[1]) ||
1322 			    !list_empty(&current->signal->cpu_timers[2])) {
1323 				/*
1324 				 * Have child wake up on its first tick to check
1325 				 * for process CPU timers.
1326 				 */
1327 				p->it_prof_expires = jiffies_to_cputime(1);
1328 			}
1329 		}
1330 	

At conditional (16): "(p)->pid != 0" taking false path

1331 		if (likely(p->pid)) {
1332 			add_parent(p);
1333 			if (unlikely(p->ptrace & PT_PTRACED))
1334 				__ptrace_link(p, current->parent);
1335 	
1336 			if (thread_group_leader(p)) {
1337 				if (clone_flags & CLONE_NEWPID)
1338 					p->nsproxy->pid_ns->child_reaper = p;
1339 	
1340 				p->signal->leader_pid = pid;
1341 				p->signal->tty = current->signal->tty;
1342 				set_task_pgrp(p, task_pgrp_nr(current));
1343 				set_task_session(p, task_session_nr(current));
1344 				attach_pid(p, PIDTYPE_PGID, task_pgrp(current));
1345 				attach_pid(p, PIDTYPE_SID, task_session(current));
1346 				list_add_tail_rcu(&p->tasks, &init_task.tasks);
1347 				__get_cpu_var(process_counts)++;
1348 			}
1349 			attach_pid(p, PIDTYPE_PID, pid);
1350 			nr_threads++;
1351 		}
1352 	
1353 		total_forks++;
1354 		spin_unlock(&current->sighand->siglock);
1355 		write_unlock_irq(&tasklist_lock);
1356 		proc_fork_connector(p);
1357 		cgroup_post_fork(p);

Event leaked_storage: Returned without freeing storage "pid"
Also see events: [alloc_fn][var_assign][pass_arg]

1358 		return p;


Perhaps it can never happen the way Coverity thinks it can. You are a much 
better judge of that than I, but it looks to me like it's at least 
possible in theory - in which case we have a potential leak every time we 
create a new process and that can't be good...

Anyway, just wanted to bring it to your attention.  Bye bye :-)


-- 
 Jesper Juhl < jesper.juhl@gmail.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Possible mem leak in copy_process()
  2008-04-16 21:45 Possible mem leak in copy_process() Jesper Juhl
@ 2008-04-17  4:17 ` Arnd Bergmann
  2008-04-17 11:57 ` Oleg Nesterov
  1 sibling, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2008-04-17  4:17 UTC (permalink / raw)
  To: Jesper Juhl; +Cc: Ingo Molnar, linux-kernel

On Wednesday 16 April 2008, Jesper Juhl wrote:
> 
> Perhaps it can never happen the way Coverity thinks it can. You are a much 
> better judge of that than I, but it looks to me like it's at least 
> possible in theory - in which case we have a potential leak every time we 
> create a new process and that can't be good...

I think you're just looking at the common case here, which is correct:
The pid is allocated for task creation and stays around as long as the
task does. It gets freed when the last reference to it goes away,
usually in the put_pid() called from exit().

	Arnd <><

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Possible mem leak in copy_process()
  2008-04-16 21:45 Possible mem leak in copy_process() Jesper Juhl
  2008-04-17  4:17 ` Arnd Bergmann
@ 2008-04-17 11:57 ` Oleg Nesterov
  2008-04-17 13:02   ` Ingo Molnar
  1 sibling, 1 reply; 9+ messages in thread
From: Oleg Nesterov @ 2008-04-17 11:57 UTC (permalink / raw)
  To: Jesper Juhl; +Cc: Ingo Molnar, linux-kernel, Pavel Emelyanov, Eric W. Biederman

Ugh! I'm not sure I really understand what Coverity says, to the
point I am not sure where it thinks the bug is, but

On 04/16, Jesper Juhl wrote:
> 
> At conditional (16): "(p)->pid != 0" taking false path
> 
> 1331 		if (likely(p->pid)) {
> 1332 			add_parent(p);
> 1333 			if (unlikely(p->ptrace & PT_PTRACED))
> 1334 				__ptrace_link(p, current->parent);
> 1335 	
> 1336 			if (thread_group_leader(p)) {
> 1337 				if (clone_flags & CLONE_NEWPID)
> 1338 					p->nsproxy->pid_ns->child_reaper = p;
> 1339 	
> 1340 				p->signal->leader_pid = pid;
> 1341 				p->signal->tty = current->signal->tty;
> 1342 				set_task_pgrp(p, task_pgrp_nr(current));
> 1343 				set_task_session(p, task_session_nr(current));
> 1344 				attach_pid(p, PIDTYPE_PGID, task_pgrp(current));
> 1345 				attach_pid(p, PIDTYPE_SID, task_session(current));
> 1346 				list_add_tail_rcu(&p->tasks, &init_task.tasks);
> 1347 				__get_cpu_var(process_counts)++;
> 1348 			}
> 1349 			attach_pid(p, PIDTYPE_PID, pid);
> 1350 			nr_threads++;
> 1351 		}
> 1352 	
> 1353 		total_forks++;
> 1354 		spin_unlock(&current->sighand->siglock);
> 1355 		write_unlock_irq(&tasklist_lock);
> 1356 		proc_fork_connector(p);
> 1357 		cgroup_post_fork(p);
> 
> Event leaked_storage: Returned without freeing storage "pid"
> Also see events: [alloc_fn][var_assign][pass_arg]

this looks like a false alarm.

p->pid == pid->numbers[0].nr. If "struct pid *pid" was allocated,
its .nr can't be 0.

IOW, !p->pid means that pid == init_struct_pid, it wasn't allocated
but was passed from the caller.

Oleg.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Possible mem leak in copy_process()
  2008-04-17 11:57 ` Oleg Nesterov
@ 2008-04-17 13:02   ` Ingo Molnar
  2008-04-17 14:02     ` fork_idle && pid problems ? (was: Possible mem leak in copy_process()) Oleg Nesterov
  0 siblings, 1 reply; 9+ messages in thread
From: Ingo Molnar @ 2008-04-17 13:02 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Jesper Juhl, linux-kernel, Pavel Emelyanov, Eric W. Biederman


* Oleg Nesterov <oleg@tv-sign.ru> wrote:

> > 1331 		if (likely(p->pid)) {

> > 1351 		}

> > Event leaked_storage: Returned without freeing storage "pid" Also 
> > see events: [alloc_fn][var_assign][pass_arg]
> 
> this looks like a false alarm.
> 
> p->pid == pid->numbers[0].nr. If "struct pid *pid" was allocated, its 
> .nr can't be 0.
> 
> IOW, !p->pid means that pid == init_struct_pid, it wasn't allocated 
> but was passed from the caller.

should we perhaps codify this rule via adding something like this to the 
else branch:

   WARN_ON_ONCE(task_pid(p) != &init_struct_pid);

?

	Ingo

^ permalink raw reply	[flat|nested] 9+ messages in thread

* fork_idle && pid problems ? (was: Possible mem leak in copy_process())
  2008-04-17 13:02   ` Ingo Molnar
@ 2008-04-17 14:02     ` Oleg Nesterov
  2008-04-17 15:50       ` fork_idle && pid problems ? Pavel Emelyanov
  0 siblings, 1 reply; 9+ messages in thread
From: Oleg Nesterov @ 2008-04-17 14:02 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Jesper Juhl, linux-kernel, Pavel Emelyanov, Eric W. Biederman,
	Sukadev Bhattiprolu

On 04/17, Ingo Molnar wrote:
> 
> * Oleg Nesterov <oleg@tv-sign.ru> wrote:
> 
> > > 1331 		if (likely(p->pid)) {
> 
> > > 1351 		}
> 
> > > Event leaked_storage: Returned without freeing storage "pid" Also 
> > > see events: [alloc_fn][var_assign][pass_arg]
> > 
> > this looks like a false alarm.
> > 
> > p->pid == pid->numbers[0].nr. If "struct pid *pid" was allocated, its 
> > .nr can't be 0.
> > 
> > IOW, !p->pid means that pid == init_struct_pid, it wasn't allocated 
> > but was passed from the caller.
> 
> should we perhaps codify this rule via adding something like this to the 
> else branch:
> 
>    WARN_ON_ONCE(task_pid(p) != &init_struct_pid);
> 
> ?

Perhaps yes, I don't know...

But please note that we heavily rely on the fact that nobody except idle
threads can have pid_nr == 0, and more importantly, each "struct pid" must
have the unique .nr withing the same namespace (init_pid_ns in this case).
I'd suggest to just add a small comment.


But wait... What _is_ the task_pid() after fork_idle() ???

fork_idle() doesn't really attach the new thread to the init_struct_pid,
so ->pids[PIDTYPE_PID].pid just points the parent's pid, no?

As for x86, the parent is /sbin/init (kernel_init->smp_prepare_cpus),
not so bad, it can't exit.

But what about HOTPLUG_CPU? Suppose we add CPU, use some non-idle
kernel thread (workqueue) to fork the idle thread. CPU goes down,
parent exits and frees the pid. Now, if this CPU goes up again, the
idle thread runs with its ->pid pointing to the freed memory, not
good.

Not serious perhaps, afaics we only need this ->pid to ensure that
swapper can safely fork /sbin/init, but still.

Pavel, Eric, Sukadev? Please say I missed something! ;)

Otherwise, we can change init_idle() to do attach_pid(init_struct_pid),
afaics we can do this lockless. In that case we should also change
INIT_STRUCT_PID() and remove the initialization of .tasks.

Oleg.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: fork_idle && pid problems ?
  2008-04-17 15:50       ` fork_idle && pid problems ? Pavel Emelyanov
@ 2008-04-17 15:36         ` Oleg Nesterov
  2008-04-17 16:40           ` Pavel Emelyanov
  0 siblings, 1 reply; 9+ messages in thread
From: Oleg Nesterov @ 2008-04-17 15:36 UTC (permalink / raw)
  To: Pavel Emelyanov
  Cc: Ingo Molnar, Jesper Juhl, linux-kernel, Eric W. Biederman,
	Sukadev Bhattiprolu

On 04/17, Pavel Emelyanov wrote:
>
> Oleg Nesterov wrote:
> > 
> > But wait... What _is_ the task_pid() after fork_idle() ???
> 
> It is NULL, but every code getting one can handle such case :)
> 
> > fork_idle() doesn't really attach the new thread to the init_struct_pid,
> > so ->pids[PIDTYPE_PID].pid just points the parent's pid, no?
> > 
> > As for x86, the parent is /sbin/init (kernel_init->smp_prepare_cpus),
> > not so bad, it can't exit.
> > 
> > But what about HOTPLUG_CPU? Suppose we add CPU, use some non-idle
> > kernel thread (workqueue) to fork the idle thread. CPU goes down,
> > parent exits and frees the pid. Now, if this CPU goes up again, the
> > idle thread runs with its ->pid pointing to the freed memory, not
> > good.
> 
> Nope - it will be NULL.

How so? I bet it won't be NULL...

	dup_task_struct:

		*tsk = *orig;

After that the child's ->pids[PIDTYPE_MAX] is a copy of parent's.
But the task is not attached to these pids.

> > Not serious perhaps, afaics we only need this ->pid to ensure that
> > swapper can safely fork /sbin/init, but still.
> > 
> > Pavel, Eric, Sukadev? Please say I missed something! ;)
> > 
> > Otherwise, we can change init_idle() to do attach_pid(init_struct_pid),
> > afaics we can do this lockless. In that case we should also change
> > INIT_STRUCT_PID() and remove the initialization of .tasks.
> 
> Well, these was some request to make tasks always have pid link
> point to not NULL (from Matt?) so we'll need this :)

For now I'd suggest the patch below. If contrary to our expectations
there is any usage of idle_task->pids, we will notice ;)

Oleg.

--- kernel/fork.c~	2008-03-07 18:11:27.000000000 +0300
+++ kernel/fork.c	2008-04-17 19:34:10.000000000 +0400
@@ -1420,6 +1420,9 @@ struct task_struct * __cpuinit fork_idle
 	if (!IS_ERR(task))
 		init_idle(task, cpu);
 
+	/* COMMENT */
+	memset(task->pids, 0, sizeof task->pids);
+
 	return task;
 }
 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: fork_idle && pid problems ?
  2008-04-17 14:02     ` fork_idle && pid problems ? (was: Possible mem leak in copy_process()) Oleg Nesterov
@ 2008-04-17 15:50       ` Pavel Emelyanov
  2008-04-17 15:36         ` Oleg Nesterov
  0 siblings, 1 reply; 9+ messages in thread
From: Pavel Emelyanov @ 2008-04-17 15:50 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Ingo Molnar, Jesper Juhl, linux-kernel, Eric W. Biederman,
	Sukadev Bhattiprolu

Oleg Nesterov wrote:
> On 04/17, Ingo Molnar wrote:
>> * Oleg Nesterov <oleg@tv-sign.ru> wrote:
>>
>>>> 1331 		if (likely(p->pid)) {
>>>> 1351 		}
>>>> Event leaked_storage: Returned without freeing storage "pid" Also 
>>>> see events: [alloc_fn][var_assign][pass_arg]
>>> this looks like a false alarm.
>>>
>>> p->pid == pid->numbers[0].nr. If "struct pid *pid" was allocated, its 
>>> .nr can't be 0.
>>>
>>> IOW, !p->pid means that pid == init_struct_pid, it wasn't allocated 
>>> but was passed from the caller.
>> should we perhaps codify this rule via adding something like this to the 
>> else branch:
>>
>>    WARN_ON_ONCE(task_pid(p) != &init_struct_pid);
>>
>> ?
> 
> Perhaps yes, I don't know...
> 
> But please note that we heavily rely on the fact that nobody except idle
> threads can have pid_nr == 0, and more importantly, each "struct pid" must
> have the unique .nr withing the same namespace (init_pid_ns in this case).
> I'd suggest to just add a small comment.
> 
> 
> But wait... What _is_ the task_pid() after fork_idle() ???

It is NULL, but every code getting one can handle such case :)

> fork_idle() doesn't really attach the new thread to the init_struct_pid,
> so ->pids[PIDTYPE_PID].pid just points the parent's pid, no?
> 
> As for x86, the parent is /sbin/init (kernel_init->smp_prepare_cpus),
> not so bad, it can't exit.
> 
> But what about HOTPLUG_CPU? Suppose we add CPU, use some non-idle
> kernel thread (workqueue) to fork the idle thread. CPU goes down,
> parent exits and frees the pid. Now, if this CPU goes up again, the
> idle thread runs with its ->pid pointing to the freed memory, not
> good.

Nope - it will be NULL.

> Not serious perhaps, afaics we only need this ->pid to ensure that
> swapper can safely fork /sbin/init, but still.
> 
> Pavel, Eric, Sukadev? Please say I missed something! ;)
> 
> Otherwise, we can change init_idle() to do attach_pid(init_struct_pid),
> afaics we can do this lockless. In that case we should also change
> INIT_STRUCT_PID() and remove the initialization of .tasks.

Well, these was some request to make tasks always have pid link
point to not NULL (from Matt?) so we'll need this :)

> Oleg.
> 
> 

Thanks,
Pavel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: fork_idle && pid problems ?
  2008-04-17 16:40           ` Pavel Emelyanov
@ 2008-04-17 16:05             ` Oleg Nesterov
  0 siblings, 0 replies; 9+ messages in thread
From: Oleg Nesterov @ 2008-04-17 16:05 UTC (permalink / raw)
  To: Pavel Emelyanov
  Cc: Ingo Molnar, Jesper Juhl, linux-kernel, Eric W. Biederman,
	Sukadev Bhattiprolu

On 04/17, Pavel Emelyanov wrote:
>
> Oleg Nesterov wrote:
> >> Well, these was some request to make tasks always have pid link
> >> point to not NULL (from Matt?) so we'll need this :)
> > 
> > For now I'd suggest the patch below. If contrary to our expectations
> > there is any usage of idle_task->pids, we will notice ;)
> > 
> > Oleg.
> > 
> > --- kernel/fork.c~	2008-03-07 18:11:27.000000000 +0300
> > +++ kernel/fork.c	2008-04-17 19:34:10.000000000 +0400
> > @@ -1420,6 +1420,9 @@ struct task_struct * __cpuinit fork_idle
> >  	if (!IS_ERR(task))
> >  		init_idle(task, cpu);
> >  
> > +	/* COMMENT */
> > +	memset(task->pids, 0, sizeof task->pids);
> > +
> 
> Hm... Looks ok, but I'd suggest such patch instead:
> 
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1348,6 +1348,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  		}
>  		attach_pid(p, PIDTYPE_PID, pid);
>  		nr_threads++;
> +	} else {
> +		p->pids[PIDTYPE_PID].pid = NULL;
> +		p->pids[PIDTYPE_SID].pid = NULL;
> +		p->pids[PIDTYPE_PGID].pid = NULL;
>  	}

This penalizes the "normal" fork()...

> it will cover cases, when we (if ever) call the copy_process from
> other place. Oh, well...

We must not use init_struct_pid as an argument for copy_process(), except
in fork_idle(). Note that the result of copy_process(init_struct_pid) is
not "visible", we can't find it via find_pid() or see it on init_task.tasks.

Not that I have a strong opinion, though. In any case, I think this is
harmless. But "not good" anyway.

Oleg.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: fork_idle && pid problems ?
  2008-04-17 15:36         ` Oleg Nesterov
@ 2008-04-17 16:40           ` Pavel Emelyanov
  2008-04-17 16:05             ` Oleg Nesterov
  0 siblings, 1 reply; 9+ messages in thread
From: Pavel Emelyanov @ 2008-04-17 16:40 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Ingo Molnar, Jesper Juhl, linux-kernel, Eric W. Biederman,
	Sukadev Bhattiprolu

Oleg Nesterov wrote:
> On 04/17, Pavel Emelyanov wrote:
>> Oleg Nesterov wrote:
>>> But wait... What _is_ the task_pid() after fork_idle() ???
>> It is NULL, but every code getting one can handle such case :)
>>
>>> fork_idle() doesn't really attach the new thread to the init_struct_pid,
>>> so ->pids[PIDTYPE_PID].pid just points the parent's pid, no?
>>>
>>> As for x86, the parent is /sbin/init (kernel_init->smp_prepare_cpus),
>>> not so bad, it can't exit.
>>>
>>> But what about HOTPLUG_CPU? Suppose we add CPU, use some non-idle
>>> kernel thread (workqueue) to fork the idle thread. CPU goes down,
>>> parent exits and frees the pid. Now, if this CPU goes up again, the
>>> idle thread runs with its ->pid pointing to the freed memory, not
>>> good.
>> Nope - it will be NULL.
> 
> How so? I bet it won't be NULL...
> 
> 	dup_task_struct:
> 
> 		*tsk = *orig;
> 
> After that the child's ->pids[PIDTYPE_MAX] is a copy of parent's.
> But the task is not attached to these pids.

Ouch... Indeed.

>>> Not serious perhaps, afaics we only need this ->pid to ensure that
>>> swapper can safely fork /sbin/init, but still.
>>>
>>> Pavel, Eric, Sukadev? Please say I missed something! ;)
>>>
>>> Otherwise, we can change init_idle() to do attach_pid(init_struct_pid),
>>> afaics we can do this lockless. In that case we should also change
>>> INIT_STRUCT_PID() and remove the initialization of .tasks.
>> Well, these was some request to make tasks always have pid link
>> point to not NULL (from Matt?) so we'll need this :)
> 
> For now I'd suggest the patch below. If contrary to our expectations
> there is any usage of idle_task->pids, we will notice ;)
> 
> Oleg.
> 
> --- kernel/fork.c~	2008-03-07 18:11:27.000000000 +0300
> +++ kernel/fork.c	2008-04-17 19:34:10.000000000 +0400
> @@ -1420,6 +1420,9 @@ struct task_struct * __cpuinit fork_idle
>  	if (!IS_ERR(task))
>  		init_idle(task, cpu);
>  
> +	/* COMMENT */
> +	memset(task->pids, 0, sizeof task->pids);
> +

Hm... Looks ok, but I'd suggest such patch instead:

--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1348,6 +1348,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		}
 		attach_pid(p, PIDTYPE_PID, pid);
 		nr_threads++;
+	} else {
+		p->pids[PIDTYPE_PID].pid = NULL;
+		p->pids[PIDTYPE_SID].pid = NULL;
+		p->pids[PIDTYPE_PGID].pid = NULL;
 	}
 
 	total_forks++;

it will cover cases, when we (if ever) call the copy_process from
other place. Oh, well...

>  	return task;
>  }
>  
> 
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2008-04-17 17:08 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-04-16 21:45 Possible mem leak in copy_process() Jesper Juhl
2008-04-17  4:17 ` Arnd Bergmann
2008-04-17 11:57 ` Oleg Nesterov
2008-04-17 13:02   ` Ingo Molnar
2008-04-17 14:02     ` fork_idle && pid problems ? (was: Possible mem leak in copy_process()) Oleg Nesterov
2008-04-17 15:50       ` fork_idle && pid problems ? Pavel Emelyanov
2008-04-17 15:36         ` Oleg Nesterov
2008-04-17 16:40           ` Pavel Emelyanov
2008-04-17 16:05             ` Oleg Nesterov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).