lkml.org 
[lkml]   [2021]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] aio: fix use-after-free due to missing POLLFREE handling
On Mon, Dec 06, 2021 at 11:28:13AM -0800, Linus Torvalds wrote:
> On Fri, Dec 3, 2021 at 4:23 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > require another solution. This solution is for the queue to be cleared
> > before it is freed, using 'wake_up_poll(wq, EPOLLHUP | POLLFREE);'.
>
> Ugh.
>
> I hate POLLFREE, and the more I look at this, the more I think it's broken.
>
> And that
>
> wake_up_poll(wq, EPOLLHUP | POLLFREE);
>
> in particular looks broken - the intent is that it should remove all
> the wait queue entries (because the wait queue head is going away),
> but wake_up_poll() iself actually does
>
> __wake_up(x, TASK_NORMAL, 1, poll_to_key(m))
>
> where that '1' is the number of exclusive entries it will wake up.
>
> So if there are two exclusive waiters, wake_up_poll() will simply stop
> waking things up after the first one.
>
> Which defeats the whole POLLFREE thing too.
>
> Maybe I'm missing something, but POLLFREE really is broken.
>
> I'd argue that all of epoll() is broken, but I guess we're stuck with it.
>
> Now, it's very possible that nobody actually uses exclusive waits for
> those wait queues, and my "nr_exclusive" argument is about something
> that isn't actually a bug in reality. But I think it's a sign of
> confusion, and it's just another issue with POLLFREE.
>
> I really wish we could have some way to not have epoll and aio mess
> with the wait-queue lists and cache the wait queue head pointers that
> they don't own.
>
> In the meantime, I don't think these patches make things worse, and
> they may fix things. But see above about "nr_exclusive" and how I
> think wait queue entries might end up avoiding POLLFREE handling..
>
> Linus

epoll supports exclusive waits, via the EPOLLEXCLUSIVE flag. So this looks like
a real problem.

It could be fixed by converting signalfd and binder to use something like this,
right?

#define wake_up_pollfree(x) \
__wake_up(x, TASK_NORMAL, 0, poll_to_key(EPOLLHUP | POLLFREE))

As for eliminating POLLFREE entirely, that would require that the waitqueue
heads be moved to a location which has a longer lifetime. I'm not sure if
that's possible. In the case of signalfd, maybe the waitqueue head could be
moved to the file private data (signalfd_ctx), and then sighand_struct would
contain a list of signalfd_ctx's which are receiving signals directed to that
sighand_struct, rather than the waitqueue head itself. I'm not sure how well
that would work. This would probably change user-visible behavior; if a
signalfd is inherited by fork(), the child process would be notified about
signals sent to the parent process, rather than itself as is currently the case.

- Eric

\
 
 \ /
  Last update: 2021-12-06 20:54    [W:0.339 / U:0.572 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site