All of lore.kernel.org
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Oleg Nesterov <oleg@redhat.com>,
	syzbot <syzbot+f675f964019f884dbd0f@syzkaller.appspotmail.com>,
	adobriyan@gmail.com, akpm@linux-foundation.org,
	allison@lohutok.net, areber@redhat.com,
	aubrey.li@linux.intel.com, avagin@gmail.com,
	bfields@fieldses.org, christian@brauner.io, cyphar@cyphar.com,
	gregkh@linuxfoundation.org, guro@fb.com, jlayton@kernel.org,
	joel@joelfernandes.org, keescook@chromium.org,
	linmiaohe@huawei.com, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, mhocko@suse.com, mingo@kernel.org,
	peterz@infradead.org, sargun@sargun.me,
	syzkaller-bugs@googlegroups.com, tglx@linutronix.de,
	viro@zeniv.linux.org.uk
Subject: Re: possible deadlock in send_sigurg
Date: Fri, 03 Apr 2020 07:57:04 -0500	[thread overview]
Message-ID: <87y2rc7mn3.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20200403093612.mtd7edubsng24uuh@wittgenstein> (Christian Brauner's message of "Fri, 3 Apr 2020 11:36:12 +0200")

Christian Brauner <christian.brauner@ubuntu.com> 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

  reply	other threads:[~2020-04-03 13:00 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-03  6:15 possible deadlock in send_sigurg syzbot
2020-04-03  9:11 ` Oleg Nesterov
2020-04-03  9:36   ` Christian Brauner
2020-04-03 12:57     ` Eric W. Biederman [this message]
2020-04-08 20:28   ` [PATCH] proc: Use a dedicated lock in struct pid Eric W. Biederman
2020-04-09  8:35     ` Christian Brauner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87y2rc7mn3.fsf@x220.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=allison@lohutok.net \
    --cc=areber@redhat.com \
    --cc=aubrey.li@linux.intel.com \
    --cc=avagin@gmail.com \
    --cc=bfields@fieldses.org \
    --cc=christian.brauner@ubuntu.com \
    --cc=christian@brauner.io \
    --cc=cyphar@cyphar.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=guro@fb.com \
    --cc=jlayton@kernel.org \
    --cc=joel@joelfernandes.org \
    --cc=keescook@chromium.org \
    --cc=linmiaohe@huawei.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhocko@suse.com \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=sargun@sargun.me \
    --cc=syzbot+f675f964019f884dbd0f@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    --cc=tglx@linutronix.de \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.