[PATCH 0/2] block: fix discard merge for single max discard segment

[PATCH 0/2] block: fix discard merge for single max discard segment

[Ming Lei]

Hello,

The 1st patch checks max discard segment limit in request merge code,
and one discard request failure issue is fixed for virtio_blk.

The 2nd patch fixes handling of single max discard segment for
virtio_blk, and potential memory corruption is fixed.


Ming Lei (2):
  block: respect queue limit of max discard segment
  block: virtio_blk: fix handling single range discard request

 block/blk-merge.c          | 10 ++++++++--
 drivers/block/virtio_blk.c | 23 +++++++++++++++--------
 2 files changed, 23 insertions(+), 10 deletions(-)

Cc: Christoph Hellwig <hch@lst.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Changpeng Liu <changpeng.liu@intel.com>
Cc: Daniel Verkamp <dverkamp@chromium.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>

-- 
2.25.2

[PATCH 1/2] block: respect queue limit of max discard segment

[Ming Lei]

When queue_max_discard_segments(q) is 1, blk_discard_mergable() will
return false for discard request, then normal request merge is applied.
However, only queue_max_segments() is checked, so max discard segment
limit isn't respected.

Check max discard segment limit in the request merge code for fixing
the issue.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-merge.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 5196dc145270..d18fb88ca8bd 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -533,10 +533,16 @@ int __blk_rq_map_sg(struct request_queue *q, struct request *rq,
 }
 EXPORT_SYMBOL(__blk_rq_map_sg);
 
