Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <000000000000f39c7b05832e0219@google.com> <20190303135502.GP2217@ZenIV.linux.org.uk>
 <20190303151846.GQ2217@ZenIV.linux.org.uk> <CAHk-=wiQ2YNB+2FT+jbdcC6qfYqvS2kLLAZbmPE_7sLfUPwFJg@mail.gmail.com>
 <20190303203011.GR2217@ZenIV.linux.org.uk> <CAHk-=widuz9QBY-Xhpc1YHf=EV1uRGyOv-v-BNSa1zW-nCCC6Q@mail.gmail.com>
 <20190304023618.GS2217@ZenIV.linux.org.uk>
In-Reply-To: <20190304023618.GS2217@ZenIV.linux.org.uk>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon, 4 Mar 2019 13:22:44 -0800
Message-ID: <CAHk-=wghRmdYKRAwakeHmjcpbLt9fJAHyU2x8s_NZONhz6RTOw@mail.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)
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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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