linux-kernel - fork_idle && pid problems ? (was: Possible mem leak in copy

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]

Message-ID: <20080417140246.GA257@tv-sign.ru>
Date:	Thu, 17 Apr 2008 18:02:46 +0400
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Jesper Juhl <jesper.juhl@...il.com>, linux-kernel@...r.kernel.org,
	Pavel Emelyanov <xemul@...nvz.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Sukadev Bhattiprolu <sukadev@...ibm.com>
Subject: fork_idle && pid problems ? (was: Possible mem leak in copy_process())

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() ???

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/