[PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[Christoph Hellwig]

Having a variable length array at the end of scsi_stream_status_header
only cause problems.  Remove it and switch the one place that actually
used it to use the struct member directly following in the actual on-disk
structure instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c         | 2 +-
 include/scsi/scsi_proto.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 950d8c9fb884..3f6e87705b62 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3215,7 +3215,7 @@ static bool sd_is_perm_stream(struct scsi_disk *sdkp, unsigned int stream_id)
 		return false;
 	if (get_unaligned_be32(&buf.h.len) < sizeof(struct scsi_stream_status))
 		return false;
-	return buf.h.stream_status[0].perm;
+	return buf.s.perm;
 }
 
 static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer)
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index aeca37816506..bc8f2b2226be 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -349,7 +349,6 @@ struct scsi_stream_status_header {
 	__be32 len;	/* length in bytes of stream_status[] array. */
 	u16 reserved;
 	__be16 number_of_open_streams;
-	DECLARE_FLEX_ARRAY(struct scsi_stream_status, stream_status);
 };
 
 static_assert(sizeof(struct scsi_stream_status_header) == 8);
-- 
2.47.2

Re: [PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[John Garry]

On 01/05/2025 19:16, Christoph Hellwig wrote:
> Having a variable length array at the end of scsi_stream_status_header
> only cause problems.  Remove it and switch the one place that actually
> used it to use the struct member directly following in the actual on-disk
> structure instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[Bart Van Assche]

On 5/1/25 11:16 AM, Christoph Hellwig wrote:
> Having a variable length array at the end of scsi_stream_status_header
> only cause problems.  Remove it and switch the one place that actually
> used it to use the struct member directly following in the actual on-disk
> structure instead.

"on-disk" is misleading since these data structures should not be stored
on the storage medium.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 950d8c9fb884..3f6e87705b62 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3215,7 +3215,7 @@ static bool sd_is_perm_stream(struct scsi_disk *sdkp, unsigned int stream_id)
>   		return false;
>   	if (get_unaligned_be32(&buf.h.len) < sizeof(struct scsi_stream_status))
>   		return false;
> -	return buf.h.stream_status[0].perm;
> +	return buf.s.perm;
>   }
>   
>   static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer)
> diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
> index aeca37816506..bc8f2b2226be 100644
> --- a/include/scsi/scsi_proto.h
> +++ b/include/scsi/scsi_proto.h
> @@ -349,7 +349,6 @@ struct scsi_stream_status_header {
>   	__be32 len;	/* length in bytes of stream_status[] array. */
>   	u16 reserved;
>   	__be16 number_of_open_streams;
> -	DECLARE_FLEX_ARRAY(struct scsi_stream_status, stream_status);
>   };
>   
>   static_assert(sizeof(struct scsi_stream_status_header) == 8);

There is an implicit assumption behind this patch, namely that the 
compiler does not insert padding bytes between struct
scsi_stream_status_header and struct scsi_stream_status. Shouldn't this 
be made explicit with a BUILD_BUG_ON() expression?

Thanks,

Bart.

