| Message ID | ZG0fDdY/PPQ/ijlt@work (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,next] scsi: lpfc: Use struct_size() helper | expand |
no_of_objects may be hardcoded to 1 right now, but does it make more sense to use? struct_size(rap, obj, be32_to_cpu(rap->no_of_objects)); We probably should have declared no_of_objects as __be32 to have avoided this confusion. On Tue, May 23, 2023 at 1:33 PM Gustavo A. R. Silva <gustavoars@kernel.org> 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> > --- > Changes in v2: > - Use literal 1 in call to struct_size(), instead of rap->no_of_objects > (Kees Cook). > > v1: > - Link: https://lore.kernel.org/linux-hardening/99e06733f5f35c6cd62e05f530b93107bfd03362.1684358315.git.gustavoars@kernel.org/ > > drivers/scsi/lpfc/lpfc_ct.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c > index e880d127d7f5..f52aeb73af8d 100644 > --- a/drivers/scsi/lpfc/lpfc_ct.c > +++ b/drivers/scsi/lpfc/lpfc_ct.c > @@ -3747,9 +3747,7 @@ lpfc_vmid_cmd(struct lpfc_vport *vport, > rap->no_of_objects = cpu_to_be32(1); > rap->obj[0].entity_id_len = vmid->vmid_len; > memcpy(rap->obj[0].entity_id, vmid->host_vmid, vmid->vmid_len); > - size = RAPP_IDENT_OFFSET + > - sizeof(struct lpfc_vmid_rapp_ident_list) + > - sizeof(struct entity_id_object); > + size = RAPP_IDENT_OFFSET + struct_size(rap, obj, 1); > retry = 1; > break; > > @@ -3767,9 +3765,7 @@ lpfc_vmid_cmd(struct lpfc_vport *vport, > dap->no_of_objects = cpu_to_be32(1); > dap->obj[0].entity_id_len = vmid->vmid_len; > memcpy(dap->obj[0].entity_id, vmid->host_vmid, vmid->vmid_len); > - size = DAPP_IDENT_OFFSET + > - sizeof(struct lpfc_vmid_dapp_ident_list) + > - sizeof(struct entity_id_object); > + size = DAPP_IDENT_OFFSET + struct_size(dap, obj, 1); > write_lock(&vport->vmid_lock); > vmid->flag &= ~LPFC_VMID_REGISTERED; > write_unlock(&vport->vmid_lock); > -- > 2.34.1 >
On Tue, May 23, 2023 at 10:25:20PM -0700, Justin Tee wrote: > no_of_objects may be hardcoded to 1 right now, but does it make more > sense to use? > > struct_size(rap, obj, be32_to_cpu(rap->no_of_objects)); Oh yeah, that's nicer. :) > We probably should have declared no_of_objects as __be32 to have > avoided this confusion. Perhaps this patch can add that too?
> > We probably should have declared no_of_objects as __be32 to have > > avoided this confusion. > > Perhaps this patch can add that too? Yes, please. Something like this is fine: diff --git a/drivers/scsi/lpfc/lpfc_hw.h b/drivers/scsi/lpfc/lpfc_hw.h index 19b2d2754f32..e5a639d96122 100644 --- a/drivers/scsi/lpfc/lpfc_hw.h +++ b/drivers/scsi/lpfc/lpfc_hw.h @@ -1414,12 +1414,12 @@ struct app_id_object { }; struct lpfc_vmid_rapp_ident_list { - uint32_t no_of_objects; + __be32 no_of_objects; struct entity_id_object obj[1]; }; struct lpfc_vmid_dapp_ident_list { - uint32_t no_of_objects; + __be32 no_of_objects; struct entity_id_object obj[1]; };
Gustavo/Justin, >> > We probably should have declared no_of_objects as __be32 to have >> > avoided this confusion. >> >> Perhaps this patch can add that too? > > Yes, please. Something like this is fine: Please send a formal patch. Thanks!
Hi Martin, Sure, will do. I'll post the formal patch shortly. The __be32 no_of_objects comment can be ignored for this patch as I've already incorporated that into another patch with subject line: [PATCH 1/1] lpfc: Fix incorrect big endian type assignments in FDMI and VMID paths Thanks, Justin On Wed, May 31, 2023 at 8:27 AM Martin K. Petersen <martin.petersen@oracle.com> wrote: > > > Gustavo/Justin, > > >> > We probably should have declared no_of_objects as __be32 to have > >> > avoided this confusion. > >> > >> Perhaps this patch can add that too? > > > > Yes, please. Something like this is fine: > > Please send a formal patch. Thanks! > > -- > Martin K. Petersen Oracle Linux Engineering
diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c index e880d127d7f5..f52aeb73af8d 100644 --- a/drivers/scsi/lpfc/lpfc_ct.c +++ b/drivers/scsi/lpfc/lpfc_ct.c @@ -3747,9 +3747,7 @@ lpfc_vmid_cmd(struct lpfc_vport *vport, rap->no_of_objects = cpu_to_be32(1); rap->obj[0].entity_id_len = vmid->vmid_len; memcpy(rap->obj[0].entity_id, vmid->host_vmid, vmid->vmid_len); - size = RAPP_IDENT_OFFSET + - sizeof(struct lpfc_vmid_rapp_ident_list) + - sizeof(struct entity_id_object); + size = RAPP_IDENT_OFFSET + struct_size(rap, obj, 1); retry = 1; break; @@ -3767,9 +3765,7 @@ lpfc_vmid_cmd(struct lpfc_vport *vport, dap->no_of_objects = cpu_to_be32(1); dap->obj[0].entity_id_len = vmid->vmid_len; memcpy(dap->obj[0].entity_id, vmid->host_vmid, vmid->vmid_len); - size = DAPP_IDENT_OFFSET + - sizeof(struct lpfc_vmid_dapp_ident_list) + - sizeof(struct entity_id_object); + size = DAPP_IDENT_OFFSET + struct_size(dap, obj, 1); write_lock(&vport->vmid_lock); vmid->flag &= ~LPFC_VMID_REGISTERED; write_unlock(&vport->vmid_lock);
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> --- Changes in v2: - Use literal 1 in call to struct_size(), instead of rap->no_of_objects (Kees Cook). v1: - Link: https://lore.kernel.org/linux-hardening/99e06733f5f35c6cd62e05f530b93107bfd03362.1684358315.git.gustavoars@kernel.org/ drivers/scsi/lpfc/lpfc_ct.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)