| Message ID | xm26pm6hvfer.fsf@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful | expand |
On Tue, May 30, 2023 at 11:32:28AM -0700, Benjamin Segall wrote: > autoremove_wake_function uses list_del_init_careful, so should epoll's > more aggressive variant. It only doesn't because it was copied from an > older wait.c rather than the most recent. > > Fixes: a16ceb139610 ("epoll: autoremove wakers even more aggressively") > Signed-off-by: Ben Segall <bsegall@google.com> > Cc: stable@vger.kernel.org > --- > fs/eventpoll.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/eventpoll.c b/fs/eventpoll.c > index 52954d4637b5..081df056398a 100644 > --- a/fs/eventpoll.c > +++ b/fs/eventpoll.c > @@ -1756,11 +1756,11 @@ static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms) > static int ep_autoremove_wake_function(struct wait_queue_entry *wq_entry, > unsigned int mode, int sync, void *key) > { > int ret = default_wake_function(wq_entry, mode, sync, key); > > - list_del_init(&wq_entry->entry); > + list_del_init_careful(&wq_entry->entry); > return ret; > } Can you please provide a more detailed explanation about why list_del_init_careful() is needed here? - Eric
On Tue, May 30, 2023 at 06:57:48PM -0700, Eric Biggers wrote: > On Tue, May 30, 2023 at 11:32:28AM -0700, Benjamin Segall wrote: > > autoremove_wake_function uses list_del_init_careful, so should epoll's > > more aggressive variant. It only doesn't because it was copied from an > > older wait.c rather than the most recent. > > > > Fixes: a16ceb139610 ("epoll: autoremove wakers even more aggressively") > > Signed-off-by: Ben Segall <bsegall@google.com> > > Cc: stable@vger.kernel.org > > --- > > fs/eventpoll.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/eventpoll.c b/fs/eventpoll.c > > index 52954d4637b5..081df056398a 100644 > > --- a/fs/eventpoll.c > > +++ b/fs/eventpoll.c > > @@ -1756,11 +1756,11 @@ static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms) > > static int ep_autoremove_wake_function(struct wait_queue_entry *wq_entry, > > unsigned int mode, int sync, void *key) > > { > > int ret = default_wake_function(wq_entry, mode, sync, key); > > > > - list_del_init(&wq_entry->entry); > > + list_del_init_careful(&wq_entry->entry); > > return ret; > > } > > Can you please provide a more detailed explanation about why > list_del_init_careful() is needed here? Yeah, this needs more explanation... Next time someone looks at this code and there's a *_careful() added they'll want to know why.
Christian Brauner <brauner@kernel.org> writes: > On Tue, May 30, 2023 at 06:57:48PM -0700, Eric Biggers wrote: >> On Tue, May 30, 2023 at 11:32:28AM -0700, Benjamin Segall wrote: >> > autoremove_wake_function uses list_del_init_careful, so should epoll's >> > more aggressive variant. It only doesn't because it was copied from an >> > older wait.c rather than the most recent. >> > >> > Fixes: a16ceb139610 ("epoll: autoremove wakers even more aggressively") >> > Signed-off-by: Ben Segall <bsegall@google.com> >> > Cc: stable@vger.kernel.org >> > --- >> > fs/eventpoll.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/fs/eventpoll.c b/fs/eventpoll.c >> > index 52954d4637b5..081df056398a 100644 >> > --- a/fs/eventpoll.c >> > +++ b/fs/eventpoll.c >> > @@ -1756,11 +1756,11 @@ static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms) >> > static int ep_autoremove_wake_function(struct wait_queue_entry *wq_entry, >> > unsigned int mode, int sync, void *key) >> > { >> > int ret = default_wake_function(wq_entry, mode, sync, key); >> > >> > - list_del_init(&wq_entry->entry); >> > + list_del_init_careful(&wq_entry->entry); >> > return ret; >> > } >> >> Can you please provide a more detailed explanation about why >> list_del_init_careful() is needed here? > > Yeah, this needs more explanation... Next time someone looks at this > code and there's a *_careful() added they'll want to know why. So the general reason is the same as with autoremove_wake_function, it pairs with the list_entry_careful in ep_poll (which is epoll's modified copy of finish_wait). I think the original actual _problem_ was a -stable issue that was fixed instead by doing additional backports, so this may just avoid potential extra loops and avoid potential compiler shenanigans from the data race.
On Wed, 31 May 2023 15:15:41 -0700 Benjamin Segall <bsegall@google.com> wrote: > >> Can you please provide a more detailed explanation about why > >> list_del_init_careful() is needed here? > > > > Yeah, this needs more explanation... Next time someone looks at this > > code and there's a *_careful() added they'll want to know why. > > So the general reason is the same as with autoremove_wake_function, it > pairs with the list_entry_careful in ep_poll (which is epoll's modified > copy of finish_wait). > > I think the original actual _problem_ was a -stable issue that was fixed > instead by doing additional backports, so this may just avoid potential > extra loops and avoid potential compiler shenanigans from the data race. The point is that the foo_careful() callsites should be commented, please.
diff --git a/fs/eventpoll.c b/fs/eventpoll.c index 52954d4637b5..081df056398a 100644 --- a/fs/eventpoll.c +++ b/fs/eventpoll.c @@ -1756,11 +1756,11 @@ static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms) static int ep_autoremove_wake_function(struct wait_queue_entry *wq_entry, unsigned int mode, int sync, void *key) { int ret = default_wake_function(wq_entry, mode, sync, key); - list_del_init(&wq_entry->entry); + list_del_init_careful(&wq_entry->entry); return ret; } /** * ep_poll - Retrieves ready events, and delivers them to the caller-supplied
autoremove_wake_function uses list_del_init_careful, so should epoll's more aggressive variant. It only doesn't because it was copied from an older wait.c rather than the most recent. Fixes: a16ceb139610 ("epoll: autoremove wakers even more aggressively") Signed-off-by: Ben Segall <bsegall@google.com> Cc: stable@vger.kernel.org --- fs/eventpoll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)