[PATCH 0/2] block/iov_iter: fix two issues related with BIO_NO_PAGE_REF

[PATCH 0/2] block/iov_iter: fix two issues related with BIO_NO_PAGE_REF

[Ming Lei]

Hi,

Fix two issues related with BIO_NO_PAGE_REF, both are introduced
recently in for-5.2/block.

Ming Lei (2):
  block: fix handling for BIO_NO_PAGE_REF
  iov_iter: fix iov_iter_type

 fs/block_dev.c      | 3 ++-
 include/linux/uio.h | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.9.5

[PATCH 1/2] block: fix handling for BIO_NO_PAGE_REF

[Ming Lei]

Commit 399254aaf489211 ("block: add BIO_NO_PAGE_REF flag") introduces
BIO_NO_PAGE_REF, and once this flag is set for one bio, all pages
in the bio won't be get/put during IO.

However, if one bio is submitted via __blkdev_direct_IO_simple(),
even though BIO_NO_PAGE_REF is set, pages still may be put.

Fixes this issue by avoiding to put pages if BIO_NO_PAGE_REF is
set.

Fixes: 399254aaf489211 ("block: add BIO_NO_PAGE_REF flag")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 fs/block_dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 24615c76c1d0..bb28e2ead679 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -264,7 +264,8 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
 	bio_for_each_segment_all(bvec, &bio, i, iter_all) {
 		if (should_dirty && !PageCompound(bvec->bv_page))
 			set_page_dirty_lock(bvec->bv_page);
-		put_page(bvec->bv_page);
+		if (!bio_flagged(&bio, BIO_NO_PAGE_REF))
+			put_page(bvec->bv_page);
 	}
 
 	if (unlikely(bio.bi_status))
-- 
2.9.5

Re: [PATCH 1/2] block: fix handling for BIO_NO_PAGE_REF

[Christoph Hellwig]

On Fri, Apr 26, 2019 at 06:45:20PM +0800, Ming Lei wrote:
> Commit 399254aaf489211 ("block: add BIO_NO_PAGE_REF flag") introduces
> BIO_NO_PAGE_REF, and once this flag is set for one bio, all pages
> in the bio won't be get/put during IO.
> 
> However, if one bio is submitted via __blkdev_direct_IO_simple(),
> even though BIO_NO_PAGE_REF is set, pages still may be put.
> 
> Fixes this issue by avoiding to put pages if BIO_NO_PAGE_REF is
> set.
> 
> Fixes: 399254aaf489211 ("block: add BIO_NO_PAGE_REF flag")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Looks good,

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

[PATCH 2/2] iov_iter: fix iov_iter_type

[Ming Lei]

Commit 875f1d0769cd ("iov_iter: add ITER_BVEC_FLAG_NO_REF flag")
introduces one extra flag of ITER_BVEC_FLAG_NO_REF, and this flag
is stored into iter->type.

However, iov_iter_type() doesn't consider the new added flag, fix
it by masking this flag in iov_iter_type().

Fixes: 875f1d0769cd ("iov_iter: add ITER_BVEC_FLAG_NO_REF flag")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/uio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index f184af1999a8..2d0131ad4604 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -60,7 +60,7 @@ struct iov_iter {
 
 static inline enum iter_type iov_iter_type(const struct iov_iter *i)
 {
-	return i->type & ~(READ | WRITE);
+	return i->type & ~(READ | WRITE | ITER_BVEC_FLAG_NO_REF);
 }
 
 static inline bool iter_is_iovec(const struct iov_iter *i)
-- 
2.9.5

Re: [PATCH 2/2] iov_iter: fix iov_iter_type

[Christoph Hellwig]

On Fri, Apr 26, 2019 at 06:45:21PM +0800, Ming Lei wrote:
>  static inline enum iter_type iov_iter_type(const struct iov_iter *i)
>  {
> -	return i->type & ~(READ | WRITE);
> +	return i->type & ~(READ | WRITE | ITER_BVEC_FLAG_NO_REF);

The way we handle i->type is a complete mess.  I think we need
three different values:

   .rw (can be READ or WRITE)
   .type (ITER_IOVEC, ITER_KVEC, ITER_BVEC, ITER_PIPE, ITER_DISCARD)
   .flags (ITER_BVEC_FLAG_NO_REF)

.type seems to be worth an u8 on its own.  And at least as long
as we have space rw and flags should probably be their own u8 aswell,
although we could use accessors to fold them.

Re: [PATCH 2/2] iov_iter: fix iov_iter_type

[Ming Lei]

On Fri, Apr 26, 2019 at 05:44:35AM -0700, Christoph Hellwig wrote:
> On Fri, Apr 26, 2019 at 06:45:21PM +0800, Ming Lei wrote:
> >  static inline enum iter_type iov_iter_type(const struct iov_iter *i)
> >  {
> > -	return i->type & ~(READ | WRITE);
> > +	return i->type & ~(READ | WRITE | ITER_BVEC_FLAG_NO_REF);
> 
> The way we handle i->type is a complete mess.  I think we need
> three different values:
> 
>    .rw (can be READ or WRITE)
>    .type (ITER_IOVEC, ITER_KVEC, ITER_BVEC, ITER_PIPE, ITER_DISCARD)
>    .flags (ITER_BVEC_FLAG_NO_REF)
> 
> .type seems to be worth an u8 on its own.  And at least as long
> as we have space rw and flags should probably be their own u8 aswell,
> although we could use accessors to fold them.

I agree we need a cleanup, which might be a bit bigger change. It should
be clean to convert them into bit fields.

This issue is quite serious, system ram can easily be leaked completely,
so could we fix the issue by the easy way first?

Thanks, 
Ming

Re: [PATCH 2/2] iov_iter: fix iov_iter_type

[Christoph Hellwig]

On Sat, Apr 27, 2019 at 06:13:23AM +0800, Ming Lei wrote:
> I agree we need a cleanup, which might be a bit bigger change. It should
> be clean to convert them into bit fields.
> 
> This issue is quite serious, system ram can easily be leaked completely,
> so could we fix the issue by the easy way first?

I'm fine with this as a quick band-aid for 5.2:

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

But we really need to fix this properly ASAP.

Re: [PATCH 0/2] block/iov_iter: fix two issues related with BIO_NO_PAGE_REF

[Ming Lei]

On Fri, Apr 26, 2019 at 06:45:19PM +0800, Ming Lei wrote:
> Hi,
> 
> Fix two issues related with BIO_NO_PAGE_REF, both are introduced
> recently in for-5.2/block.
> 
> Ming Lei (2):
>   block: fix handling for BIO_NO_PAGE_REF
>   iov_iter: fix iov_iter_type
> 
>  fs/block_dev.c      | 3 ++-
>  include/linux/uio.h | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> -- 
> 2.9.5
> 

Hi Jens,

The two page leak issues are quite serious, given whole system memory
can be consumed up easily in some dio tests.

Could you consider to merge them if you are fine?

Thanks,
Ming

Re: [PATCH 0/2] block/iov_iter: fix two issues related with BIO_NO_PAGE_REF

[Jens Axboe]

On 4/26/19 4:45 AM, Ming Lei wrote:
> Hi,
> 
> Fix two issues related with BIO_NO_PAGE_REF, both are introduced
> recently in for-5.2/block.

Applied, thanks.

-- 
Jens Axboe