On Wed 02-05-18 12:45:40, Dave Chinner wrote:
> On Sat, Apr 21, 2018 at 03:03:09PM +0200, Jan Kara wrote:
> > On Wed 18-04-18 14:08:26, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > >
> > > Currently iomap_dio_rw() only handles (data)sync write completions
> > > for AIO. This means we can't optimised non-AIO IO to minimise device
> > > flushes as we can't tell the caller whether a flush is required or
> > > not.
> > >
> > > To solve this problem and enable further optimisations, make
> > > iomap_dio_rw responsible for data sync behaviour for all IO, not
> > > just AIO.
> > >
> > > In doing so, the sync operation is now accounted as part of the DIO
> > > IO by inode_dio_end(), hence post-IO data stability updates will no
> > > long race against operations that serialise via inode_dio_wait()
> > > such as truncate or hole punch.
> > >
> > > Signed-Off-By: Dave Chinner <dchinner@xxxxxxxxxx>
> > > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> >
> > Looks good to me. You can add:
> >
> > Reviewed-by: Jan Kara <jack@xxxxxxx>
>
> It looks good, but it's broken in a subtle, nasty way. :/
>
> > > @@ -768,14 +776,8 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
> > > static void iomap_dio_complete_work(struct work_struct *work)
> > > {
> > > struct iomap_dio *dio = container_of(work, struct iomap_dio, aio.work);
> > > - struct kiocb *iocb = dio->iocb;
> > > - bool is_write = (dio->flags & IOMAP_DIO_WRITE);
> > > - ssize_t ret;
> > >
> > > - ret = iomap_dio_complete(dio);
> > > - if (is_write && ret > 0)
> > > - ret = generic_write_sync(iocb, ret);
> > > - iocb->ki_complete(iocb, ret, 0);
> > > + dio->iocb->ki_complete(dio->iocb, iomap_dio_complete(dio), 0);
>
> This generates a use after free from KASAN from generic/016. it
> appears the compiler orders the code so that it dereferences
> dio->iocb after iomap_dio_complete() has freed the dio structure
> (yay!).
Yeah, very subtle but the compiler is indeed free to do this (in C the
sequence point is only the function call but the order of evaluation of
function arguments is unspecified). Thanks for catching this.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
