aio completions vs file_accessed race, was: Re: [PATCH 7/8] xfs: split direct I/O and DAX path

[Christoph Hellwig]

On Wed, Sep 28, 2016 at 07:53:52PM -0700, Darrick J. Wong wrote:
> So I noticed that generic/323 starts crashing in file_accessed -> touch_atime
> because iocb->ki_filp->f_path.dentry == NULL.  For a while I thought it was
> some weird reflink bug, but I finally had time to go build a vanilla 4.8-rc8
> kernel and that blew up here too.  I'm not sure why this line got inserted
> here, since it wasn't there prior to this patch, AFAICT.

This line was there before near the end of xfs_file_dio_aio_read already,
e.g. line 376 just before the above commit, but it only got introduced
a bit earlier in "xfs: stop using generic_file_read_iter for direct I/O",
which copied it over from generic_file_read_iter.  І think any new
issues in these commits could just be a minor timing change, as
we're not changing struct file refcounting in any way here.

generic/323 reproduces the last struct file reference being dropped
by aio completions, so it seems like we have an issue here, which
I suspect is something in the common code.  I can't reproduce it
locally, but looking at the aio_complete -> kiocb_free callchain
and the lack of other struct file refcounting in aio.c it seems
inherently unsafe to reference struct file once the completion
may have run, that is after (__)blkdev_direct_IO returned.

I'll see if I can come up with a solution for that, most likely
that would involve moving the file_accessed call into __blkdev_direct_IO
before we drop the final reference on the dio structure.