From: Matthew Wilcox <willy@infradead.org> To: Kirill Tkhai <ktkhai@virtuozzo.com> Cc: jlayton@kernel.org, bfields@fieldses.org, viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, boqun.feng@gmail.com, longman@redhat.com, peterz@infradead.org, mingo@redhat.com Subject: Re: [PATCH] fasync: Fix deadlock between task-context and interrupt-context kill_fasync() Date: Tue, 17 Apr 2018 07:01:10 -0700 Message-ID: <20180417140110.GB21954@bombadil.infradead.org> (raw) In-Reply-To: <152292939368.19745.13784475656016424647.stgit@localhost.localdomain> On Thu, Apr 05, 2018 at 02:58:06PM +0300, Kirill Tkhai wrote: > I observed the following deadlock between them: > > [task 1] [task 2] [task 3] > kill_fasync() mm_update_next_owner() copy_process() > spin_lock_irqsave(&fa->fa_lock) read_lock(&tasklist_lock) write_lock_irq(&tasklist_lock) > send_sigio() <IRQ> ... > read_lock(&fown->lock) kill_fasync() ... > read_lock(&tasklist_lock) spin_lock_irqsave(&fa->fa_lock) ... > > Task 1 can't acquire read locked tasklist_lock, since there is > already task 3 expressed its wish to take the lock exclusive. > Task 2 holds the read locked lock, but it can't take the spin lock. I think the important question is to Peter ... why didn't lockdep catch this? > - spin_lock_irq(&fa->fa_lock); > + write_lock_irq(&fa->fa_lock); > fa->fa_file = NULL; > - spin_unlock_irq(&fa->fa_lock); > + write_unlock_irq(&fa->fa_lock); ... > - spin_lock_irq(&fa->fa_lock); > + write_lock_irq(&fa->fa_lock); > fa->fa_fd = fd; > - spin_unlock_irq(&fa->fa_lock); > + write_unlock_irq(&fa->fa_lock); Do we really need a lock here? If we convert each of these into WRITE_ONCE, then ... > - spin_lock_irqsave(&fa->fa_lock, flags); > + read_lock(&fa->fa_lock); > if (fa->fa_file) { file = READ_ONCE(fa->fa_file) then we're not opening any new races, are we? > fown = &fa->fa_file->f_owner; > /* Don't send SIGURG to processes which have not set a > @@ -997,7 +996,7 @@ static void kill_fasync_rcu(struct fasync_struct *fa, int sig, int band) > if (!(sig == SIGURG && fown->signum == 0)) > send_sigio(fown, fa->fa_fd, band); > } > - spin_unlock_irqrestore(&fa->fa_lock, flags); > + read_unlock(&fa->fa_lock); > fa = rcu_dereference(fa->fa_next); > } > } > diff --git a/include/linux/fs.h b/include/linux/fs.h > index c6baf767619e..297e2dcd9dd2 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1250,7 +1250,7 @@ static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl) > } > > struct fasync_struct { > - spinlock_t fa_lock; > + rwlock_t fa_lock; > int magic; > int fa_fd; > struct fasync_struct *fa_next; /* singly linked list */ >
next prev parent reply index Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-05 11:58 Kirill Tkhai 2018-04-17 9:04 ` Kirill Tkhai 2018-04-17 11:42 ` Jeff Layton 2018-04-17 11:53 ` Kirill Tkhai 2018-04-17 13:31 ` Jeff Layton 2018-04-17 13:59 ` Kirill Tkhai 2018-04-17 14:01 ` Matthew Wilcox [this message] 2018-04-17 14:15 ` Kirill Tkhai 2018-04-18 20:00 ` Jeff Layton 2018-04-18 22:40 ` Kirill Tkhai 2018-04-27 13:44 ` Boqun Feng
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=20180417140110.GB21954@bombadil.infradead.org \ --to=willy@infradead.org \ --cc=bfields@fieldses.org \ --cc=boqun.feng@gmail.com \ --cc=jlayton@kernel.org \ --cc=ktkhai@virtuozzo.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=longman@redhat.com \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --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
LKML Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \ linux-kernel@vger.kernel.org public-inbox-index lkml Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git