From: David Howells <dhowells@redhat.com> To: Siddh Raman Pant <code@siddh.me> Cc: dhowells@redhat.com, "Greg KH" <gregkh@linuxfoundation.org>, "Christophe JAILLET" <christophe.jaillet@wanadoo.fr>, "Eric Dumazet" <edumazet@google.com>, "Fabio M. De Francesco" <fmdefrancesco@gmail.com>, "linux-security-modules" <linux-security-module@vger.kernel.org>, "linux-kernel-mentees" <linux-kernel-mentees@lists.linuxfoundation.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "syzbot+c70d87ac1d001f29a058" <syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com> Subject: Re: [PATCH] kernel/watch_queue: Make pipe NULL while clearing watch_queue Date: Wed, 27 Jul 2022 15:46:40 +0100 [thread overview] Message-ID: <3558070.1658933200@warthog.procyon.org.uk> (raw) In-Reply-To: <1822b768504.1d4e377e236061.5518350412857967240@siddh.me> Siddh Raman Pant <code@siddh.me> wrote: > Greg KH <gregkh@linuxfoundation.org> wrote: > > > - spin_unlock_bh(&wqueue->lock); > > > rcu_read_unlock(); > > > > Also you now have a spinlock held when calling rcu_read_unlock(), are > > you sure that's ok? Worse, we have softirqs disabled still, which might cause problems for rcu_read_unlock()? > We logically should not do write operations in a read critical section, so the > nulling of `wqueue->pipe->watch_queue` should happen after rcu_read_unlock(). > Also, since we already have a spinlock, we can use it to ensure the nulling. > So I think it is okay. Read/write locks are perhaps misnamed in this sense; they perhaps should be shared/exclusive. But, yes, we *can* do certain write operations with the lock held - if we're careful. Locks are required if we need to pairs of related memory accesses; if we're only making a single non-dependent write, then we don't necessarily need a write lock. However, you're referring to RCU read lock. That's a very special lock that has to do with maintenance of persistence of objects without taking any other lock. The moment you drop that lock, anything you accessed under RCU protocol rules should be considered to have evaporated. Think of it more as a way to have a deferred destructor/deallocator. So I would do: + + /* Clearing the watch queue, so we should clean the associated pipe. */ + if (wqueue->pipe) { + wqueue->pipe->watch_queue = NULL; + wqueue->pipe = NULL; + } + spin_unlock_bh(&wqueue->lock); rcu_read_unlock(); } However, since you're now changing wqueue->pipe whilst a notification is being posted, you need a barrier in post_one_notification() to prevent the compiler from reloading the value: struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); David
WARNING: multiple messages have this Message-ID (diff)
From: David Howells <dhowells@redhat.com> To: Siddh Raman Pant <code@siddh.me> Cc: syzbot+c70d87ac1d001f29a058 <syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com>, linux-security-modules <linux-security-module@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, dhowells@redhat.com, Eric Dumazet <edumazet@google.com>, Christophe JAILLET <christophe.jaillet@wanadoo.fr>, "Fabio M. De Francesco" <fmdefrancesco@gmail.com>, linux-kernel-mentees <linux-kernel-mentees@lists.linuxfoundation.org> Subject: Re: [PATCH] kernel/watch_queue: Make pipe NULL while clearing watch_queue Date: Wed, 27 Jul 2022 15:46:40 +0100 [thread overview] Message-ID: <3558070.1658933200@warthog.procyon.org.uk> (raw) In-Reply-To: <1822b768504.1d4e377e236061.5518350412857967240@siddh.me> Siddh Raman Pant <code@siddh.me> wrote: > Greg KH <gregkh@linuxfoundation.org> wrote: > > > - spin_unlock_bh(&wqueue->lock); > > > rcu_read_unlock(); > > > > Also you now have a spinlock held when calling rcu_read_unlock(), are > > you sure that's ok? Worse, we have softirqs disabled still, which might cause problems for rcu_read_unlock()? > We logically should not do write operations in a read critical section, so the > nulling of `wqueue->pipe->watch_queue` should happen after rcu_read_unlock(). > Also, since we already have a spinlock, we can use it to ensure the nulling. > So I think it is okay. Read/write locks are perhaps misnamed in this sense; they perhaps should be shared/exclusive. But, yes, we *can* do certain write operations with the lock held - if we're careful. Locks are required if we need to pairs of related memory accesses; if we're only making a single non-dependent write, then we don't necessarily need a write lock. However, you're referring to RCU read lock. That's a very special lock that has to do with maintenance of persistence of objects without taking any other lock. The moment you drop that lock, anything you accessed under RCU protocol rules should be considered to have evaporated. Think of it more as a way to have a deferred destructor/deallocator. So I would do: + + /* Clearing the watch queue, so we should clean the associated pipe. */ + if (wqueue->pipe) { + wqueue->pipe->watch_queue = NULL; + wqueue->pipe = NULL; + } + spin_unlock_bh(&wqueue->lock); rcu_read_unlock(); } However, since you're now changing wqueue->pipe whilst a notification is being posted, you need a barrier in post_one_notification() to prevent the compiler from reloading the value: struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); David _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2022-07-27 14:46 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-23 13:54 [PATCH] kernel/watch_queue: Make pipe NULL while clearing watch_queue Siddh Raman Pant 2022-07-23 13:54 ` Siddh Raman Pant via Linux-kernel-mentees 2022-07-23 14:03 ` Greg KH 2022-07-23 14:03 ` Greg KH 2022-07-23 14:29 ` Siddh Raman Pant via Linux-kernel-mentees 2022-07-23 14:29 ` Siddh Raman Pant 2022-07-24 3:45 ` Khalid Masum 2022-07-24 3:45 ` Khalid Masum 2022-07-24 4:02 ` Siddh Raman Pant 2022-07-24 4:02 ` Siddh Raman Pant via Linux-kernel-mentees 2022-07-23 14:04 ` Greg KH 2022-07-23 14:04 ` Greg KH 2022-07-23 14:29 ` Siddh Raman Pant via Linux-kernel-mentees 2022-07-23 14:29 ` Siddh Raman Pant 2022-07-27 14:46 ` David Howells [this message] 2022-07-27 14:46 ` David Howells 2022-07-27 16:20 ` Siddh Raman Pant 2022-07-27 16:20 ` Siddh Raman Pant via Linux-kernel-mentees 2022-07-31 18:11 ` Dipanjan Das 2022-07-31 18:11 ` Dipanjan Das 2022-07-31 18:46 ` Siddh Raman Pant 2022-07-31 18:46 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-01 8:47 ` Greg KH 2022-08-01 8:47 ` Greg KH 2022-08-01 8:53 ` Siddh Raman Pant 2022-08-01 8:53 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-01 21:06 ` Hillf Danton 2022-08-02 1:14 ` Siddh Raman Pant 2022-08-02 1:19 ` Siddh Raman Pant 2022-07-27 14:15 ` David Howells 2022-07-27 14:15 ` David Howells 2022-07-27 14:23 ` Siddh Raman Pant 2022-07-27 14:23 ` Siddh Raman Pant via Linux-kernel-mentees 2022-08-01 12:15 Hillf Danton 2022-08-01 12:52 ` Siddh Raman Pant 2022-08-01 12:52 ` Siddh Raman Pant via Linux-kernel-mentees
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=3558070.1658933200@warthog.procyon.org.uk \ --to=dhowells@redhat.com \ --cc=christophe.jaillet@wanadoo.fr \ --cc=code@siddh.me \ --cc=edumazet@google.com \ --cc=fmdefrancesco@gmail.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=syzbot+c70d87ac1d001f29a058@syzkaller.appspotmail.com \ /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: linkBe 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.