[lkml]   [2019]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll)
On Sun, Mar 3, 2019 at 6:36 PM Al Viro <> wrote:
> OK, having dug through the archives, the reasons were not strong.
> So that part is OK...

I've committed the patch.

However, I didn't actually do the separate and independent cleanups:

> > @@ -1060,6 +1071,8 @@ static inline void iocb_put(struct aio_kiocb *iocb)
> > {
> > if (refcount_read(&iocb->ki_refcnt) == 0 ||
> > refcount_dec_and_test(&iocb->ki_refcnt)) {
> > + if (iocb->ki_filp)
> > + fput(iocb->ki_filp);
> > percpu_ref_put(&iocb->ki_ctx->reqs);
> > kmem_cache_free(kiocb_cachep, iocb);
> > }
> That reminds me - refcount_t here is rather ridiculous; what we
> have is
> * everything other than aio_poll: ki_refcnt is 0 all along
> * aio_poll: originally 0, then set to 2, then two iocb_put()
> are done (either both synchronous to aio_poll(), or one sync and one
> async).
> That's not a refcount at all. It's a flag, set only for aio_poll ones.
> And that test in iocb_put() is "if flag is set, clear it and bugger off".
> What's worse, AFAICS the callers in aio_poll() are buggered (see below)

Ok. Suggestions?

> > - if (unlikely(apt.error)) {
> > - fput(req->file);
> > + if (unlikely(apt.error))
> > return apt.error;
> > - }
> >
> > if (mask)
> > aio_poll_complete(aiocb, mask);
> Looking at that thing... How does it manage to avoid leaks
> when we try to use it on e.g. /dev/tty, which has
> poll_wait(file, &tty->read_wait, wait);
> poll_wait(file, &tty->write_wait, wait);
> in n_tty_poll()?

I don't think that's he only case that uses multiple poll entries.
It's now the poll() machinery is designed to be used, after all.
Although maybe everybody ends up using just a single wait-queue..

> AFAICS, we'll succeed adding it to the first queue, then have
> aio_poll_queue_proc() fail and set apt.error to -EINVAL.

Yeah, that's bogus, but documented. I guess nobody really expects to
use aio_poll on anything but a socket or something?

But your refcount leak looks valid. Hmm.

So yes, that whole ki_refcnt looks a bit odd and pointless. And
apparently buggy too.

> Comments?

Can we just

(a) remove ki_refcnt entirely

(b) remove the "iocb_put(aiocb);"

from aio_poll()?

Every actual successful io submission should end in aio_complete(), or
we free the aio iocb in the "out_put_req" error case. There's no point
in doing the refcount as you pointed out, and it seems to be actively

Anyway, all of this (and your suggestion to just remove
"aio_poll_complete()" and just use "aio_complete()") is independent of
the file refcounting part, I feel. Which is why I just committed that
patch as-is (84c4e1f89fef "aio: simplify - and fix - fget/fput for


 \ /
  Last update: 2019-03-04 22:29    [W:0.116 / U:4.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site