lkml.org 
[lkml]   [2021]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH][next] scsi: Replace one-element arrays with flexible-array members
On Thu, Dec 02, 2021 at 11:23:26AM -0800, Kees Cook wrote:
> On Fri, Nov 05, 2021 at 12:24:13PM +0100, Steffen Maier wrote:
> > On 11/5/21 10:11, Gustavo A. R. Silva wrote:
> > > Use flexible-array members in struct fc_fdmi_attr_entry and
> > > fs_fdmi_attrs instead of one-element arrays, and refactor the
> > > code accordingly.
> > >
> > > Also, turn the one-element array _port_ in struct fc_fdmi_rpl
> > > into a simple object of type struct fc_fdmi_port_name, as it
> > > seems there is no more than just one port expected:
> > >
> > > $ git grep -nw numport drivers/scsi/
> > > drivers/scsi/csiostor/csio_lnode.c:447: reg_pl->numport = htonl(1);
> > > drivers/scsi/libfc/fc_encode.h:232: put_unaligned_be32(1, &ct->payload.rhba.port.numport);
> > >
> > > Also, this helps with the ongoing efforts to globally enable
> > > -Warray-bounds and get us closer to being able to tighten the
> > > FORTIFY_SOURCE routines on memcpy().
> > >
> > > https://github.com/KSPP/linux/issues/79
> > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > > ---
> > > drivers/scsi/csiostor/csio_lnode.c | 2 +-
> > > drivers/scsi/libfc/fc_encode.h | 4 ++--
> > > include/scsi/fc/fc_ms.h | 6 +++---
> > > 3 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/scsi/csiostor/csio_lnode.c b/drivers/scsi/csiostor/csio_lnode.c
> > > index d5ac93897023..cf9dd79ee488 100644
> > > --- a/drivers/scsi/csiostor/csio_lnode.c
> > > +++ b/drivers/scsi/csiostor/csio_lnode.c
> > > @@ -445,7 +445,7 @@ csio_ln_fdmi_dprt_cbfn(struct csio_hw *hw, struct csio_ioreq *fdmi_req)
> > > /* Register one port per hba */
> > > reg_pl = (struct fc_fdmi_rpl *)pld;
> > > reg_pl->numport = htonl(1);
> > > - memcpy(&reg_pl->port[0].portname, csio_ln_wwpn(ln), 8);
> > > + memcpy(&reg_pl->port.portname, csio_ln_wwpn(ln), 8);
> > > pld += sizeof(*reg_pl);
> > >
> > > /* Start appending HBA attributes hba */
> > > diff --git a/drivers/scsi/libfc/fc_encode.h b/drivers/scsi/libfc/fc_encode.h
> > > index 74ae7fd15d8d..5806f99e4061 100644
> > > --- a/drivers/scsi/libfc/fc_encode.h
> > > +++ b/drivers/scsi/libfc/fc_encode.h
> > > @@ -232,7 +232,7 @@ static inline int fc_ct_ms_fill(struct fc_lport *lport,
> > > put_unaligned_be32(1, &ct->payload.rhba.port.numport);
> > > /* Port Name */
> > > put_unaligned_be64(lport->wwpn,
> > > - &ct->payload.rhba.port.port[0].portname);
> > > + &ct->payload.rhba.port.port.portname);
> > >
> > > /* HBA Attributes */
> > > put_unaligned_be32(numattrs,
> >
> > > diff --git a/include/scsi/fc/fc_ms.h b/include/scsi/fc/fc_ms.h
> > > index 00191695233a..44fbe84fa664 100644
> > > --- a/include/scsi/fc/fc_ms.h
> > > +++ b/include/scsi/fc/fc_ms.h
> >
> > > @@ -174,7 +174,7 @@ struct fs_fdmi_attrs {
> >
> > /*
> > * Registered Port List
> >
> > > */
> > > struct fc_fdmi_rpl {
> > > __be32 numport;
> > > - struct fc_fdmi_port_name port[1];
> > > + struct fc_fdmi_port_name port;
> > > } __attribute__((__packed__));
> >
> > While I'm not affected by the change, it feels to me as if these are
> > protocol definitions originating in a T11 Fibre Channel standard FC-GS. It's
> > a port *list*. Can you "modify" the standard here?
> >
> > The fact, that currently existing code users only ever seem to use one
> > single port in the list, would be an independent thing to me.
>
> There are three changes made here, and I suspect it might make sense to
> split them up.
>
> In a quick look, I see "struct fc_fdmi_attr_entry" has a sizeof() call
> against it, so it's not clear if it's safe to switch it to a flexible
> array without other changes.

I'll take a look.

> The change to struct fs_fdmi_attrs looks okay, since it appears to be
> used only in casts, but it might make sense to use diffoscope on the
> changed .o files to validate nothing weird has happened.

OK.

> For struct fc_dmi_rpl, as long as "numport" is always set/validated to
> 1, I think this change is fine.

OK. I'll split this up into a small series and keep

struct fc_fdmi_port_name port[1];

as a one-element array.

Thanks
--
Gustavo

\
 
 \ /
  Last update: 2021-12-02 20:35    [W:0.054 / U:0.652 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site