Re: [PATCH] aio: fix a user triggered use after free (and fix freeze protection of aio writes)

[Christoph Hellwig <hch@lst.de>]

On Sat, Oct 29, 2016 at 05:12:30PM +0100, Al Viro wrote:
> Eww...  IOW, as soon as we'd submitted an async iocb, nobody can so much as
> look at struct file *or* iocb, right?  Or underlying inode, or any fs-private
> data structures attached to it...

Yeah.

> I certainly agree that it's a bug, but IMO you are just papering over it.
> Just look at e.g.
>         written = mapping->a_ops->direct_IO(iocb, &data);
> 
>         /*
>          * Finally, try again to invalidate clean pages which might have been
>          * cached by non-direct readahead, or faulted in by get_user_pages()
>          * if the source of the write was an mmap'ed region of the file
>          * we're writing.  Either one is a pretty crazy thing to do,
>          * so we don't support it 100%.  If this invalidation
>          * fails, tough, the write still worked...
>          */
>         if (mapping->nrpages) {
>                 invalidate_inode_pages2_range(mapping,
>                                               pos >> PAGE_SHIFT, end);
>         }
> in generic_file_direct_write() - who said that mapping doesn't point into
> freed memory by that point?

True, somewhat unlike due to inode caching, but for sure possible
and something that needs to be deal with.

> Why do we play that kind of insane games, anyway?  Why not e.g. refcount
> the (async) iocb and have kiocb_free() drop the reference, with io_submit_one()
> holding one across the call of aio_run_iocb()?  Cacheline bouncing issues?
> Anything more subtle?

There shouldn't really be a need to refcount the iocb itself - nothing
worth looking at.  The one we consider was struct file, and I didn't
like it because of the cacheline bouncing if we decrement if from both
the submitter and completion context.  But it starts to sounds like
the sane version more and more.