Re: [PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[Christoph Hellwig]

On Sat, May 03, 2025 at 12:05:46PM -0700, Bart Van Assche wrote:
> There is an implicit assumption behind this patch, namely that the compiler 
> does not insert padding bytes between struct
> scsi_stream_status_header and struct scsi_stream_status. Shouldn't this be 
> made explicit with a BUILD_BUG_ON() expression?

We make that assumption for all our naturally alignend structures 
in every on-disk and on the wire structure.

Re: [PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[Bart Van Assche]

On 5/1/25 11:16 AM, Christoph Hellwig wrote:
> diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
> index aeca37816506..bc8f2b2226be 100644
> --- a/include/scsi/scsi_proto.h
> +++ b/include/scsi/scsi_proto.h
> @@ -349,7 +349,6 @@ struct scsi_stream_status_header {
>   	__be32 len;	/* length in bytes of stream_status[] array. */
>   	u16 reserved;
>   	__be16 number_of_open_streams;
> -	DECLARE_FLEX_ARRAY(struct scsi_stream_status, stream_status);
>   };

An additional note: the following comment needs to be updated since
the member it refers to is removed:

/* length in bytes of stream_status[] array. */

Bart.

[PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[Christoph Hellwig]

Having a variable length array at the end of scsi_stream_status_header
only cause problems.  Remove it and switch sd_is_perm_stream which is
the only place that currently uses it to use the scsi_stream_status
directly following it in the local buf structure.

Besides being a much better data structure design, this also avoids
a -Wflex-array-member-not-at-end warning.

Reported-by: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/sd.c         | 2 +-
 include/scsi/scsi_proto.h | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 950d8c9fb884..3f6e87705b62 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3215,7 +3215,7 @@ static bool sd_is_perm_stream(struct scsi_disk *sdkp, unsigned int stream_id)
 		return false;
 	if (get_unaligned_be32(&buf.h.len) < sizeof(struct scsi_stream_status))
 		return false;
-	return buf.h.stream_status[0].perm;
+	return buf.s.perm;
 }
 
 static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer)
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index aeca37816506..f64385cde5b9 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -346,10 +346,9 @@ static_assert(sizeof(struct scsi_stream_status) == 8);
 
 /* GET STREAM STATUS parameter data */
 struct scsi_stream_status_header {
-	__be32 len;	/* length in bytes of stream_status[] array. */
+	__be32 len;	/* length in bytes of following payload */
 	u16 reserved;
 	__be16 number_of_open_streams;
-	DECLARE_FLEX_ARRAY(struct scsi_stream_status, stream_status);
 };
 
 static_assert(sizeof(struct scsi_stream_status_header) == 8);
-- 
2.47.2

Re: [PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[Bart Van Assche]

On 5/4/25 11:06 PM, Christoph Hellwig wrote:
> Having a variable length array at the end of scsi_stream_status_header
> only cause problems.  Remove it and switch sd_is_perm_stream which is
> the only place that currently uses it to use the scsi_stream_status
> directly following it in the local buf structure.
> 
> Besides being a much better data structure design, this also avoids
> a -Wflex-array-member-not-at-end warning.
> 
> Reported-by: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> ---
>   drivers/scsi/sd.c         | 2 +-
>   include/scsi/scsi_proto.h | 3 +--
>   2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 950d8c9fb884..3f6e87705b62 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3215,7 +3215,7 @@ static bool sd_is_perm_stream(struct scsi_disk *sdkp, unsigned int stream_id)
>   		return false;
>   	if (get_unaligned_be32(&buf.h.len) < sizeof(struct scsi_stream_status))
>   		return false;
> -	return buf.h.stream_status[0].perm;
> +	return buf.s.perm;
>   }
>   
>   static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer)
> diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
> index aeca37816506..f64385cde5b9 100644
> --- a/include/scsi/scsi_proto.h
> +++ b/include/scsi/scsi_proto.h
> @@ -346,10 +346,9 @@ static_assert(sizeof(struct scsi_stream_status) == 8);
>   
>   /* GET STREAM STATUS parameter data */
>   struct scsi_stream_status_header {
> -	__be32 len;	/* length in bytes of stream_status[] array. */
> +	__be32 len;	/* length in bytes of following payload */
>   	u16 reserved;
>   	__be16 number_of_open_streams;
> -	DECLARE_FLEX_ARRAY(struct scsi_stream_status, stream_status);
>   };
>   
>   static_assert(sizeof(struct scsi_stream_status_header) == 8);

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[Martin K. Petersen]

Christoph,

> Having a variable length array at the end of scsi_stream_status_header
> only cause problems. Remove it and switch sd_is_perm_stream which is
> the only place that currently uses it to use the scsi_stream_status
> directly following it in the local buf structure.

Applied to 6.16/scsi-staging, thanks!

-- 
Martin K. Petersen

Re: [PATCH] scsi: remove the stream_status member from scsi_stream_status_header

[Martin K. Petersen]

On Mon, 05 May 2025 08:06:37 +0200, Christoph Hellwig wrote:

> Having a variable length array at the end of scsi_stream_status_header
> only cause problems.  Remove it and switch sd_is_perm_stream which is
> the only place that currently uses it to use the scsi_stream_status
> directly following it in the local buf structure.
> 
> Besides being a much better data structure design, this also avoids
> a -Wflex-array-member-not-at-end warning.
> 
> [...]

Applied to 6.16/scsi-queue, thanks!

[1/1] scsi: remove the stream_status member from scsi_stream_status_header
      https://git.kernel.org/mkp/scsi/c/cd6856d38881

-- 
Martin K. Petersen	Oracle Linux Engineering