lkml.org 
[lkml]   [2008]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: fork_idle && pid problems ?
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


\
 
 \ /
  Last update: 2008-04-17 18:21    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site