Re: possible deadlock in send_sigurg

[Eric W. Biederman]

Christian Brauner <christian.brauner@xxxxxxxxxx> writes:

> On Fri, Apr 03, 2020 at 11:11:35AM +0200, Oleg Nesterov wrote:
>> On 04/02, syzbot wrote:
>> >
>> >                       lock_acquire+0x1f2/0x8f0 kernel/locking/lockdep.c:4923
>> >                       __raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
>> >                       _raw_spin_lock+0x2a/0x40 kernel/locking/spinlock.c:151
>> >                       spin_lock include/linux/spinlock.h:353 [inline]
>> >                       proc_pid_make_inode+0x1f9/0x3c0 fs/proc/base.c:1880
>> 
>> Yes, spin_lock(wait_pidfd.lock) is not safe...
>> 
>> Eric, at first glance the fix is simple.
>> 
>> Oleg.
>> 
>> 
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>
> Um, when did this lock get added to proc/base.c in the first place and
> why has it been abused for this?

Because struct pid is too bloated already.

> People just recently complained loudly about this in the
> cred_guard_mutex thread that abusing locks for things they weren't
> intended for is a bad idea...

The problem there is/was holding locks over places they shouldn't.
It looks like I made an equally dump mistake with struct pid.

That said can you take a look at calling putting do_notify_pidfd
someplace sane.  I can't see how it makes sense to call that in
the same set of circumstances where we notify the parent.

Reparenting should not be a concern, nor should ptracing.  Which I think
means that do_notify_pid can potentially get called many times more
than it needs to be.

Not to mention it is being called a bit too soon when called from
do_notify_parent.  Which I saw earlier is causing problems.  Signal
sending can call do_notify_parent early because everything just queues
up and no action is taken.  Wake-ups on the other hand trigger more
immediate action.

There is no connection to the current bug except this discussion
just remimded me about do_notify_pidfd and I figured I should say
something before I forget again.

Eric