+static inline unsigned int blk_rq_get_max_segments(struct request *rq)
+{
+	return req_op(rq) == REQ_OP_DISCARD ?
+		queue_max_discard_segments(rq->q) : queue_max_segments(rq->q);
+}
+
 static inline int ll_new_hw_segment(struct request *req, struct bio *bio,
 		unsigned int nr_phys_segs)
 {
-	if (req->nr_phys_segments + nr_phys_segs > queue_max_segments(req->q))
+	if (req->nr_phys_segments + nr_phys_segs > blk_rq_get_max_segments(req))
 		goto no_merge;
 
 	if (blk_integrity_merge_bio(req->q, req, bio) == false)
@@ -624,7 +630,7 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 		return 0;
 
 	total_phys_segments = req->nr_phys_segments + next->nr_phys_segments;
-	if (total_phys_segments > queue_max_segments(q))
+	if (total_phys_segments > blk_rq_get_max_segments(req))
 		return 0;
 
 	if (blk_integrity_merge_rq(q, req, next) == false)
-- 
2.25.2

[PATCH 2/2] block: virtio_blk: fix handling single range discard request

[Ming Lei]

1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") starts
to support multi-range discard for virtio-blk. However, the virtio-blk
disk may report max discard segment as 1, at least that is exactly what
qemu is doing.

So far, block layer switches to normal request merge if max discard segment
limit is 1, and multiple bios can be merged to single segment. This way may
cause memory corruption in virtblk_setup_discard_write_zeroes().

Fix the issue by handling single max discard segment in straightforward
way.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Changpeng Liu <changpeng.liu@intel.com>
Cc: Daniel Verkamp <dverkamp@chromium.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/block/virtio_blk.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 63b213e00b37..05b01903122b 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -126,14 +126,21 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
 	if (!range)
 		return -ENOMEM;
 
-	__rq_for_each_bio(bio, req) {
-		u64 sector = bio->bi_iter.bi_sector;
-		u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
-
-		range[n].flags = cpu_to_le32(flags);
-		range[n].num_sectors = cpu_to_le32(num_sectors);
-		range[n].sector = cpu_to_le64(sector);
-		n++;
+	if (queue_max_discard_segments(req->q) == 1) {
+		range[0].flags = cpu_to_le32(flags);
+		range[0].num_sectors = cpu_to_le32(blk_rq_sectors(req));
+		range[0].sector = cpu_to_le64(blk_rq_pos(req));
+		n = 1;
+	} else {
+		__rq_for_each_bio(bio, req) {
+			u64 sector = bio->bi_iter.bi_sector;
+			u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
+
+			range[n].flags = cpu_to_le32(flags);
+			range[n].num_sectors = cpu_to_le32(num_sectors);
+			range[n].sector = cpu_to_le64(sector);
+			n++;
+		}
 	}
 
 	req->special_vec.bv_page = virt_to_page(range);
-- 
2.25.2

Re: [PATCH 2/2] block: virtio_blk: fix handling single range discard request

[Stefano Garzarella]

Hi Ming,

On Tue, Aug 11, 2020 at 05:21:34PM +0800, Ming Lei wrote:
> 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") starts
> to support multi-range discard for virtio-blk. However, the virtio-blk
> disk may report max discard segment as 1, at least that is exactly what
> qemu is doing.
> 
> So far, block layer switches to normal request merge if max discard segment
> limit is 1, and multiple bios can be merged to single segment. This way may
> cause memory corruption in virtblk_setup_discard_write_zeroes().
> 
> Fix the issue by handling single max discard segment in straightforward
> way.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Changpeng Liu <changpeng.liu@intel.com>
> Cc: Daniel Verkamp <dverkamp@chromium.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  drivers/block/virtio_blk.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 63b213e00b37..05b01903122b 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c'
> @@ -126,14 +126,21 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
>  	if (!range)
>  		return -ENOMEM;

We are allocating the 'range' array to contain 'segments' elements.
When queue_max_discard_segments() returns 1, should we limit 'segments'
to 1?

>  
> -	__rq_for_each_bio(bio, req) {
> -		u64 sector = bio->bi_iter.bi_sector;
> -		u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> -
> -		range[n].flags = cpu_to_le32(flags);
> -		range[n].num_sectors = cpu_to_le32(num_sectors);
> -		range[n].sector = cpu_to_le64(sector);
> -		n++;
> +	if (queue_max_discard_segments(req->q) == 1) {
> +		range[0].flags = cpu_to_le32(flags);
> +		range[0].num_sectors = cpu_to_le32(blk_rq_sectors(req));
> +		range[0].sector = cpu_to_le64(blk_rq_pos(req));
> +		n = 1;
                ^
                this doesn't seem necessary since we don't use 'n' afterwards.

Thanks,
Stefano

> +	} else {
> +		__rq_for_each_bio(bio, req) {
> +			u64 sector = bio->bi_iter.bi_sector;
> +			u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> +
> +			range[n].flags = cpu_to_le32(flags);
> +			range[n].num_sectors = cpu_to_le32(num_sectors);
> +			range[n].sector = cpu_to_le64(sector);
> +			n++;
> +		}
>  	}
>  
>  	req->special_vec.bv_page = virt_to_page(range);
> -- 
> 2.25.2
> 

Re: [PATCH 2/2] block: virtio_blk: fix handling single range discard request

[Ming Lei]

On Tue, Aug 11, 2020 at 02:30:44PM +0200, Stefano Garzarella wrote:
> Hi Ming,
> 
> On Tue, Aug 11, 2020 at 05:21:34PM +0800, Ming Lei wrote:
> > 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") starts
> > to support multi-range discard for virtio-blk. However, the virtio-blk
> > disk may report max discard segment as 1, at least that is exactly what
> > qemu is doing.
> > 
> > So far, block layer switches to normal request merge if max discard segment
> > limit is 1, and multiple bios can be merged to single segment. This way may
> > cause memory corruption in virtblk_setup_discard_write_zeroes().
> > 
> > Fix the issue by handling single max discard segment in straightforward
> > way.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Changpeng Liu <changpeng.liu@intel.com>
> > Cc: Daniel Verkamp <dverkamp@chromium.org>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  drivers/block/virtio_blk.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 63b213e00b37..05b01903122b 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c'
> > @@ -126,14 +126,21 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
> >  	if (!range)
> >  		return -ENOMEM;
> 
> We are allocating the 'range' array to contain 'segments' elements.
> When queue_max_discard_segments() returns 1, should we limit 'segments'
> to 1?

That is block layer's responsibility to make sure that 'segments' is <=
1, and we can double check & warn here.

> 
> >  
> > -	__rq_for_each_bio(bio, req) {
> > -		u64 sector = bio->bi_iter.bi_sector;
> > -		u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> > -
> > -		range[n].flags = cpu_to_le32(flags);
> > -		range[n].num_sectors = cpu_to_le32(num_sectors);
> > -		range[n].sector = cpu_to_le64(sector);
> > -		n++;
> > +	if (queue_max_discard_segments(req->q) == 1) {
> > +		range[0].flags = cpu_to_le32(flags);
> > +		range[0].num_sectors = cpu_to_le32(blk_rq_sectors(req));
> > +		range[0].sector = cpu_to_le64(blk_rq_pos(req));
> > +		n = 1;
>                 ^
>                 this doesn't seem necessary since we don't use 'n' afterwards.

OK.

Thanks,
Ming

Re: [PATCH 2/2] block: virtio_blk: fix handling single range discard request

[Stefano Garzarella]

On Tue, Aug 11, 2020 at 09:09:53PM +0800, Ming Lei wrote:
> On Tue, Aug 11, 2020 at 02:30:44PM +0200, Stefano Garzarella wrote:
> > Hi Ming,
> > 
> > On Tue, Aug 11, 2020 at 05:21:34PM +0800, Ming Lei wrote:
> > > 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") starts
> > > to support multi-range discard for virtio-blk. However, the virtio-blk
> > > disk may report max discard segment as 1, at least that is exactly what
> > > qemu is doing.
> > > 
> > > So far, block layer switches to normal request merge if max discard segment
> > > limit is 1, and multiple bios can be merged to single segment. This way may
> > > cause memory corruption in virtblk_setup_discard_write_zeroes().
> > > 
> > > Fix the issue by handling single max discard segment in straightforward
> > > way.
> > > 
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Cc: Changpeng Liu <changpeng.liu@intel.com>
> > > Cc: Daniel Verkamp <dverkamp@chromium.org>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  drivers/block/virtio_blk.c | 23 +++++++++++++++--------
> > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > index 63b213e00b37..05b01903122b 100644
> > > --- a/drivers/block/virtio_blk.c
> > > +++ b/drivers/block/virtio_blk.c'
> > > @@ -126,14 +126,21 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
> > >  	if (!range)
> > >  		return -ENOMEM;
> > 
> > We are allocating the 'range' array to contain 'segments' elements.
> > When queue_max_discard_segments() returns 1, should we limit 'segments'
> > to 1?
> 
> That is block layer's responsibility to make sure that 'segments' is <=
> 1, and we can double check & warn here.

So, IIUC, the number of bio in a request may not be the same as
the return value of blk_rq_nr_discard_segments(). Is it right?

If it is true, is not clear to me why we are allocating the 'range'
array using 'segments' as the size, and then we are filling it cycling
on the bios.

Maybe I need to take a closer look at the block layer to understand it better.

Thanks,
Stefano

> 
> > 
> > >  
> > > -	__rq_for_each_bio(bio, req) {
> > > -		u64 sector = bio->bi_iter.bi_sector;
> > > -		u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> > > -
> > > -		range[n].flags = cpu_to_le32(flags);
> > > -		range[n].num_sectors = cpu_to_le32(num_sectors);
> > > -		range[n].sector = cpu_to_le64(sector);
> > > -		n++;
> > > +	if (queue_max_discard_segments(req->q) == 1) {
> > > +		range[0].flags = cpu_to_le32(flags);
> > > +		range[0].num_sectors = cpu_to_le32(blk_rq_sectors(req));
> > > +		range[0].sector = cpu_to_le64(blk_rq_pos(req));
> > > +		n = 1;
> >                 ^
> >                 this doesn't seem necessary since we don't use 'n' afterwards.
> 
> OK.
> 
> Thanks,
> Ming
> 

Re: [PATCH 2/2] block: virtio_blk: fix handling single range discard request

[Ming Lei]

On Tue, Aug 11, 2020 at 03:39:25PM +0200, Stefano Garzarella wrote:
> On Tue, Aug 11, 2020 at 09:09:53PM +0800, Ming Lei wrote:
> > On Tue, Aug 11, 2020 at 02:30:44PM +0200, Stefano Garzarella wrote:
> > > Hi Ming,
> > > 
> > > On Tue, Aug 11, 2020 at 05:21:34PM +0800, Ming Lei wrote:
> > > > 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") starts
> > > > to support multi-range discard for virtio-blk. However, the virtio-blk
> > > > disk may report max discard segment as 1, at least that is exactly what
> > > > qemu is doing.
> > > > 
> > > > So far, block layer switches to normal request merge if max discard segment
> > > > limit is 1, and multiple bios can be merged to single segment. This way may
> > > > cause memory corruption in virtblk_setup_discard_write_zeroes().
> > > > 
> > > > Fix the issue by handling single max discard segment in straightforward
> > > > way.
> > > > 
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Changpeng Liu <changpeng.liu@intel.com>
> > > > Cc: Daniel Verkamp <dverkamp@chromium.org>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > >  drivers/block/virtio_blk.c | 23 +++++++++++++++--------
> > > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > > index 63b213e00b37..05b01903122b 100644
> > > > --- a/drivers/block/virtio_blk.c
> > > > +++ b/drivers/block/virtio_blk.c'
> > > > @@ -126,14 +126,21 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
> > > >  	if (!range)
> > > >  		return -ENOMEM;
> > > 
> > > We are allocating the 'range' array to contain 'segments' elements.
> > > When queue_max_discard_segments() returns 1, should we limit 'segments'
> > > to 1?
> > 
> > That is block layer's responsibility to make sure that 'segments' is <=
> > 1, and we can double check & warn here.
> 
> So, IIUC, the number of bio in a request may not be the same as
> the return value of blk_rq_nr_discard_segments(). Is it right?

In case that queue_max_discard_segments() is 1, it is right. If
queue_max_discard_segments() is > 1, nr_range is supposed to be
same with number of bios in a request.


Thanks, 
Ming

Re: [PATCH 2/2] block: virtio_blk: fix handling single range discard request

[Stefano Garzarella]

On Tue, Aug 11, 2020 at 09:43:26PM +0800, Ming Lei wrote:
> On Tue, Aug 11, 2020 at 03:39:25PM +0200, Stefano Garzarella wrote:
> > On Tue, Aug 11, 2020 at 09:09:53PM +0800, Ming Lei wrote:
> > > On Tue, Aug 11, 2020 at 02:30:44PM +0200, Stefano Garzarella wrote:
> > > > Hi Ming,
> > > > 
> > > > On Tue, Aug 11, 2020 at 05:21:34PM +0800, Ming Lei wrote:
> > > > > 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") starts
> > > > > to support multi-range discard for virtio-blk. However, the virtio-blk
> > > > > disk may report max discard segment as 1, at least that is exactly what
> > > > > qemu is doing.
> > > > > 
> > > > > So far, block layer switches to normal request merge if max discard segment
> > > > > limit is 1, and multiple bios can be merged to single segment. This way may
> > > > > cause memory corruption in virtblk_setup_discard_write_zeroes().
> > > > > 
> > > > > Fix the issue by handling single max discard segment in straightforward
> > > > > way.
> > > > > 
> > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > > Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
> > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > Cc: Changpeng Liu <changpeng.liu@intel.com>
> > > > > Cc: Daniel Verkamp <dverkamp@chromium.org>
> > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > ---
> > > > >  drivers/block/virtio_blk.c | 23 +++++++++++++++--------
> > > > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > > > index 63b213e00b37..05b01903122b 100644
> > > > > --- a/drivers/block/virtio_blk.c
> > > > > +++ b/drivers/block/virtio_blk.c'
> > > > > @@ -126,14 +126,21 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
> > > > >  	if (!range)
> > > > >  		return -ENOMEM;
> > > > 
> > > > We are allocating the 'range' array to contain 'segments' elements.
> > > > When queue_max_discard_segments() returns 1, should we limit 'segments'
> > > > to 1?
> > > 
> > > That is block layer's responsibility to make sure that 'segments' is <=
> > > 1, and we can double check & warn here.
> > 
> > So, IIUC, the number of bio in a request may not be the same as
> > the return value of blk_rq_nr_discard_segments(). Is it right?
> 
> In case that queue_max_discard_segments() is 1, it is right. If
> queue_max_discard_segments() is > 1, nr_range is supposed to be
> same with number of bios in a request.

Got it. Thanks for clarify.

In the meantime I took a look at nvme_setup_discard() and there is
WARN_ON_ONCE(n != segments), maybe we should do the same.

Thanks,
Stefano