Re: free_pid() && PIDNS_HASH_ADDING

[ebiederm@xmission.com (Eric W. Biederman)]

Oleg Nesterov <oleg@redhat.com> writes:

> On 09/08, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.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