linux-kernel - Re: fork_idle && pid problems ?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 17 Apr 2008 19:50:52 +0400
From:	Pavel Emelyanov <xemul@...nvz.org>
To:	Oleg Nesterov <oleg@...sign.ru>
CC:	Ingo Molnar <mingo@...e.hu>, Jesper Juhl <jesper.juhl@...il.com>,
	linux-kernel@...r.kernel.org,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Sukadev Bhattiprolu <sukadev@...ibm.com>
Subject: Re: fork_idle && pid problems ?

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