fix virt_boundary_mask handling in SCSI

fix virt_boundary_mask handling in SCSI

[Christoph Hellwig]

Hi all,

this series fixes a corruption when drivers using virt_boundary_mask set
a limited max_segment_size by accident, which Red Hat reported as causing
data corruption with storvsc.  I did audit the tree and also found that
this can affect SRP and iSER as well.

Note that I've dropped the Tested-by from Laurence because the patch
changed very slightly from the last version.

Diffstat:
 infiniband/ulp/srp/ib_srp.c |    5 +++--
 scsi/hosts.c                |   20 +++++++++++++-------
 2 files changed, 16 insertions(+), 9 deletions(-)

[PATCH 1/2] RDMA/srp: don't set a max_segment_size when virt_boundary_mask is set

[Christoph Hellwig]

virt_boundary_mask implies an unlimited max_segment_size.  Setting both
can lead to data corruption, and we're going to check for this in the
SCSI midlayer soon.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 1378651735f6..23ed2fc688f0 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3705,9 +3705,10 @@ static ssize_t add_target_store(struct device *dev,
 	target_host->max_id      = 1;
 	target_host->max_lun     = -1LL;
 	target_host->max_cmd_len = sizeof ((struct srp_cmd *) (void *) 0L)->cdb;
-	target_host->max_segment_size = ib_dma_max_seg_size(ibdev);
 
-	if (!(ibdev->attrs.kernel_cap_flags & IBK_SG_GAPS_REG))
+	if (ibdev->attrs.kernel_cap_flags & IBK_SG_GAPS_REG)
+		target_host->max_segment_size = ib_dma_max_seg_size(ibdev);
+	else
 		target_host->virt_boundary_mask = ~srp_dev->mr_page_mask;
 
 	target = host_to_target(target_host);
-- 
2.47.2

[PATCH 2/2] scsi: enforce unlimited max_segment_size when virt_boundary_mask is set

[Christoph Hellwig]

The virt_boundary_mask limit requires an unlimited max_segment_size for
bio splitting to not corrupt data.  Historically, the block layer tried
to validate this, although the check was half-hearted until the addition
of the atomic queue limits API.  The full blown check than triggered
issues with stacked devices incorrectly inheriting limits such as the
virt boundary and got disabled in commit b561ea56a264 ("block: allow
device to have both virt_boundary_mask and max segment size") instead of
fixing the issue properly.

Ensure that the SCSI mid layer doesn't set the default low
max_segment_size limit for this case, and check for invalid
max_segment_size values in the host template, similar to the original
block layer check given that SCSI devices can't be stacked.

This fixes reported data corruption on storvsc, although as far as I can
tell storvsc always failed to properly set the max_segment_size limit as
the SCSI APIs historically applied that when setting up the host, while
storvsc only set the virt_boundary_mask when configuring the scsi_device.

Fixes: 81988a0e6b03 ("storvsc: get rid of bounce buffer")
Fixes: b561ea56a264 ("block: allow device to have both virt_boundary_mask and max segment size")
Reported-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/hosts.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index e021f1106bea..6ca7be197dfe 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -473,10 +473,19 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
 	else
 		shost->max_sectors = SCSI_DEFAULT_MAX_SECTORS;
 
-	if (sht->max_segment_size)
-		shost->max_segment_size = sht->max_segment_size;
-	else
-		shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
+	if (sht->virt_boundary_mask)
+		shost->virt_boundary_mask = sht->virt_boundary_mask;
+
+	if (shost->virt_boundary_mask) {
+		WARN_ON_ONCE(sht->max_segment_size &&
+			     sht->max_segment_size != UINT_MAX);
+		shost->max_segment_size = UINT_MAX;
+	} else {
+		if (sht->max_segment_size)
+			shost->max_segment_size = sht->max_segment_size;
+		else
+			shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
+	}
 
 	/* 32-byte (dword) is a common minimum for HBAs. */
 	if (sht->dma_alignment)
@@ -492,9 +501,6 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
 	else
 		shost->dma_boundary = 0xffffffff;
 
-	if (sht->virt_boundary_mask)
-		shost->virt_boundary_mask = sht->virt_boundary_mask;
-
 	device_initialize(&shost->shost_gendev);
 	dev_set_name(&shost->shost_gendev, "host%d", shost->host_no);
 	shost->shost_gendev.bus = &scsi_bus_type;
-- 
2.47.2

Re: [PATCH 2/2] scsi: enforce unlimited max_segment_size when virt_boundary_mask is set

[John Garry]

On 23/06/2025 09:02, Christoph Hellwig wrote:
> The virt_boundary_mask limit requires an unlimited max_segment_size for
> bio splitting to not corrupt data.  Historically, the block layer tried
> to validate this, although the check was half-hearted until the addition
> of the atomic queue limits API.  The full blown check than triggered
> issues with stacked devices incorrectly inheriting limits such as the
> virt boundary and got disabled in commit b561ea56a264 ("block: allow
> device to have both virt_boundary_mask and max segment size") instead of
> fixing the issue properly.
> 
> Ensure that the SCSI mid layer doesn't set the default low
> max_segment_size limit for this case, and check for invalid
> max_segment_size values in the host template, similar to the original
> block layer check given that SCSI devices can't be stacked.
> 
> This fixes reported data corruption on storvsc, although as far as I can
> tell storvsc always failed to properly set the max_segment_size limit as
> the SCSI APIs historically applied that when setting up the host, while
> storvsc only set the virt_boundary_mask when configuring the scsi_device.
> 
> Fixes: 81988a0e6b03 ("storvsc: get rid of bounce buffer")
> Fixes: b561ea56a264 ("block: allow device to have both virt_boundary_mask and max segment size")
> Reported-by: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Regardless of nitpicking of coding style, FWIW:

Reviewed-by: John Garry <john.g.garry@oracle.com>

> ---
>   drivers/scsi/hosts.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index e021f1106bea..6ca7be197dfe 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -473,10 +473,19 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
>   	else
>   		shost->max_sectors = SCSI_DEFAULT_MAX_SECTORS;
>   
> -	if (sht->max_segment_size)
> -		shost->max_segment_size = sht->max_segment_size;
> -	else
> -		shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
> +	if (sht->virt_boundary_mask)
> +		shost->virt_boundary_mask = sht->virt_boundary_mask;

nit: you could just always set shost->virt_boundary_mask = 
sht->virt_boundary_mask

> +
> +	if (shost->virt_boundary_mask) {

Or combine into a single if-else statement.

> +		WARN_ON_ONCE(sht->max_segment_size &&
> +			     sht->max_segment_size != UINT_MAX);
> +		shost->max_segment_size = UINT_MAX;
> +	} else {

else if might be nicer, by maybe not as (I think) {} should be used and 
that is not pretty for single line statements.

> +		if (sht->max_segment_size)
 > +			shost->max_segment_size = sht->max_segment_size;> +		else
> +			shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
> +	}
>   
>   	/* 32-byte (dword) is a common minimum for HBAs. */
>   	if (sht->dma_alignment)
> @@ -492,9 +501,6 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
>   	else
>   		shost->dma_boundary = 0xffffffff;
>   
> -	if (sht->virt_boundary_mask)
> -		shost->virt_boundary_mask = sht->virt_boundary_mask;
> -
>   	device_initialize(&shost->shost_gendev);
>   	dev_set_name(&shost->shost_gendev, "host%d", shost->host_no);
>   	shost->shost_gendev.bus = &scsi_bus_type;

Re: [PATCH 2/2] scsi: enforce unlimited max_segment_size when virt_boundary_mask is set

[Ming Lei]

On Mon, Jun 23, 2025 at 10:02:54AM +0200, Christoph Hellwig wrote:
> The virt_boundary_mask limit requires an unlimited max_segment_size for
> bio splitting to not corrupt data.  Historically, the block layer tried
> to validate this, although the check was half-hearted until the addition
> of the atomic queue limits API.  The full blown check than triggered
> issues with stacked devices incorrectly inheriting limits such as the
> virt boundary and got disabled in commit b561ea56a264 ("block: allow
> device to have both virt_boundary_mask and max segment size") instead of
> fixing the issue properly.
> 
> Ensure that the SCSI mid layer doesn't set the default low
> max_segment_size limit for this case, and check for invalid
> max_segment_size values in the host template, similar to the original
> block layer check given that SCSI devices can't be stacked.
> 
> This fixes reported data corruption on storvsc, although as far as I can
> tell storvsc always failed to properly set the max_segment_size limit as
> the SCSI APIs historically applied that when setting up the host, while
> storvsc only set the virt_boundary_mask when configuring the scsi_device.
> 
> Fixes: 81988a0e6b03 ("storvsc: get rid of bounce buffer")
> Fixes: b561ea56a264 ("block: allow device to have both virt_boundary_mask and max segment size")
> Reported-by: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

Re: [PATCH 2/2] scsi: enforce unlimited max_segment_size when virt_boundary_mask is set

[Christoph Hellwig]

On Mon, Jun 23, 2025 at 09:37:10AM +0100, John Garry wrote:
>> -	else
>> -		shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
>> +	if (sht->virt_boundary_mask)
>> +		shost->virt_boundary_mask = sht->virt_boundary_mask;
>
> nit: you could just always set shost->virt_boundary_mask = 
> sht->virt_boundary_mask

I could, but it would change behavior and break drivers.  The SCSI
midlayer allows overriding the template provided values in the host
itself after allocating and before adding it.  For the
virt_boundary_mask that features is used by iser and srp.

>> +	if (shost->virt_boundary_mask) {
>
> Or combine into a single if-else statement.
>
>> +		WARN_ON_ONCE(sht->max_segment_size &&
>> +			     sht->max_segment_size != UINT_MAX);
>> +		shost->max_segment_size = UINT_MAX;
>> +	} else {
>
> else if might be nicer, by maybe not as (I think) {} should be used and 
> that is not pretty for single line statements.

I also really want to keep the virt boundary vs non virt boundary cases
visually separe as that's the main branch.

Re: [PATCH 2/2] scsi: enforce unlimited max_segment_size when virt_boundary_mask is set

[John Garry]

On 23/06/2025 14:35, Christoph Hellwig wrote:
> On Mon, Jun 23, 2025 at 09:37:10AM +0100, John Garry wrote:
>>> -	else
>>> -		shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
>>> +	if (sht->virt_boundary_mask)
>>> +		shost->virt_boundary_mask = sht->virt_boundary_mask;
>> nit: you could just always set shost->virt_boundary_mask =
>> sht->virt_boundary_mask
> I could, but it would change behavior and break drivers.  The SCSI
> midlayer allows overriding the template provided values in the host
> itself after allocating and before adding it.  For the
> virt_boundary_mask that features is used by iser and srp.

Since shost is zero-init'ed, I did not think that my suggestion for this 
minor simplification in scsi_host_alloc() logically changes anything.

Re: [PATCH 2/2] scsi: enforce unlimited max_segment_size when virt_boundary_mask is set

[Christoph Hellwig]

On Mon, Jun 23, 2025 at 03:03:27PM +0100, John Garry wrote:
> On 23/06/2025 14:35, Christoph Hellwig wrote:
>> On Mon, Jun 23, 2025 at 09:37:10AM +0100, John Garry wrote:
>>>> -	else
>>>> -		shost->max_segment_size = BLK_MAX_SEGMENT_SIZE;
>>>> +	if (sht->virt_boundary_mask)
>>>> +		shost->virt_boundary_mask = sht->virt_boundary_mask;
>>> nit: you could just always set shost->virt_boundary_mask =
>>> sht->virt_boundary_mask
>> I could, but it would change behavior and break drivers.  The SCSI
>> midlayer allows overriding the template provided values in the host
>> itself after allocating and before adding it.  For the
>> virt_boundary_mask that features is used by iser and srp.
>
> Since shost is zero-init'ed, I did not think that my suggestion for this 
> minor simplification in scsi_host_alloc() logically changes anything.

Oh, you're right - I thought we did the sht assignments in scsi_add_host.
So the changes would be fine.  But that also means we don't catch
conflicts added by the direct shost manipulation.

Re: [PATCH 1/2] RDMA/srp: don't set a max_segment_size when virt_boundary_mask is set

[Bart Van Assche]

On 6/23/25 1:02 AM, Christoph Hellwig wrote:
> virt_boundary_mask implies an unlimited max_segment_size.  Setting both
> can lead to data corruption, and we're going to check for this in the
> SCSI midlayer soon.

Please make this patch description more detailed and mention that
__blk_rq_map_sg() may split sg-lists such that the virt_boundary_mask is
not respected if max_segment_size != UINT_MAX.

Thanks,

Bart.

Re: [PATCH 1/2] RDMA/srp: don't set a max_segment_size when virt_boundary_mask is set

[Hannes Reinecke]

On 6/23/25 10:02, Christoph Hellwig wrote:
> virt_boundary_mask implies an unlimited max_segment_size.  Setting both
> can lead to data corruption, and we're going to check for this in the
> SCSI midlayer soon.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 1378651735f6..23ed2fc688f0 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -3705,9 +3705,10 @@ static ssize_t add_target_store(struct device *dev,
>   	target_host->max_id      = 1;
>   	target_host->max_lun     = -1LL;
>   	target_host->max_cmd_len = sizeof ((struct srp_cmd *) (void *) 0L)->cdb;
> -	target_host->max_segment_size = ib_dma_max_seg_size(ibdev);
>   
> -	if (!(ibdev->attrs.kernel_cap_flags & IBK_SG_GAPS_REG))
> +	if (ibdev->attrs.kernel_cap_flags & IBK_SG_GAPS_REG)
> +		target_host->max_segment_size = ib_dma_max_seg_size(ibdev);
> +	else
>   		target_host->virt_boundary_mask = ~srp_dev->mr_page_mask;
>   
>   	target = host_to_target(target_host);

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 2/2] scsi: enforce unlimited max_segment_size when virt_boundary_mask is set

[Hannes Reinecke]

On 6/23/25 10:02, Christoph Hellwig wrote:
> The virt_boundary_mask limit requires an unlimited max_segment_size for
> bio splitting to not corrupt data.  Historically, the block layer tried
> to validate this, although the check was half-hearted until the addition
> of the atomic queue limits API.  The full blown check than triggered
> issues with stacked devices incorrectly inheriting limits such as the
> virt boundary and got disabled in commit b561ea56a264 ("block: allow
> device to have both virt_boundary_mask and max segment size") instead of
> fixing the issue properly.
> 
> Ensure that the SCSI mid layer doesn't set the default low
> max_segment_size limit for this case, and check for invalid
> max_segment_size values in the host template, similar to the original
> block layer check given that SCSI devices can't be stacked.
> 
> This fixes reported data corruption on storvsc, although as far as I can
> tell storvsc always failed to properly set the max_segment_size limit as
> the SCSI APIs historically applied that when setting up the host, while
> storvsc only set the virt_boundary_mask when configuring the scsi_device.
> 
> Fixes: 81988a0e6b03 ("storvsc: get rid of bounce buffer")
> Fixes: b561ea56a264 ("block: allow device to have both virt_boundary_mask and max segment size")
> Reported-by: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/hosts.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: fix virt_boundary_mask handling in SCSI

[Laurence Oberman]

On Mon, 2025-06-23 at 10:02 +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series fixes a corruption when drivers using virt_boundary_mask
> set
> a limited max_segment_size by accident, which Red Hat reported as
> causing
> data corruption with storvsc.  I did audit the tree and also found
> that
> this can affect SRP and iSER as well.
> 
> Note that I've dropped the Tested-by from Laurence because the patch
> changed very slightly from the last version.
> 
> Diffstat:
>  infiniband/ulp/srp/ib_srp.c |    5 +++--
>  scsi/hosts.c                |   20 +++++++++++++-------
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
Grabbing latest and will test tomorrow and reply

Re: fix virt_boundary_mask handling in SCSI

[Laurence Oberman]

On Tue, 2025-06-24 at 10:21 -0400, Laurence Oberman wrote:
> On Mon, 2025-06-23 at 10:02 +0200, Christoph Hellwig wrote:
> > Hi all,
> > 
> > this series fixes a corruption when drivers using
> > virt_boundary_mask
> > set
> > a limited max_segment_size by accident, which Red Hat reported as
> > causing
> > data corruption with storvsc.  I did audit the tree and also found
> > that
> > this can affect SRP and iSER as well.
> > 
> > Note that I've dropped the Tested-by from Laurence because the
> > patch
> > changed very slightly from the last version.
> > 
> > Diffstat:
> >  infiniband/ulp/srp/ib_srp.c |    5 +++--
> >  scsi/hosts.c                |   20 +++++++++++++-------
> >  2 files changed, 16 insertions(+), 9 deletions(-)
> > 
> Grabbing latest and will test tomorrow and reply
> 
For the series looks good.
Same testing shows no corruptions on storvsc for the REDO so passed.
For SRP initiators generic testing done with fio and passed, unable to
test SRP LUNS with Oracle REDO at this time.

Here it is, enough reviewers already so just the testing
Patches were applied to a 9.6 kernel because I needed such a kernel for
Oracle compatiility.

tested-by: Laurence Oberman <oberman@redhat.com>

Re: fix virt_boundary_mask handling in SCSI

[Laurence Oberman]

On Tue, 2025-06-24 at 12:11 -0400, Laurence Oberman wrote:
> On Tue, 2025-06-24 at 10:21 -0400, Laurence Oberman wrote:
> > On Mon, 2025-06-23 at 10:02 +0200, Christoph Hellwig wrote:
> > > Hi all,
> > > 
> > > this series fixes a corruption when drivers using
> > > virt_boundary_mask
> > > set
> > > a limited max_segment_size by accident, which Red Hat reported as
> > > causing
> > > data corruption with storvsc.  I did audit the tree and also
> > > found
> > > that
> > > this can affect SRP and iSER as well.
> > > 
> > > Note that I've dropped the Tested-by from Laurence because the
> > > patch
> > > changed very slightly from the last version.
> > > 
> > > Diffstat:
> > >  infiniband/ulp/srp/ib_srp.c |    5 +++--
> > >  scsi/hosts.c                |   20 +++++++++++++-------
> > >  2 files changed, 16 insertions(+), 9 deletions(-)
> > > 
> > Grabbing latest and will test tomorrow and reply
> > 
> For the series looks good.
> Same testing shows no corruptions on storvsc for the REDO so passed.
> For SRP initiators generic testing done with fio and passed, unable
> to
> test SRP LUNS with Oracle REDO at this time.
> 
> Here it is, enough reviewers already so just the testing
> Patches were applied to a 9.6 kernel because I needed such a kernel
> for
> Oracle compatiility.
> 
> tested-by: Laurence Oberman <oberman@redhat.com>
> 
> 
> 
Nit, fix my email, dropped an l should be loberman@redhat.  com of
course