Re: proc_flush_task oops

[Eric W. Biederman]

Alexey Dobriyan <adobriyan@xxxxxxxxx> writes:

> On 12/22/17, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>> Dave Jones <davej@xxxxxxxxxxxxxxxxx> writes:
>>
>>> On Thu, Dec 21, 2017 at 07:31:26PM -0600, Eric W. Biederman wrote:
>>>  > Dave Jones <davej@xxxxxxxxxxxxxxxxx> writes:
>>>  >
>>>  > > On Thu, Dec 21, 2017 at 12:38:12PM +0200, Alexey Dobriyan wrote:
>>>  > >
>>>  > >  > > with proc_mnt still set to NULL is a mystery to me.
>>>  > >  > >
>>>  > >  > > Is there any chance the idr code doesn't always return the
>>> lowest valid
>>>  > >  > > free number?  So init gets assigned something other than 1?
>>>  > >  >
>>>  > >  > Well, this theory is easy to test (attached).
>>>  > >
>>>  > > I didn't hit this BUG, but I hit the same oops in proc_flush_task.
>>>  >
>>>  > Scratch one idea.
>>>  >
>>>  > If it isn't too much trouble can you try this.
>>>  >
>>>  > I am wondering if somehow the proc_mnt that is NULL is somewhere in
>>> the
>>>  > middle of the stack of pid namespaces.
>>>  >
>>>  > This adds two warnings.  The first just reports which pid namespace in
>>>  > the stack of pid namespaces is problematic, and the pid number in that
>>>  > pid namespace.  Which should give a whole lot more to go by.
>>>  >
>>>  > The second warning complains if we manage to create a pid namespace
>>>  > where the parent pid namespace is not properly set up.  The test to
>>>  > prevent that looks quite robust, but at this point I don't know where
>>> to
>>>  > look.
>>>
>>> Progress ?
>>>
>>> [ 1653.030190] ------------[ cut here ]------------
>>> [ 1653.030852] 1/1: 2 no proc_mnt
>>> [ 1653.030946] WARNING: CPU: 2 PID: 4420 at kernel/pid.c:213
>>> alloc_pid+0x24f/0x2a0
>>
>> Yes.  I don't know why Alexey's patch did not fire but this is
>> confirmation that the first pid allocated was #2 and not #1.
>>
>> Which explains the pid_mnt not being set, and it is definitely the new
>> code, changing from the pid bitmap+hash table to an idr.
>>
>> So it looks like idr_alloc_cyclic in some configuration for the first
>> allocation returns value #2 instead of value #1.
>>
>> I don't know that code, and it is quite complicated so I will have to
>> stare at it some more to even guess why it is doing that.
>>
>> This is confirmation that reverting those pid changes will fix the
>> problem.  As they are definitely at fault.
>>
>>
>> Hmm.  After a little more staring I have a hunch what is going wrong.
>> It is just possible that there is a failure in alloc_pid during the
>> first pid allocation and then idr_next gets left at 2.  I need to sleep
>> before I can think of a patch to test that.
>>
>> Hmm.  A failure and then restart would also explain why Alexey's patch
>> did not fire.  An incomplete reset of state.
>
> You are right (you are also right about sysctl :-\)
>
> unshare
> fork
>     alloc_pid in level 1 succeeds
>     alloc_pid in level 0 fails, ->idr_next is 2
> fork
>     alloc pid 2
> exit
>
> Reliable reproducer and fail injection patch attached
>
> I'd say proper fix is allocating pids in the opposite order
> so that failure in the last layer doesn't move IDR cursor
> in baby child pidns.

I agree with you about changing the order.  That will make
the code simpler and in the second loop actually conforming C code,
and fix the immediate problem.

I was worrying about the case where the mount of the proc filesystem
fails, but we call disable_pid_allocation in that case.  So we won't try
to allocate a pid again.  That seems better than any path where we might
have to reset the allocation state.



The nasty thing is that the pid bitmap+hashtable code also did not set
the pointer back and we did not have any problems.  AKA the bug is not
new.

Which means with the new code allocating pid numbers is failing much
more often when allocating pids than the old code ever did.  That is not
good.

Is it perhaps the GFP_ATOMIC in a context where we could otherwise
sleep?

Eric