linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Dumazet <eric.dumazet@gmail.com>,
	David Miller <davem@davemloft.net>,
	Jason Baron <jbaron@akamai.com>,
	kgraul@linux.ibm.com, ktkhai@virtuozzo.com,
	kyeongdon.kim@lge.com,
	Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	Netdev <netdev@vger.kernel.org>,
	pabeni@redhat.com, syzkaller-bugs@googlegroups.com,
	xiyou.wangcong@gmail.com, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll)
Date: Mon, 4 Mar 2019 13:22:44 -0800	[thread overview]
Message-ID: <CAHk-=wghRmdYKRAwakeHmjcpbLt9fJAHyU2x8s_NZONhz6RTOw@mail.gmail.com> (raw)
In-Reply-To: <20190304023618.GS2217@ZenIV.linux.org.uk>

On Sun, Mar 3, 2019 at 6:36 PM Al Viro <viro@zeniv.linux.org.uk> 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
buggy.

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
io_submit()").

                   Linus

  reply	other threads:[~2019-03-04 21:29 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-03 10:22 KASAN: use-after-free Read in unix_dgram_poll syzbot
2019-03-03 13:55 ` Al Viro
2019-03-03 15:18   ` [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll) Al Viro
2019-03-03 18:37     ` Eric Dumazet
2019-03-03 19:44     ` Linus Torvalds
2019-03-03 20:13       ` Linus Torvalds
2019-03-03 20:30       ` Al Viro
2019-03-03 22:23         ` Linus Torvalds
2019-03-04  2:36           ` Al Viro
2019-03-04 21:22             ` Linus Torvalds [this message]
2019-03-07  0:03               ` [PATCH 1/8] aio: make sure file is pinned Al Viro
2019-03-07  0:03                 ` [PATCH 2/8] aio_poll_wake(): don't set ->woken if we ignore the wakeup Al Viro
2019-03-07  2:18                   ` Al Viro
2019-03-08 11:16                     ` zhengbin (A)
2019-03-07  0:03                 ` [PATCH 3/8] aio_poll(): sanitize the logics after vfs_poll(), get rid of leak on error Al Viro
2019-03-07  2:11                   ` zhengbin (A)
2019-03-07  0:03                 ` [PATCH 4/8] aio_poll(): get rid of weird refcounting Al Viro
2019-03-07  0:03                 ` [PATCH 5/8] make aio_read()/aio_write() return int Al Viro
2019-03-07  0:03                 ` [PATCH 6/8] move dropping ->ki_eventfd into iocb_put() Al Viro
2019-03-07  0:03                 ` [PATCH 7/8] deal with get_reqs_available() in aio_get_req() itself Al Viro
2019-03-07  0:03                 ` [PATCH 8/8] aio: move sanity checks and request allocation to io_submit_one() Al Viro
2019-03-07  0:23                 ` [PATCH 1/8] aio: make sure file is pinned Linus Torvalds
2019-03-07  0:41                   ` Al Viro
2019-03-07  0:48                     ` Al Viro
2019-03-07  1:20                       ` Al Viro
2019-03-07  1:30                         ` Linus Torvalds
2019-03-08  3:36                           ` Al Viro
2019-03-08 15:50                             ` Christoph Hellwig
2019-03-10  7:06                             ` Al Viro
2019-03-10  7:08                               ` [PATCH 1/8] pin iocb through aio Al Viro
2019-03-10  7:08                                 ` [PATCH 2/8] keep io_event in aio_kiocb Al Viro
2019-03-11 19:43                                   ` Christoph Hellwig
2019-03-11 21:17                                     ` Al Viro
2019-03-10  7:08                                 ` [PATCH 3/8] aio: store event at final iocb_put() Al Viro
2019-03-11 19:44                                   ` Christoph Hellwig
2019-03-11 21:13                                     ` Al Viro
2019-03-11 22:52                                       ` Al Viro
2019-03-10  7:08                                 ` [PATCH 4/8] Fix aio_poll() races Al Viro
2019-03-11 19:58                                   ` Christoph Hellwig
2019-03-11 21:06                                     ` Al Viro
2019-03-12 19:18                                       ` Christoph Hellwig
2019-03-10  7:08                                 ` [PATCH 5/8] make aio_read()/aio_write() return int Al Viro
2019-03-11 19:44                                   ` Christoph Hellwig
2019-03-10  7:08                                 ` [PATCH 6/8] move dropping ->ki_eventfd into iocb_destroy() Al Viro
2019-03-11 19:46                                   ` Christoph Hellwig
2019-03-10  7:08                                 ` [PATCH 7/8] deal with get_reqs_available() in aio_get_req() itself Al Viro
2019-03-11 19:46                                   ` Christoph Hellwig
2019-03-10  7:08                                 ` [PATCH 8/8] aio: move sanity checks and request allocation to io_submit_one() Al Viro
2019-03-11 19:48                                   ` Christoph Hellwig
2019-03-11 21:12                                     ` Al Viro
2019-03-11 19:41                                 ` [PATCH 1/8] pin iocb through aio Christoph Hellwig
2019-03-11 19:41                               ` [PATCH 1/8] aio: make sure file is pinned Christoph Hellwig
2019-03-04  7:53     ` [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll) Dmitry Vyukov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wghRmdYKRAwakeHmjcpbLt9fJAHyU2x8s_NZONhz6RTOw@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=davem@davemloft.net \
    --cc=eric.dumazet@gmail.com \
    --cc=hch@lst.de \
    --cc=jbaron@akamai.com \
    --cc=kgraul@linux.ibm.com \
    --cc=ktkhai@virtuozzo.com \
    --cc=kyeongdon.kim@lge.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=syzkaller-bugs@googlegroups.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=xiyou.wangcong@gmail.com \
    --subject='Re: [PATCH] aio: prevent the final fput() in the middle of vfs_poll() (Re: KASAN: use-after-free Read in unix_dgram_poll)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).