Message ID | 20200405231339.29612-1-bvanassche@acm.org (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | qla2xxx: Increase the size of struct qla_fcp_prio_cfg to FCP_PRIO_CFG_SIZE | expand |
Hi Bart, On Sun, Apr 05, 2020 at 04:13:39PM -0700, Bart Van Assche wrote: > This patch fixes the following Coverity complaint without changing any > functionality: > > CID 337793 (#1 of 1): Wrong size argument (SIZEOF_MISMATCH) > suspicious_sizeof: Passing argument ha->fcp_prio_cfg of type > struct qla_fcp_prio_cfg * and argument 32768UL to function memset is > suspicious because a multiple of sizeof (struct qla_fcp_prio_cfg) /*48*/ > is expected. > > memset(ha->fcp_prio_cfg, 0, FCP_PRIO_CFG_SIZE); > > Cc: Nilesh Javali <njavali@marvell.com> > Cc: Himanshu Madhani <hmadhani@marvell.com> > Cc: Quinn Tran <qutran@marvell.com> > Cc: Martin Wilck <mwilck@suse.com> > Cc: Daniel Wagner <dwagner@suse.de> > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Looks good to me. Reviewed-by: Daniel Wagner <dwagner@suse.de> Thanks, Daniel
On 4/5/20 6:13 PM, Bart Van Assche wrote: > This patch fixes the following Coverity complaint without changing any > functionality: > > CID 337793 (#1 of 1): Wrong size argument (SIZEOF_MISMATCH) > suspicious_sizeof: Passing argument ha->fcp_prio_cfg of type > struct qla_fcp_prio_cfg * and argument 32768UL to function memset is > suspicious because a multiple of sizeof (struct qla_fcp_prio_cfg) /*48*/ > is expected. > > memset(ha->fcp_prio_cfg, 0, FCP_PRIO_CFG_SIZE); > > Cc: Nilesh Javali <njavali@marvell.com> > Cc: Himanshu Madhani <hmadhani@marvell.com> > Cc: Quinn Tran <qutran@marvell.com> > Cc: Martin Wilck <mwilck@suse.com> > Cc: Daniel Wagner <dwagner@suse.de> > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/qla2xxx/qla_fw.h | 3 ++- > drivers/scsi/qla2xxx/qla_os.c | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/qla2xxx/qla_fw.h b/drivers/scsi/qla2xxx/qla_fw.h > index f9bad5bd7198..647e67c6ba5e 100644 > --- a/drivers/scsi/qla2xxx/qla_fw.h > +++ b/drivers/scsi/qla2xxx/qla_fw.h > @@ -2217,8 +2217,9 @@ struct qla_fcp_prio_cfg { > #define FCP_PRIO_ATTR_PERSIST 0x2 > uint8_t reserved; /* Reserved for future use */ > #define FCP_PRIO_CFG_HDR_SIZE 0x10 > - struct qla_fcp_prio_entry entry[1]; /* fcp priority entries */ > + struct qla_fcp_prio_entry entry[1023]; /* fcp priority entries */ > #define FCP_PRIO_CFG_ENTRY_SIZE 0x20 > + uint8_t reserved2[16]; > }; > > #define FCP_PRIO_CFG_SIZE (32*1024) /* fcp prio data per port*/ > diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c > index d9072ea7c42b..784f3e553f15 100644 > --- a/drivers/scsi/qla2xxx/qla_os.c > +++ b/drivers/scsi/qla2xxx/qla_os.c > @@ -7840,6 +7840,7 @@ qla2x00_module_init(void) > BUILD_BUG_ON(sizeof(struct vf_evfp_entry_24xx) != 56); > BUILD_BUG_ON(sizeof(struct qla_flt_region) != 16); > BUILD_BUG_ON(sizeof(struct qla_flt_header) != 8); > + BUILD_BUG_ON(sizeof(struct qla_fcp_prio_cfg) != FCP_PRIO_CFG_SIZE); > > /* Allocate cache for SRBs. */ > srb_cachep = kmem_cache_create("qla2xxx_srbs", sizeof(srb_t), 0, > Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
On Sun, Apr 05, 2020 at 04:13:39PM -0700, Bart Van Assche wrote: > This patch fixes the following Coverity complaint without changing any > functionality: > > CID 337793 (#1 of 1): Wrong size argument (SIZEOF_MISMATCH) > suspicious_sizeof: Passing argument ha->fcp_prio_cfg of type > struct qla_fcp_prio_cfg * and argument 32768UL to function memset is > suspicious because a multiple of sizeof (struct qla_fcp_prio_cfg) /*48*/ > is expected. > > memset(ha->fcp_prio_cfg, 0, FCP_PRIO_CFG_SIZE); > > --- > drivers/scsi/qla2xxx/qla_fw.h | 3 ++- > drivers/scsi/qla2xxx/qla_os.c | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/qla2xxx/qla_fw.h b/drivers/scsi/qla2xxx/qla_fw.h > index f9bad5bd7198..647e67c6ba5e 100644 > --- a/drivers/scsi/qla2xxx/qla_fw.h > +++ b/drivers/scsi/qla2xxx/qla_fw.h > @@ -2217,8 +2217,9 @@ struct qla_fcp_prio_cfg { > #define FCP_PRIO_ATTR_PERSIST 0x2 > uint8_t reserved; /* Reserved for future use */ > #define FCP_PRIO_CFG_HDR_SIZE 0x10 > - struct qla_fcp_prio_entry entry[1]; /* fcp priority entries */ > + struct qla_fcp_prio_entry entry[1023]; /* fcp priority entries */ > #define FCP_PRIO_CFG_ENTRY_SIZE 0x20 > + uint8_t reserved2[16]; > }; > > #define FCP_PRIO_CFG_SIZE (32*1024) /* fcp prio data per port*/ Hi Bart, A new constant may be introduced to define size of qla_fcp_prio_entry. That would let to drop the magic 32 number here and allow to add one more BUILD_BUG_ON for sizeof(struct qla_fcp_prio_entry). Besides that, Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> > diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c > index d9072ea7c42b..784f3e553f15 100644 > --- a/drivers/scsi/qla2xxx/qla_os.c > +++ b/drivers/scsi/qla2xxx/qla_os.c > @@ -7840,6 +7840,7 @@ qla2x00_module_init(void) > BUILD_BUG_ON(sizeof(struct vf_evfp_entry_24xx) != 56); > BUILD_BUG_ON(sizeof(struct qla_flt_region) != 16); > BUILD_BUG_ON(sizeof(struct qla_flt_header) != 8); > + BUILD_BUG_ON(sizeof(struct qla_fcp_prio_cfg) != FCP_PRIO_CFG_SIZE); > > /* Allocate cache for SRBs. */ > srb_cachep = kmem_cache_create("qla2xxx_srbs", sizeof(srb_t), 0, Thanks, Roman
Roman, >> - struct qla_fcp_prio_entry entry[1]; /* fcp priority entries */ >> + struct qla_fcp_prio_entry entry[1023]; /* fcp priority entries */ >> #define FCP_PRIO_CFG_ENTRY_SIZE 0x20 >> + uint8_t reserved2[16]; >> }; >> >> #define FCP_PRIO_CFG_SIZE (32*1024) /* fcp prio data per port*/ > > A new constant may be introduced to define size of qla_fcp_prio_entry. > That would let to drop the magic 32 number here and allow to add one > more BUILD_BUG_ON for sizeof(struct qla_fcp_prio_entry). I agree that additional sanity testing here would be nice. I wonder what the firmware interface says about the runt entry?
On 2020-04-13 08:30, Roman Bolshakov wrote: > On Sun, Apr 05, 2020 at 04:13:39PM -0700, Bart Van Assche wrote: >> This patch fixes the following Coverity complaint without changing any >> functionality: >> >> CID 337793 (#1 of 1): Wrong size argument (SIZEOF_MISMATCH) >> suspicious_sizeof: Passing argument ha->fcp_prio_cfg of type >> struct qla_fcp_prio_cfg * and argument 32768UL to function memset is >> suspicious because a multiple of sizeof (struct qla_fcp_prio_cfg) /*48*/ >> is expected. >> >> memset(ha->fcp_prio_cfg, 0, FCP_PRIO_CFG_SIZE); >> >> --- >> drivers/scsi/qla2xxx/qla_fw.h | 3 ++- >> drivers/scsi/qla2xxx/qla_os.c | 1 + >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/qla2xxx/qla_fw.h b/drivers/scsi/qla2xxx/qla_fw.h >> index f9bad5bd7198..647e67c6ba5e 100644 >> --- a/drivers/scsi/qla2xxx/qla_fw.h >> +++ b/drivers/scsi/qla2xxx/qla_fw.h >> @@ -2217,8 +2217,9 @@ struct qla_fcp_prio_cfg { >> #define FCP_PRIO_ATTR_PERSIST 0x2 >> uint8_t reserved; /* Reserved for future use */ >> #define FCP_PRIO_CFG_HDR_SIZE 0x10 >> - struct qla_fcp_prio_entry entry[1]; /* fcp priority entries */ >> + struct qla_fcp_prio_entry entry[1023]; /* fcp priority entries */ >> #define FCP_PRIO_CFG_ENTRY_SIZE 0x20 >> + uint8_t reserved2[16]; >> }; >> >> #define FCP_PRIO_CFG_SIZE (32*1024) /* fcp prio data per port*/ > > Hi Bart, > > A new constant may be introduced to define size of qla_fcp_prio_entry. > That would let to drop the magic 32 number here and allow to add one > more BUILD_BUG_ON for sizeof(struct qla_fcp_prio_entry). Hi Roman, The constant FCP_PRIO_CFG_ENTRY_SIZE is only used once, namely in the following code: len = ha->fcp_prio_cfg->num_entries * FCP_PRIO_CFG_ENTRY_SIZE; How about removing the definition of FCP_PRIO_CFG_ENTRY_SIZE and changing FCP_PRIO_CFG_ENTRY_SIZE in the above calculation into sizeof(struct qla_fcp_prio_entry)? Thanks, Bart.
On Mon, Apr 13, 2020 at 08:56:18PM -0700, Bart Van Assche wrote: > On 2020-04-13 08:30, Roman Bolshakov wrote: > > On Sun, Apr 05, 2020 at 04:13:39PM -0700, Bart Van Assche wrote: > >> This patch fixes the following Coverity complaint without changing any > >> functionality: > >> > >> CID 337793 (#1 of 1): Wrong size argument (SIZEOF_MISMATCH) > >> suspicious_sizeof: Passing argument ha->fcp_prio_cfg of type > >> struct qla_fcp_prio_cfg * and argument 32768UL to function memset is > >> suspicious because a multiple of sizeof (struct qla_fcp_prio_cfg) /*48*/ > >> is expected. > >> > >> memset(ha->fcp_prio_cfg, 0, FCP_PRIO_CFG_SIZE); > >> > >> --- > >> drivers/scsi/qla2xxx/qla_fw.h | 3 ++- > >> drivers/scsi/qla2xxx/qla_os.c | 1 + > >> 2 files changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/scsi/qla2xxx/qla_fw.h b/drivers/scsi/qla2xxx/qla_fw.h > >> index f9bad5bd7198..647e67c6ba5e 100644 > >> --- a/drivers/scsi/qla2xxx/qla_fw.h > >> +++ b/drivers/scsi/qla2xxx/qla_fw.h > >> @@ -2217,8 +2217,9 @@ struct qla_fcp_prio_cfg { > >> #define FCP_PRIO_ATTR_PERSIST 0x2 > >> uint8_t reserved; /* Reserved for future use */ > >> #define FCP_PRIO_CFG_HDR_SIZE 0x10 > >> - struct qla_fcp_prio_entry entry[1]; /* fcp priority entries */ > >> + struct qla_fcp_prio_entry entry[1023]; /* fcp priority entries */ > >> #define FCP_PRIO_CFG_ENTRY_SIZE 0x20 > >> + uint8_t reserved2[16]; > >> }; > >> > >> #define FCP_PRIO_CFG_SIZE (32*1024) /* fcp prio data per port*/ > > > > A new constant may be introduced to define size of qla_fcp_prio_entry. > > That would let to drop the magic 32 number here and allow to add one > > more BUILD_BUG_ON for sizeof(struct qla_fcp_prio_entry). > > The constant FCP_PRIO_CFG_ENTRY_SIZE is only used once, namely in the > following code: > > len = ha->fcp_prio_cfg->num_entries * FCP_PRIO_CFG_ENTRY_SIZE; > > How about removing the definition of FCP_PRIO_CFG_ENTRY_SIZE and > changing FCP_PRIO_CFG_ENTRY_SIZE in the above calculation into > sizeof(struct qla_fcp_prio_entry)? > Hi Bart, I overlooked the FCP_PRIO_CFG_ENTRY_SIZE definition in the hunk :) Since it's already there, we can just replace 32 with it and add BUILD_BUG_ON using the definition. What you propose is also reasonable if used with BUILD_BUG_ON that has numeric literal for FCP_PRIO_CFG_ENTRY_SIZE. Thanks, Roman
On Mon, Apr 13, 2020 at 10:24:37PM -0400, Martin K. Petersen wrote: > > Roman, > > >> - struct qla_fcp_prio_entry entry[1]; /* fcp priority entries */ > >> + struct qla_fcp_prio_entry entry[1023]; /* fcp priority entries */ > >> #define FCP_PRIO_CFG_ENTRY_SIZE 0x20 > >> + uint8_t reserved2[16]; > >> }; > >> > >> #define FCP_PRIO_CFG_SIZE (32*1024) /* fcp prio data per port*/ > > > > A new constant may be introduced to define size of qla_fcp_prio_entry. > > That would let to drop the magic 32 number here and allow to add one > > more BUILD_BUG_ON for sizeof(struct qla_fcp_prio_entry). > > I wonder what the firmware interface says about the runt entry? > Hi Martin, NVRAM and Option ROM contents/layout doesn't seem to be a part of the FW spec. Roman
diff --git a/drivers/scsi/qla2xxx/qla_fw.h b/drivers/scsi/qla2xxx/qla_fw.h index f9bad5bd7198..647e67c6ba5e 100644 --- a/drivers/scsi/qla2xxx/qla_fw.h +++ b/drivers/scsi/qla2xxx/qla_fw.h @@ -2217,8 +2217,9 @@ struct qla_fcp_prio_cfg { #define FCP_PRIO_ATTR_PERSIST 0x2 uint8_t reserved; /* Reserved for future use */ #define FCP_PRIO_CFG_HDR_SIZE 0x10 - struct qla_fcp_prio_entry entry[1]; /* fcp priority entries */ + struct qla_fcp_prio_entry entry[1023]; /* fcp priority entries */ #define FCP_PRIO_CFG_ENTRY_SIZE 0x20 + uint8_t reserved2[16]; }; #define FCP_PRIO_CFG_SIZE (32*1024) /* fcp prio data per port*/ diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c index d9072ea7c42b..784f3e553f15 100644 --- a/drivers/scsi/qla2xxx/qla_os.c +++ b/drivers/scsi/qla2xxx/qla_os.c @@ -7840,6 +7840,7 @@ qla2x00_module_init(void) BUILD_BUG_ON(sizeof(struct vf_evfp_entry_24xx) != 56); BUILD_BUG_ON(sizeof(struct qla_flt_region) != 16); BUILD_BUG_ON(sizeof(struct qla_flt_header) != 8); + BUILD_BUG_ON(sizeof(struct qla_fcp_prio_cfg) != FCP_PRIO_CFG_SIZE); /* Allocate cache for SRBs. */ srb_cachep = kmem_cache_create("qla2xxx_srbs", sizeof(srb_t), 0,
This patch fixes the following Coverity complaint without changing any functionality: CID 337793 (#1 of 1): Wrong size argument (SIZEOF_MISMATCH) suspicious_sizeof: Passing argument ha->fcp_prio_cfg of type struct qla_fcp_prio_cfg * and argument 32768UL to function memset is suspicious because a multiple of sizeof (struct qla_fcp_prio_cfg) /*48*/ is expected. memset(ha->fcp_prio_cfg, 0, FCP_PRIO_CFG_SIZE); Cc: Nilesh Javali <njavali@marvell.com> Cc: Himanshu Madhani <hmadhani@marvell.com> Cc: Quinn Tran <qutran@marvell.com> Cc: Martin Wilck <mwilck@suse.com> Cc: Daniel Wagner <dwagner@suse.de> Cc: Roman Bolshakov <r.bolshakov@yadro.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/qla2xxx/qla_fw.h | 3 ++- drivers/scsi/qla2xxx/qla_os.c | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)