linux-kernel - Re: free_pid() && PIDNS_HASH

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:	Mon, 09 Sep 2013 10:54:58 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	"Serge E. Hallyn" <serge@...lyn.com>,
	Serge Hallyn <serge.hallyn@...ntu.com>,
	linux-kernel@...r.kernel.org
Subject: Re: free_pid() && PIDNS_HASH_ADDING

Oleg Nesterov <oleg@...hat.com> writes:

> On 09/08, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@...hat.com> writes:
>>
>> > On 09/08, Oleg Nesterov wrote:
>> >>
>> >> Off topic. What if the first alloc_pid() succeeds and then later
>> >> copy_process() fails. In this case free_pid() is called but
>> >> PIDNS_HASH_ADDING was not cleared, we miss kern_unmount(), no?
>> >
>> > Perhaps something like below?
>>
>> I am thinking more:
>>
>> diff --git a/kernel/pid.c b/kernel/pid.c
>> index ab75add..ef59516 100644
>> --- a/kernel/pid.c
>> +++ b/kernel/pid.c
>> @@ -273,6 +273,10 @@ void free_pid(struct pid *pid)
>>                          */
>>                         wake_up_process(ns->child_reaper);
>>                         break;
>> +               case PIDNS_HASH_ADDING:
>> +                       /* Handle a fork failure of the first process */
>> +                       ns->nr_hashed = 0;
>
> Agreed, it also makes sense to clear ->nr_hashed. But I still think
> that WARN_ON(ns->child_reaper) makes sense too.

I don't know that I like warnings for impossible conditions.  How could
we even make a mistake that gets us there?

>> At which point I ask myself what of the pathlogocical case where the
>> first fork fails but because we created the pid namespace with unshare
>> there is a concurrent fork from another process into the pid namespace
>> that succeeds.  Resulting in one pid in the pid namespace that is not
>> the reaper.
>
> But how can setns() work before the first fork() succeeds and makes the
> ->child_reaper visible in /proc ?
>
> Probably I missed something obvious, I didn't sleep today...

Actually that is a very good point.  That is an accidental feature but
one I very much appreciate today.

Of course this leads me to the question of what the checkpoint/restart
guys can do about checkpointing that properly. Sigh.

Eric
--
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/