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(¤t->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(¤t->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(¤t->signal->cpu_timers[0]) ||
1321 !list_empty(¤t->signal->cpu_timers[1]) ||
1322 !list_empty(¤t->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(¤t->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(¤t->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).