Mailing List Archive: Linux: Kernel

fork_idle && pid problems ? (was: Possible mem leak in copy_process())

Index | Next | Previous | View Threaded

oleg at tv-sign

Apr 17, 2008, 7:02 AM

Post #1 of 5 (826 views)
Permalink

fork_idle && pid problems ? (was: Possible mem leak in copy_process())

On 04/17, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg [at] tv-sign> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

oleg at tv-sign

Apr 17, 2008, 8:36 AM

Post #2 of 5 (753 views)
Permalink

Re: fork_idle && pid problems ? [In reply to]

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;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

xemul at openvz

Apr 17, 2008, 8:50 AM

Post #3 of 5 (755 views)
Permalink

Re: fork_idle && pid problems ? [In reply to]

Oleg Nesterov wrote:
> On 04/17, Ingo Molnar wrote:
>> * Oleg Nesterov <oleg [at] tv-sign> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

oleg at tv-sign

Apr 17, 2008, 9:05 AM

Post #4 of 5 (755 views)
Permalink

Re: fork_idle && pid problems ? [In reply to]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

xemul at openvz

Apr 17, 2008, 9:40 AM

Post #5 of 5 (752 views)
Permalink

Re: fork_idle && pid problems ? [In reply to]

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;
> }
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads