| Message ID | 1475225194-3702-3-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri 30-09-16 10:46:34, Christoph Hellwig wrote: > After the call to ->direct_IO the final reference to the file might have > been dropped by aio_complete already, and the call to file_accessed might > cause a use after free. > > Instead update the access time before the I/O, similar to how we > update the time stamps before writes. > > Signed-off-by: Christoph Hellwig <hch@lst.de> The patch looks good. You can add: Reviewed-by: Jan Kara <jack@suse.cz> But frankly it looks like a nasty catch that iocb->ki_filp can go away under you in the AIO case. Do I get it right that this means there must be some other thread closing your fd while the read is running, right? Also it seems that file_end_write(file) call in aio_run_iocb() is prone to the same race? Won't we be better off to just to do additional get_file() / fput() pair in the AIO submission path so that whole AIO submission path is guaranteed to have struct file available? I understand this is very performance sensitive path but we'll be adding just two atomic ops... Honza
On Tue, Oct 04, 2016 at 10:13:24AM +0200, Jan Kara wrote: > But frankly it looks like a nasty catch that iocb->ki_filp can go away > under you in the AIO case. Do I get it right that this means there must be > some other thread closing your fd while the read is running, right? Yes, that's what generic/323 tests. > Also it seems that file_end_write(file) call in aio_run_iocb() is prone to > the same race? Indeed. That's easy to fix by moving the file_end_write to aio_complete, though. > Won't we be better off to just to do additional get_file() / fput() pair in > the AIO submission path so that whole AIO submission path is guaranteed to > have struct file available? I understand this is very performance sensitive > path but we'll be adding just two atomic ops... I'd rather avoid those if we can. But a big comment and some refactoring in this area would be useful to make that easier to understand. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/mm/filemap.c b/mm/filemap.c index 8a287df..2f1175e 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1910,16 +1910,18 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter) if (iocb->ki_flags & IOCB_DIRECT) { struct address_space *mapping = file->f_mapping; struct inode *inode = mapping->host; + struct iov_iter data = *iter; loff_t size; size = i_size_read(inode); retval = filemap_write_and_wait_range(mapping, iocb->ki_pos, iocb->ki_pos + count - 1); - if (!retval) { - struct iov_iter data = *iter; - retval = mapping->a_ops->direct_IO(iocb, &data); - } + if (retval < 0) + goto out; + file_accessed(file); + + retval = mapping->a_ops->direct_IO(iocb, &data); if (retval > 0) { iocb->ki_pos += retval; iov_iter_advance(iter, retval); @@ -1935,10 +1937,8 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter) * DAX files, so don't bother trying. */ if (retval < 0 || !iov_iter_count(iter) || iocb->ki_pos >= size || - IS_DAX(inode)) { - file_accessed(file); + IS_DAX(inode)) goto out; - } } retval = do_generic_file_read(file, &iocb->ki_pos, iter, retval);
After the call to ->direct_IO the final reference to the file might have been dropped by aio_complete already, and the call to file_accessed might cause a use after free. Instead update the access time before the I/O, similar to how we update the time stamps before writes. Signed-off-by: Christoph Hellwig <hch@lst.de> --- mm/filemap.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)