diff mbox series

[RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful

Message ID xm26pm6hvfer.fsf@google.com
State New
Headers
Series [RESEND] epoll: ep_autoremove_wake_function should use list_del_init_careful |

Commit Message

Benjamin Segall May 30, 2023, 6:32 p.m. UTC 
  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(-)

Comments

Eric Biggers May 31, 2023, 1:57 a.m. UTC | #1 
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
Christian Brauner May 31, 2023, 7:53 a.m. UTC | #2 
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.
Benjamin Segall May 31, 2023, 10:15 p.m. UTC | #3 
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.
Andrew Morton May 31, 2023, 10:26 p.m. UTC | #4 
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 mbox series

Patch

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