[PATCH 1/2] iomap: Fix pipe page leakage during splicing

[PATCH 1/2] iomap: Fix pipe page leakage during splicing

[Jan Kara]

When splicing using iomap_dio_rw() to a pipe, we may leak pipe pages
because bio_iov_iter_get_pages() records that the pipe will have full
extent worth of data however if file size is not block size aligned
iomap_dio_rw() returns less than what bio_iov_iter_get_pages() set up
and splice code gets confused leaking a pipe page with the file tail.

Handle the situation similarly to the old direct IO implementation and
revert iter to actually returned read amount which makes iter consistent
with value returned from iomap_dio_rw() and thus the splice code is
happy.

Fixes: ff6a9292e6f6 ("iomap: implement direct I/O")
CC: stable@vger.kernel.org
Reported-by: syzbot+991400e8eba7e00a26e1@syzkaller.appspotmail.com
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/iomap/direct-io.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 1fc28c2da279..30189652c560 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -497,8 +497,15 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		}
 		pos += ret;
 
-		if (iov_iter_rw(iter) == READ && pos >= dio->i_size)
+		if (iov_iter_rw(iter) == READ && pos >= dio->i_size) {
+			/*
+			 * We will report we've read data only upto i_size.
+			 * Revert iter to a state corresponding to that as
+			 * some callers (such as splice code) rely on it.
+			 */
+			iov_iter_revert(iter, pos - dio->i_size);
 			break;
+		}
 	} while ((count = iov_iter_count(iter)) > 0);
 	blk_finish_plug(&plug);
 
-- 
2.16.4

Re: [PATCH 1/2] iomap: Fix pipe page leakage during splicing

[Darrick J. Wong]

On Thu, Nov 21, 2019 at 05:15:34PM +0100, Jan Kara wrote:
> When splicing using iomap_dio_rw() to a pipe, we may leak pipe pages
> because bio_iov_iter_get_pages() records that the pipe will have full
> extent worth of data however if file size is not block size aligned
> iomap_dio_rw() returns less than what bio_iov_iter_get_pages() set up
> and splice code gets confused leaking a pipe page with the file tail.
> 
> Handle the situation similarly to the old direct IO implementation and
> revert iter to actually returned read amount which makes iter consistent
> with value returned from iomap_dio_rw() and thus the splice code is
> happy.
> 
> Fixes: ff6a9292e6f6 ("iomap: implement direct I/O")
> CC: stable@vger.kernel.org
> Reported-by: syzbot+991400e8eba7e00a26e1@syzkaller.appspotmail.com
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/iomap/direct-io.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 1fc28c2da279..30189652c560 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -497,8 +497,15 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  		}
>  		pos += ret;
>  
> -		if (iov_iter_rw(iter) == READ && pos >= dio->i_size)
> +		if (iov_iter_rw(iter) == READ && pos >= dio->i_size) {
> +			/*
> +			 * We will report we've read data only upto i_size.

Nit: "up to"; will fix that on the way in.

> +			 * Revert iter to a state corresponding to that as
> +			 * some callers (such as splice code) rely on it.
> +			 */
> +			iov_iter_revert(iter, pos - dio->i_size);

Just to make sure I'm getting this right, iov_iter_revert walks the
iterator variables backwards through pipe buffers/bvec/iovec, which has
the effect of undoing whatever iterator walking we've just done.

In contrast, iov_iter_reexpand undoes a previous subtraction to
iov->count which was (presumably) done via iov_iter_truncate.

Or to put it another way, _revert walks the iteration pointer backwards,
whereas _truncate/_reexpand modify where the iteration ends.  Right?

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  			break;
> +		}
>  	} while ((count = iov_iter_count(iter)) > 0);
>  	blk_finish_plug(&plug);
>  
> -- 
> 2.16.4
> 

Re: [PATCH 1/2] iomap: Fix pipe page leakage during splicing

[Matthew Bobrowski]

On Thu, Nov 21, 2019 at 03:55:28PM -0800, Darrick J. Wong wrote:
> On Thu, Nov 21, 2019 at 05:15:34PM +0100, Jan Kara wrote:
> > @@ -497,8 +497,15 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
> >  		}
> >  		pos += ret;
> >  
> > -		if (iov_iter_rw(iter) == READ && pos >= dio->i_size)
> > +		if (iov_iter_rw(iter) == READ && pos >= dio->i_size) {
> > +			/*
> > +			 * We will report we've read data only upto i_size.
> 
> Nit: "up to"; will fix that on the way in.

A nit of a nit: "We will report that we've read..."; I think it reads
better, so might as well update it if you're already fixing the other
nit up as you're pulling this in. :P

/M

Re: [PATCH 1/2] iomap: Fix pipe page leakage during splicing

[Jan Kara]

On Thu 21-11-19 15:55:28, Darrick J. Wong wrote:
> On Thu, Nov 21, 2019 at 05:15:34PM +0100, Jan Kara wrote:
> > When splicing using iomap_dio_rw() to a pipe, we may leak pipe pages
> > because bio_iov_iter_get_pages() records that the pipe will have full
> > extent worth of data however if file size is not block size aligned
> > iomap_dio_rw() returns less than what bio_iov_iter_get_pages() set up
> > and splice code gets confused leaking a pipe page with the file tail.
> > 
> > Handle the situation similarly to the old direct IO implementation and
> > revert iter to actually returned read amount which makes iter consistent
> > with value returned from iomap_dio_rw() and thus the splice code is
> > happy.
> > 
> > Fixes: ff6a9292e6f6 ("iomap: implement direct I/O")
> > CC: stable@vger.kernel.org
> > Reported-by: syzbot+991400e8eba7e00a26e1@syzkaller.appspotmail.com
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/iomap/direct-io.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > index 1fc28c2da279..30189652c560 100644
> > --- a/fs/iomap/direct-io.c
> > +++ b/fs/iomap/direct-io.c
> > @@ -497,8 +497,15 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
> >  		}
> >  		pos += ret;
> >  
> > -		if (iov_iter_rw(iter) == READ && pos >= dio->i_size)
> > +		if (iov_iter_rw(iter) == READ && pos >= dio->i_size) {
> > +			/*
> > +			 * We will report we've read data only upto i_size.
> 
> Nit: "up to"; will fix that on the way in.
> 
> > +			 * Revert iter to a state corresponding to that as
> > +			 * some callers (such as splice code) rely on it.
> > +			 */
> > +			iov_iter_revert(iter, pos - dio->i_size);
> 
> Just to make sure I'm getting this right, iov_iter_revert walks the
> iterator variables backwards through pipe buffers/bvec/iovec, which has
> the effect of undoing whatever iterator walking we've just done.
> 
> In contrast, iov_iter_reexpand undoes a previous subtraction to
> iov->count which was (presumably) done via iov_iter_truncate.
> 
> Or to put it another way, _revert walks the iteration pointer backwards,
> whereas _truncate/_reexpand modify where the iteration ends.  Right?

Correct.

> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] iomap: Fix pipe page leakage during splicing

[Christoph Hellwig]

Looks good modulo the spelling critique:

Reviewed-by: Christoph Hellwig <hch@lst.de>