* [PATCH 0/2][next] scsi: qla2xxx: Replace one-element array with flexible-array member
@ 2022-11-18 23:46 Gustavo A. R. Silva
2022-11-18 23:47 ` [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper Gustavo A. R. Silva
2022-11-18 23:47 ` [PATCH 2/2][next] scsi: qla2xxx: Use struct_size() in code related to struct ct_sns_gpnft_rsp Gustavo A. R. Silva
0 siblings, 2 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2022-11-18 23:46 UTC (permalink / raw)
To: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali
Cc: linux-scsi, linux-kernel, Gustavo A. R. Silva, linux-hardening
Hi!
This series aims to replace a one-element array with flexible-array
member in drivers/scsi/qla2xxx/qla_def.h through the use of the
DECLARE_FLEX_ARRAY() helper.
This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy() and help us make progress towards globally
enabling -fstrict-flex-arrays=3 [1].
Link: https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
Gustavo A. R. Silva (2):
scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY()
helper
scsi: qla2xxx: Use struct_size() in code related to struct
ct_sns_gpnft_rsp
drivers/scsi/qla2xxx/qla_def.h | 4 ++--
drivers/scsi/qla2xxx/qla_gs.c | 5 ++---
2 files changed, 4 insertions(+), 5 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper
2022-11-18 23:46 [PATCH 0/2][next] scsi: qla2xxx: Replace one-element array with flexible-array member Gustavo A. R. Silva
@ 2022-11-18 23:47 ` Gustavo A. R. Silva
2022-11-19 7:09 ` Kees Cook
2022-11-19 8:44 ` Christophe JAILLET
2022-11-18 23:47 ` [PATCH 2/2][next] scsi: qla2xxx: Use struct_size() in code related to struct ct_sns_gpnft_rsp Gustavo A. R. Silva
1 sibling, 2 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2022-11-18 23:47 UTC (permalink / raw)
To: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali
Cc: linux-scsi, linux-kernel, Gustavo A. R. Silva, linux-hardening
One-element arrays as fake flex arrays are deprecated and we are moving
towards adopting C99 flexible-array members, instead. So, replace
one-element array declaration in struct ct_sns_gpnft_rsp, which is
ultimately being used inside a union:
drivers/scsi/qla2xxx/qla_def.h:
3240 struct ct_sns_gpnft_pkt {
3241 union {
3242 struct ct_sns_req req;
3243 struct ct_sns_gpnft_rsp rsp;
3244 } p;
3245 };
Important to mention is that doing a build before/after this patch results
in no binary differences.
This help us make progress towards globally enabling
-fstrict-flex-arrays=3 [1].
Link: https://github.com/KSPP/linux/issues/245
Link: https://github.com/KSPP/linux/issues/193
Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
drivers/scsi/qla2xxx/qla_def.h | 4 ++--
drivers/scsi/qla2xxx/qla_gs.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index a26a373be9da..1eea977ef426 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -3151,12 +3151,12 @@ struct ct_sns_gpnft_rsp {
uint8_t vendor_unique;
};
/* Assume the largest number of targets for the union */
- struct ct_sns_gpn_ft_data {
+ DECLARE_FLEX_ARRAY(struct ct_sns_gpn_ft_data {
u8 control_byte;
u8 port_id[3];
u32 reserved;
u8 port_name[8];
- } entries[1];
+ }, entries);
};
/* CT command response */
diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
index 64ab070b8716..69d3bc795f90 100644
--- a/drivers/scsi/qla2xxx/qla_gs.c
+++ b/drivers/scsi/qla2xxx/qla_gs.c
@@ -4073,7 +4073,7 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp)
sp->u.iocb_cmd.u.ctarg.req_size = GPN_FT_REQ_SIZE;
rspsz = sizeof(struct ct_sns_gpnft_rsp) +
- ((vha->hw->max_fibre_devices - 1) *
+ (vha->hw->max_fibre_devices *
sizeof(struct ct_sns_gpn_ft_data));
sp->u.iocb_cmd.u.ctarg.rsp = dma_alloc_coherent(&vha->hw->pdev->dev,
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2][next] scsi: qla2xxx: Use struct_size() in code related to struct ct_sns_gpnft_rsp
2022-11-18 23:46 [PATCH 0/2][next] scsi: qla2xxx: Replace one-element array with flexible-array member Gustavo A. R. Silva
2022-11-18 23:47 ` [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper Gustavo A. R. Silva
@ 2022-11-18 23:47 ` Gustavo A. R. Silva
2022-11-19 7:10 ` Kees Cook
1 sibling, 1 reply; 11+ messages in thread
From: Gustavo A. R. Silva @ 2022-11-18 23:47 UTC (permalink / raw)
To: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali
Cc: linux-scsi, linux-kernel, Gustavo A. R. Silva, linux-hardening
Prefer struct_size() over open-coded versions of idiom:
sizeof(struct-with-flex-array) + sizeof(typeof-flex-array-elements) * count
where count is the max number of items the flexible array is supposed to
contain.
Link: https://github.com/KSPP/linux/issues/160
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
drivers/scsi/qla2xxx/qla_gs.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
index 69d3bc795f90..27e1df56b0fb 100644
--- a/drivers/scsi/qla2xxx/qla_gs.c
+++ b/drivers/scsi/qla2xxx/qla_gs.c
@@ -4072,9 +4072,8 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp)
}
sp->u.iocb_cmd.u.ctarg.req_size = GPN_FT_REQ_SIZE;
- rspsz = sizeof(struct ct_sns_gpnft_rsp) +
- (vha->hw->max_fibre_devices *
- sizeof(struct ct_sns_gpn_ft_data));
+ rspsz = struct_size((struct ct_sns_gpnft_rsp *)0, entries,
+ vha->hw->max_fibre_devices);
sp->u.iocb_cmd.u.ctarg.rsp = dma_alloc_coherent(&vha->hw->pdev->dev,
rspsz,
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper
2022-11-18 23:47 ` [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper Gustavo A. R. Silva
@ 2022-11-19 7:09 ` Kees Cook
2022-11-19 8:44 ` Christophe JAILLET
1 sibling, 0 replies; 11+ messages in thread
From: Kees Cook @ 2022-11-19 7:09 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali, linux-scsi,
linux-kernel, linux-hardening
On Fri, Nov 18, 2022 at 05:47:13PM -0600, Gustavo A. R. Silva wrote:
> One-element arrays as fake flex arrays are deprecated and we are moving
> towards adopting C99 flexible-array members, instead. So, replace
> one-element array declaration in struct ct_sns_gpnft_rsp, which is
> ultimately being used inside a union:
>
> drivers/scsi/qla2xxx/qla_def.h:
> 3240 struct ct_sns_gpnft_pkt {
> 3241 union {
> 3242 struct ct_sns_req req;
> 3243 struct ct_sns_gpnft_rsp rsp;
> 3244 } p;
> 3245 };
>
> Important to mention is that doing a build before/after this patch results
> in no binary differences.
>
> This help us make progress towards globally enabling
> -fstrict-flex-arrays=3 [1].
>
> Link: https://github.com/KSPP/linux/issues/245
> Link: https://github.com/KSPP/linux/issues/193
> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2][next] scsi: qla2xxx: Use struct_size() in code related to struct ct_sns_gpnft_rsp
2022-11-18 23:47 ` [PATCH 2/2][next] scsi: qla2xxx: Use struct_size() in code related to struct ct_sns_gpnft_rsp Gustavo A. R. Silva
@ 2022-11-19 7:10 ` Kees Cook
2022-11-19 7:20 ` Gustavo A. R. Silva
0 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2022-11-19 7:10 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali, linux-scsi,
linux-kernel, linux-hardening
On Fri, Nov 18, 2022 at 05:47:56PM -0600, Gustavo A. R. Silva wrote:
> Prefer struct_size() over open-coded versions of idiom:
>
> sizeof(struct-with-flex-array) + sizeof(typeof-flex-array-elements) * count
>
> where count is the max number of items the flexible array is supposed to
> contain.
>
> Link: https://github.com/KSPP/linux/issues/160
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> drivers/scsi/qla2xxx/qla_gs.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
> index 69d3bc795f90..27e1df56b0fb 100644
> --- a/drivers/scsi/qla2xxx/qla_gs.c
> +++ b/drivers/scsi/qla2xxx/qla_gs.c
> @@ -4072,9 +4072,8 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp)
> }
> sp->u.iocb_cmd.u.ctarg.req_size = GPN_FT_REQ_SIZE;
>
> - rspsz = sizeof(struct ct_sns_gpnft_rsp) +
> - (vha->hw->max_fibre_devices *
> - sizeof(struct ct_sns_gpn_ft_data));
> + rspsz = struct_size((struct ct_sns_gpnft_rsp *)0, entries,
> + vha->hw->max_fibre_devices);
This should be able to use sp->u.iocb_cmd.u.ctarg.rsp instead of the
explicit struct with a NULL. (It's just using typeof() internally, so
it's okay that it isn't allocated yet.)
-Kees
>
> sp->u.iocb_cmd.u.ctarg.rsp = dma_alloc_coherent(&vha->hw->pdev->dev,
> rspsz,
> --
> 2.34.1
>
--
Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2][next] scsi: qla2xxx: Use struct_size() in code related to struct ct_sns_gpnft_rsp
2022-11-19 7:10 ` Kees Cook
@ 2022-11-19 7:20 ` Gustavo A. R. Silva
2022-11-19 7:49 ` Gustavo A. R. Silva
0 siblings, 1 reply; 11+ messages in thread
From: Gustavo A. R. Silva @ 2022-11-19 7:20 UTC (permalink / raw)
To: Kees Cook
Cc: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali, linux-scsi,
linux-kernel, linux-hardening
On Fri, Nov 18, 2022 at 11:10:44PM -0800, Kees Cook wrote:
> On Fri, Nov 18, 2022 at 05:47:56PM -0600, Gustavo A. R. Silva wrote:
> > Prefer struct_size() over open-coded versions of idiom:
> >
> > sizeof(struct-with-flex-array) + sizeof(typeof-flex-array-elements) * count
> >
> > where count is the max number of items the flexible array is supposed to
> > contain.
> >
> > Link: https://github.com/KSPP/linux/issues/160
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > ---
> > drivers/scsi/qla2xxx/qla_gs.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
> > index 69d3bc795f90..27e1df56b0fb 100644
> > --- a/drivers/scsi/qla2xxx/qla_gs.c
> > +++ b/drivers/scsi/qla2xxx/qla_gs.c
> > @@ -4072,9 +4072,8 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp)
> > }
> > sp->u.iocb_cmd.u.ctarg.req_size = GPN_FT_REQ_SIZE;
> >
> > - rspsz = sizeof(struct ct_sns_gpnft_rsp) +
> > - (vha->hw->max_fibre_devices *
> > - sizeof(struct ct_sns_gpn_ft_data));
> > + rspsz = struct_size((struct ct_sns_gpnft_rsp *)0, entries,
> > + vha->hw->max_fibre_devices);
>
> This should be able to use sp->u.iocb_cmd.u.ctarg.rsp instead of the
> explicit struct with a NULL. (It's just using typeof() internally, so
> it's okay that it isn't allocated yet.)
mmh... yeah; and considering they're already going all the way down to
sp->u.iocb_cmd.u.ctarg.req_size, I think accessing sp->u.iocb_cmd.u.ctarg.rsp
is perfectly fine. :)
I'll respin. Thanks for the feedback!
--
Gustavo
>
> -Kees
>
> >
> > sp->u.iocb_cmd.u.ctarg.rsp = dma_alloc_coherent(&vha->hw->pdev->dev,
> > rspsz,
> > --
> > 2.34.1
> >
>
> --
> Kees Cook
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2][next] scsi: qla2xxx: Use struct_size() in code related to struct ct_sns_gpnft_rsp
2022-11-19 7:20 ` Gustavo A. R. Silva
@ 2022-11-19 7:49 ` Gustavo A. R. Silva
0 siblings, 0 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2022-11-19 7:49 UTC (permalink / raw)
To: Kees Cook
Cc: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali, linux-scsi,
linux-kernel, linux-hardening
On Sat, Nov 19, 2022 at 01:20:09AM -0600, Gustavo A. R. Silva wrote:
> On Fri, Nov 18, 2022 at 11:10:44PM -0800, Kees Cook wrote:
> > On Fri, Nov 18, 2022 at 05:47:56PM -0600, Gustavo A. R. Silva wrote:
> > > Prefer struct_size() over open-coded versions of idiom:
> > >
> > > sizeof(struct-with-flex-array) + sizeof(typeof-flex-array-elements) * count
> > >
> > > where count is the max number of items the flexible array is supposed to
> > > contain.
> > >
> > > Link: https://github.com/KSPP/linux/issues/160
> > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > > ---
> > > drivers/scsi/qla2xxx/qla_gs.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
> > > index 69d3bc795f90..27e1df56b0fb 100644
> > > --- a/drivers/scsi/qla2xxx/qla_gs.c
> > > +++ b/drivers/scsi/qla2xxx/qla_gs.c
> > > @@ -4072,9 +4072,8 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp)
> > > }
> > > sp->u.iocb_cmd.u.ctarg.req_size = GPN_FT_REQ_SIZE;
> > >
> > > - rspsz = sizeof(struct ct_sns_gpnft_rsp) +
> > > - (vha->hw->max_fibre_devices *
> > > - sizeof(struct ct_sns_gpn_ft_data));
> > > + rspsz = struct_size((struct ct_sns_gpnft_rsp *)0, entries,
> > > + vha->hw->max_fibre_devices);
> >
> > This should be able to use sp->u.iocb_cmd.u.ctarg.rsp instead of the
> > explicit struct with a NULL. (It's just using typeof() internally, so
> > it's okay that it isn't allocated yet.)
>
> mmh... yeah; and considering they're already going all the way down to
> sp->u.iocb_cmd.u.ctarg.req_size, I think accessing sp->u.iocb_cmd.u.ctarg.rsp
> is perfectly fine. :)
except that... it seems sp->u.iocb_cmd.u.ctarg.rsp is a pointer to void
and not a struct of type struct ct_sns_gpnft_rsp. :O
drivers/scsi/qla2xxx/qla_def.h:474:
struct ct_arg {
void *iocb;
u16 nport_handle;
dma_addr_t req_dma;
dma_addr_t rsp_dma;
u32 req_size;
u32 rsp_size;
u32 req_allocated_size;
u32 rsp_allocated_size;
void *req;
void *rsp;
port_id_t id;
};
I wonder if you wanted to point out something entirely different...?
--
Gustavo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper
2022-11-18 23:47 ` [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper Gustavo A. R. Silva
2022-11-19 7:09 ` Kees Cook
@ 2022-11-19 8:44 ` Christophe JAILLET
2022-11-19 8:56 ` Gustavo A. R. Silva
1 sibling, 1 reply; 11+ messages in thread
From: Christophe JAILLET @ 2022-11-19 8:44 UTC (permalink / raw)
To: Gustavo A. R. Silva, Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali
Cc: linux-scsi, linux-kernel, linux-hardening
Le 19/11/2022 à 00:47, Gustavo A. R. Silva a écrit :
> One-element arrays as fake flex arrays are deprecated and we are moving
> towards adopting C99 flexible-array members, instead. So, replace
> one-element array declaration in struct ct_sns_gpnft_rsp, which is
> ultimately being used inside a union:
>
> drivers/scsi/qla2xxx/qla_def.h:
> 3240 struct ct_sns_gpnft_pkt {
> 3241 union {
> 3242 struct ct_sns_req req;
> 3243 struct ct_sns_gpnft_rsp rsp;
> 3244 } p;
> 3245 };
>
> Important to mention is that doing a build before/after this patch results
> in no binary differences.
Hi,
even with the:
> rspsz = sizeof(struct ct_sns_gpnft_rsp) +
> - ((vha->hw->max_fibre_devices - 1) *
> + (vha->hw->max_fibre_devices *
> sizeof(struct ct_sns_gpn_ft_data));
change ?
CJ
>
> This help us make progress towards globally enabling
> -fstrict-flex-arrays=3 [1].
>
> Link: https://github.com/KSPP/linux/issues/245
> Link: https://github.com/KSPP/linux/issues/193
> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> drivers/scsi/qla2xxx/qla_def.h | 4 ++--
> drivers/scsi/qla2xxx/qla_gs.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index a26a373be9da..1eea977ef426 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -3151,12 +3151,12 @@ struct ct_sns_gpnft_rsp {
> uint8_t vendor_unique;
> };
> /* Assume the largest number of targets for the union */
> - struct ct_sns_gpn_ft_data {
> + DECLARE_FLEX_ARRAY(struct ct_sns_gpn_ft_data {
> u8 control_byte;
> u8 port_id[3];
> u32 reserved;
> u8 port_name[8];
> - } entries[1];
> + }, entries);
> };
>
> /* CT command response */
> diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
> index 64ab070b8716..69d3bc795f90 100644
> --- a/drivers/scsi/qla2xxx/qla_gs.c
> +++ b/drivers/scsi/qla2xxx/qla_gs.c
> @@ -4073,7 +4073,7 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp)
> sp->u.iocb_cmd.u.ctarg.req_size = GPN_FT_REQ_SIZE;
>
> rspsz = sizeof(struct ct_sns_gpnft_rsp) +
> - ((vha->hw->max_fibre_devices - 1) *
> + (vha->hw->max_fibre_devices *
> sizeof(struct ct_sns_gpn_ft_data));
>
> sp->u.iocb_cmd.u.ctarg.rsp = dma_alloc_coherent(&vha->hw->pdev->dev,
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper
2022-11-19 8:44 ` Christophe JAILLET
@ 2022-11-19 8:56 ` Gustavo A. R. Silva
2022-11-19 10:23 ` Christophe JAILLET
0 siblings, 1 reply; 11+ messages in thread
From: Gustavo A. R. Silva @ 2022-11-19 8:56 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali, linux-scsi,
linux-kernel, linux-hardening
On Sat, Nov 19, 2022 at 09:44:02AM +0100, Christophe JAILLET wrote:
> Le 19/11/2022 à 00:47, Gustavo A. R. Silva a écrit :
> > One-element arrays as fake flex arrays are deprecated and we are moving
> > towards adopting C99 flexible-array members, instead. So, replace
> > one-element array declaration in struct ct_sns_gpnft_rsp, which is
> > ultimately being used inside a union:
> >
> > drivers/scsi/qla2xxx/qla_def.h:
> > 3240 struct ct_sns_gpnft_pkt {
> > 3241 union {
> > 3242 struct ct_sns_req req;
> > 3243 struct ct_sns_gpnft_rsp rsp;
> > 3244 } p;
> > 3245 };
> >
> > Important to mention is that doing a build before/after this patch results
> > in no binary differences.
>
> Hi,
>
> even with the:
>
> > rspsz = sizeof(struct ct_sns_gpnft_rsp) +
> > - ((vha->hw->max_fibre_devices - 1) *
> > + (vha->hw->max_fibre_devices *
> > sizeof(struct ct_sns_gpn_ft_data));
>
> change ?
Yep; that change compensates for the removal of the 1 in the declaration
of entries[].
The above piece of code is a common idiom to calculate the size for an
allocation when a one-element array is involved. In the original code
(vha->hw->max_fibre_devices - 1) compensates for the _extra_ size of one
element of type struct ct_sns_gpn_ft_data in sizeof(struct ct_sns_gpnft_rsp).
--
Gustavo
>
> CJ
>
> >
> > This help us make progress towards globally enabling
> > -fstrict-flex-arrays=3 [1].
> >
> > Link: https://github.com/KSPP/linux/issues/245
> > Link: https://github.com/KSPP/linux/issues/193
> > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > ---
> > drivers/scsi/qla2xxx/qla_def.h | 4 ++--
> > drivers/scsi/qla2xxx/qla_gs.c | 2 +-
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> > index a26a373be9da..1eea977ef426 100644
> > --- a/drivers/scsi/qla2xxx/qla_def.h
> > +++ b/drivers/scsi/qla2xxx/qla_def.h
> > @@ -3151,12 +3151,12 @@ struct ct_sns_gpnft_rsp {
> > uint8_t vendor_unique;
> > };
> > /* Assume the largest number of targets for the union */
> > - struct ct_sns_gpn_ft_data {
> > + DECLARE_FLEX_ARRAY(struct ct_sns_gpn_ft_data {
> > u8 control_byte;
> > u8 port_id[3];
> > u32 reserved;
> > u8 port_name[8];
> > - } entries[1];
> > + }, entries);
> > };
> > /* CT command response */
> > diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
> > index 64ab070b8716..69d3bc795f90 100644
> > --- a/drivers/scsi/qla2xxx/qla_gs.c
> > +++ b/drivers/scsi/qla2xxx/qla_gs.c
> > @@ -4073,7 +4073,7 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp)
> > sp->u.iocb_cmd.u.ctarg.req_size = GPN_FT_REQ_SIZE;
> > rspsz = sizeof(struct ct_sns_gpnft_rsp) +
> > - ((vha->hw->max_fibre_devices - 1) *
> > + (vha->hw->max_fibre_devices *
> > sizeof(struct ct_sns_gpn_ft_data));
> > sp->u.iocb_cmd.u.ctarg.rsp = dma_alloc_coherent(&vha->hw->pdev->dev,
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper
2022-11-19 8:56 ` Gustavo A. R. Silva
@ 2022-11-19 10:23 ` Christophe JAILLET
2022-11-19 19:03 ` Gustavo A. R. Silva
0 siblings, 1 reply; 11+ messages in thread
From: Christophe JAILLET @ 2022-11-19 10:23 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali, linux-scsi,
linux-kernel, linux-hardening
Le 19/11/2022 à 09:56, Gustavo A. R. Silva a écrit :
> On Sat, Nov 19, 2022 at 09:44:02AM +0100, Christophe JAILLET wrote:
>> Le 19/11/2022 à 00:47, Gustavo A. R. Silva a écrit :
>>> One-element arrays as fake flex arrays are deprecated and we are moving
>>> towards adopting C99 flexible-array members, instead. So, replace
>>> one-element array declaration in struct ct_sns_gpnft_rsp, which is
>>> ultimately being used inside a union:
>>>
>>> drivers/scsi/qla2xxx/qla_def.h:
>>> 3240 struct ct_sns_gpnft_pkt {
>>> 3241 union {
>>> 3242 struct ct_sns_req req;
>>> 3243 struct ct_sns_gpnft_rsp rsp;
>>> 3244 } p;
>>> 3245 };
>>>
>>> Important to mention is that doing a build before/after this patch results
>>> in no binary differences.
>>
>> Hi,
>>
>> even with the:
>>
>>> rspsz = sizeof(struct ct_sns_gpnft_rsp) +
>>> - ((vha->hw->max_fibre_devices - 1) *
>>> + (vha->hw->max_fibre_devices *
>>> sizeof(struct ct_sns_gpn_ft_data));
>>
>> change ?
>
> Yep; that change compensates for the removal of the 1 in the declaration
> of entries[].
>
> The above piece of code is a common idiom to calculate the size for an
> allocation when a one-element array is involved. In the original code
> (vha->hw->max_fibre_devices - 1) compensates for the _extra_ size of one
> element of type struct ct_sns_gpn_ft_data in sizeof(struct ct_sns_gpnft_rsp).
>
Yes, I do agree, that the code is equivalent. I was surprised that a
compiler was smart enough to generate the same binary code.
With gcc 11.3.0 (x86_64), I do get some differences when I do:
make drivers/scsi/qla2xxx/qla_gs.o
objdump -D drivers/scsi/qla2xxx/qla_gs.o > before.asm
patch -p1 < patch.diff
make drivers/scsi/qla2xxx/qla_gs.o
objdump -D drivers/scsi/qla2xxx/qla_gs.o > after.asm
diff -u before.asm after.asm
Mostly some slight ordering of instruction changes, but the binary are
not the same.
CJ
> --
> Gustavo
>
>>
>> CJ
>>
>>>
>>> This help us make progress towards globally enabling
>>> -fstrict-flex-arrays=3 [1].
>>>
>>> Link: https://github.com/KSPP/linux/issues/245
>>> Link: https://github.com/KSPP/linux/issues/193
>>> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>> ---
>>> drivers/scsi/qla2xxx/qla_def.h | 4 ++--
>>> drivers/scsi/qla2xxx/qla_gs.c | 2 +-
>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>>> index a26a373be9da..1eea977ef426 100644
>>> --- a/drivers/scsi/qla2xxx/qla_def.h
>>> +++ b/drivers/scsi/qla2xxx/qla_def.h
>>> @@ -3151,12 +3151,12 @@ struct ct_sns_gpnft_rsp {
>>> uint8_t vendor_unique;
>>> };
>>> /* Assume the largest number of targets for the union */
>>> - struct ct_sns_gpn_ft_data {
>>> + DECLARE_FLEX_ARRAY(struct ct_sns_gpn_ft_data {
>>> u8 control_byte;
>>> u8 port_id[3];
>>> u32 reserved;
>>> u8 port_name[8];
>>> - } entries[1];
>>> + }, entries);
>>> };
>>> /* CT command response */
>>> diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
>>> index 64ab070b8716..69d3bc795f90 100644
>>> --- a/drivers/scsi/qla2xxx/qla_gs.c
>>> +++ b/drivers/scsi/qla2xxx/qla_gs.c
>>> @@ -4073,7 +4073,7 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp)
>>> sp->u.iocb_cmd.u.ctarg.req_size = GPN_FT_REQ_SIZE;
>>> rspsz = sizeof(struct ct_sns_gpnft_rsp) +
>>> - ((vha->hw->max_fibre_devices - 1) *
>>> + (vha->hw->max_fibre_devices *
>>> sizeof(struct ct_sns_gpn_ft_data));
>>> sp->u.iocb_cmd.u.ctarg.rsp = dma_alloc_coherent(&vha->hw->pdev->dev,
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper
2022-11-19 10:23 ` Christophe JAILLET
@ 2022-11-19 19:03 ` Gustavo A. R. Silva
0 siblings, 0 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2022-11-19 19:03 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Martin K. Petersen, James E.J. Bottomley,
GR-QLogic-Storage-Upstream, Nilesh Javali, linux-scsi,
linux-kernel, linux-hardening
> Yes, I do agree, that the code is equivalent. I was surprised that a
> compiler was smart enough to generate the same binary code.
We discard the tiny changes that don't affect the logic or the control
flow of the program.
--
Gustavo
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-11-19 19:03 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-18 23:46 [PATCH 0/2][next] scsi: qla2xxx: Replace one-element array with flexible-array member Gustavo A. R. Silva
2022-11-18 23:47 ` [PATCH 1/2][next] scsi: qla2xxx: Replace one-element array with DECLARE_FLEX_ARRAY() helper Gustavo A. R. Silva
2022-11-19 7:09 ` Kees Cook
2022-11-19 8:44 ` Christophe JAILLET
2022-11-19 8:56 ` Gustavo A. R. Silva
2022-11-19 10:23 ` Christophe JAILLET
2022-11-19 19:03 ` Gustavo A. R. Silva
2022-11-18 23:47 ` [PATCH 2/2][next] scsi: qla2xxx: Use struct_size() in code related to struct ct_sns_gpnft_rsp Gustavo A. R. Silva
2022-11-19 7:10 ` Kees Cook
2022-11-19 7:20 ` Gustavo A. R. Silva
2022-11-19 7:49 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).