* [PATCH RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful
@ 2023-05-30 18:32 Benjamin Segall
2023-05-31 1:57 ` Eric Biggers
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Segall @ 2023-05-30 18:32 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner
Cc: linux-fsdevel, linux-kernel, stable, Andrew Morton
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;
}
/**
* ep_poll - Retrieves ready events, and delivers them to the caller-supplied
--
2.39.0.rc0.267.gcb52ba06e7-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful
2023-05-30 18:32 [PATCH RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful Benjamin Segall
@ 2023-05-31 1:57 ` Eric Biggers
2023-05-31 7:53 ` Christian Brauner
0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2023-05-31 1:57 UTC (permalink / raw)
To: Benjamin Segall
Cc: Alexander Viro, Christian Brauner, linux-fsdevel, linux-kernel,
stable, Andrew Morton
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful
2023-05-31 1:57 ` Eric Biggers
@ 2023-05-31 7:53 ` Christian Brauner
2023-05-31 22:15 ` Benjamin Segall
0 siblings, 1 reply; 6+ messages in thread
From: Christian Brauner @ 2023-05-31 7:53 UTC (permalink / raw)
To: Eric Biggers
Cc: Benjamin Segall, Alexander Viro, linux-fsdevel, linux-kernel,
stable, Andrew Morton
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful
2023-05-31 7:53 ` Christian Brauner
@ 2023-05-31 22:15 ` Benjamin Segall
2023-05-31 22:26 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Segall @ 2023-05-31 22:15 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric Biggers, Alexander Viro, linux-fsdevel, linux-kernel, stable,
Andrew Morton
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful
2023-05-31 22:15 ` Benjamin Segall
@ 2023-05-31 22:26 ` Andrew Morton
2023-05-31 23:24 ` [PATCH v2] " Benjamin Segall
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2023-05-31 22:26 UTC (permalink / raw)
To: Benjamin Segall
Cc: Christian Brauner, Eric Biggers, Alexander Viro, linux-fsdevel,
linux-kernel, stable
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] epoll: ep_autoremove_wake_function should use list_del_init_careful
2023-05-31 22:26 ` Andrew Morton
@ 2023-05-31 23:24 ` Benjamin Segall
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Segall @ 2023-05-31 23:24 UTC (permalink / raw)
To: Andrew Morton
Cc: Christian Brauner, Eric Biggers, Alexander Viro, linux-fsdevel,
linux-kernel, stable
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
Change-Id: Icca05359250297f091779c9dcf4fefea92ee8c93
---
fs/eventpoll.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 980483455cc09..266d45c7685b4 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1803,11 +1803,15 @@ 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);
+ /*
+ * Pairs with list_empty_careful in ep_poll, and ensures future loop
+ * iterations see the cause of this wakeup.
+ */
+ list_del_init_careful(&wq_entry->entry);
return ret;
}
/**
* ep_poll - Retrieves ready events, and delivers them to the caller-supplied
--
2.41.0.rc0.172.g3f132b7071-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-05-31 23:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-30 18:32 [PATCH RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful Benjamin Segall
2023-05-31 1:57 ` Eric Biggers
2023-05-31 7:53 ` Christian Brauner
2023-05-31 22:15 ` Benjamin Segall
2023-05-31 22:26 ` Andrew Morton
2023-05-31 23:24 ` [PATCH v2] " Benjamin Segall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).