From: ebiederm@xmission.com (Eric W. Biederman)
To: Oleg Nesterov <oleg@redhat.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>,
        Serge Hallyn <serge.hallyn@ubuntu.com>, linux-kernel@vger.kernel.org
References: <20130829211114.GA20726@sergelap> <87mwo0xb9p.fsf@xmission.com>
	<20130830144232.GA18281@mail.hallyn.com>
	<87hae6vh0w.fsf_-_@xmission.com> <20130908175602.GA3172@redhat.com>
	<20130908180137.GA4915@redhat.com> <87bo43gg4h.fsf@xmission.com>
	<20130909151530.GA6755@redhat.com>
Date: Mon, 09 Sep 2013 10:54:58 -0700
In-Reply-To: <20130909151530.GA6755@redhat.com> (Oleg Nesterov's message of
	"Mon, 9 Sep 2013 17:15:30 +0200")
Message-ID: <87bo41j2x9.fsf@tw-ebiederman.twitter.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: free_pid() && PIDNS_HASH_ADDING
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2118
Lines: 57

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/