| Message ID | 20190722062231.115865-3-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fcoe: cleanup fc_rport_priv usage | expand |
On Mon, Jul 22, 2019 at 08:22:30AM +0200, Hannes Reinecke wrote: > Gcc-9 complains for a memset across pointer boundaries, which happens > as the code tries to allocate a flexible array on the stack. > Turns out we cannot do this without relying on gcc-isms, so > with this patch we'll embed the fc_rport_priv structure into > fcoe_rport, can use the normal 'container_of' outcast, and > will only have to do a memset over one structure. This looks mostly good, but: I think the subject and changelog are a bit odd. What you do here is to change that way how the private data is allocated by embeddeding the fc_rport_priv structure into the private data, so that should be the focus of the description. That this was triggered by the memset warning might be worth mentioning, but probably only after explaining what you did. > @@ -2738,10 +2736,7 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) > { > struct fip_header *fiph; > enum fip_vn2vn_subcode sub; > - struct { > - struct fc_rport_priv rdata; > - struct fcoe_rport frport; > - } buf; > + struct fcoe_rport buf; Wouldn't rport or frport be a better name for this variable? > fiph = (struct fip_header *)skb->data; > @@ -2757,7 +2752,8 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) > goto drop; > } > > - rc = fcoe_ctlr_vn_parse(fip, skb, &buf.rdata); > + memset(&buf, 0, sizeof(buf)); Instead of the memset you could do an initialization at declaration time: struct fcoe_rport rport = { }; > - struct { > - struct fc_rport_priv rdata; > - struct fcoe_rport frport; > - } buf; > + struct fcoe_rport buf; > int rc; > > fiph = (struct fip_header *)skb->data; > sub = fiph->fip_subcode; > - rc = fcoe_ctlr_vlan_parse(fip, skb, &buf.rdata); > + memset(&buf, 0, sizeof(buf)); Same two comments apply here.
On 2019-07-22 7:50 a.m., Christoph Hellwig wrote: > On Mon, Jul 22, 2019 at 08:22:30AM +0200, Hannes Reinecke wrote: >> Gcc-9 complains for a memset across pointer boundaries, which happens >> as the code tries to allocate a flexible array on the stack. >> Turns out we cannot do this without relying on gcc-isms, so >> with this patch we'll embed the fc_rport_priv structure into >> fcoe_rport, can use the normal 'container_of' outcast, and >> will only have to do a memset over one structure. > > This looks mostly good, but: > > I think the subject and changelog are a bit odd. What you do here > is to change that way how the private data is allocated by embeddeding > the fc_rport_priv structure into the private data, so that should be > the focus of the description. That this was triggered by the memset > warning might be worth mentioning, but probably only after explaining > what you did. > >> @@ -2738,10 +2736,7 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) >> { >> struct fip_header *fiph; >> enum fip_vn2vn_subcode sub; >> - struct { >> - struct fc_rport_priv rdata; >> - struct fcoe_rport frport; >> - } buf; >> + struct fcoe_rport buf; > > Wouldn't rport or frport be a better name for this variable? > >> fiph = (struct fip_header *)skb->data; >> @@ -2757,7 +2752,8 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) >> goto drop; >> } >> >> - rc = fcoe_ctlr_vn_parse(fip, skb, &buf.rdata); >> + memset(&buf, 0, sizeof(buf)); > > Instead of the memset you could do an initialization at declaration > time: > > struct fcoe_rport rport = { }; https://en.cppreference.com/w/c/language/struct_initialization "When initializing an object of struct or union type, the initializer must be a non-empty, brace-enclosed, comma-separated list of initializers for the members:" Hmmm, "non-empty", is that a GNU extension? However it is good C++11, so if that is where we a moving, great. Doug Gilbert >> - struct { >> - struct fc_rport_priv rdata; >> - struct fcoe_rport frport; >> - } buf; >> + struct fcoe_rport buf; >> int rc; >> >> fiph = (struct fip_header *)skb->data; >> sub = fiph->fip_subcode; >> - rc = fcoe_ctlr_vlan_parse(fip, skb, &buf.rdata); >> + memset(&buf, 0, sizeof(buf)); > > Same two comments apply here. >
On 7/22/19 1:50 PM, Christoph Hellwig wrote: > On Mon, Jul 22, 2019 at 08:22:30AM +0200, Hannes Reinecke wrote: >> Gcc-9 complains for a memset across pointer boundaries, which happens >> as the code tries to allocate a flexible array on the stack. >> Turns out we cannot do this without relying on gcc-isms, so >> with this patch we'll embed the fc_rport_priv structure into >> fcoe_rport, can use the normal 'container_of' outcast, and >> will only have to do a memset over one structure. > > This looks mostly good, but: > > I think the subject and changelog are a bit odd. What you do here > is to change that way how the private data is allocated by embeddeding > the fc_rport_priv structure into the private data, so that should be > the focus of the description. That this was triggered by the memset > warning might be worth mentioning, but probably only after explaining > what you did. > Yeah, that was carried over from the original patch. Will be updating it. >> @@ -2738,10 +2736,7 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) >> { >> struct fip_header *fiph; >> enum fip_vn2vn_subcode sub; >> - struct { >> - struct fc_rport_priv rdata; >> - struct fcoe_rport frport; >> - } buf; >> + struct fcoe_rport buf; > > Wouldn't rport or frport be a better name for this variable? > Possibly ... Keeping it at 'buf' introduced less churn, but by all means... >> fiph = (struct fip_header *)skb->data; >> @@ -2757,7 +2752,8 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) >> goto drop; >> } >> >> - rc = fcoe_ctlr_vn_parse(fip, skb, &buf.rdata); >> + memset(&buf, 0, sizeof(buf)); > > Instead of the memset you could do an initialization at declaration > time: > > struct fcoe_rport rport = { }; > Would probably optimized to the same assembler code by the compiler, but okay. Will be reposting. Cheers, Hannes
On Mon, Jul 22, 2019 at 02:50:50PM -0400, Douglas Gilbert wrote:
> Hmmm, "non-empty", is that a GNU extension?
Yes, empty initializers are a GNU extension. One that is pretty heavily
used in the kernel.
diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c index 0d7770d07405..728ff37111ed 100644 --- a/drivers/scsi/fcoe/fcoe_ctlr.c +++ b/drivers/scsi/fcoe/fcoe_ctlr.c @@ -2005,7 +2005,7 @@ EXPORT_SYMBOL_GPL(fcoe_wwn_from_mac); */ static inline struct fcoe_rport *fcoe_ctlr_rport(struct fc_rport_priv *rdata) { - return (struct fcoe_rport *)(rdata + 1); + return container_of(rdata, struct fcoe_rport, rdata); } /** @@ -2269,7 +2269,7 @@ static void fcoe_ctlr_vn_start(struct fcoe_ctlr *fip) */ static int fcoe_ctlr_vn_parse(struct fcoe_ctlr *fip, struct sk_buff *skb, - struct fc_rport_priv *rdata) + struct fcoe_rport *frport) { struct fip_header *fiph; struct fip_desc *desc = NULL; @@ -2277,16 +2277,12 @@ static int fcoe_ctlr_vn_parse(struct fcoe_ctlr *fip, struct fip_wwn_desc *wwn = NULL; struct fip_vn_desc *vn = NULL; struct fip_size_desc *size = NULL; - struct fcoe_rport *frport; size_t rlen; size_t dlen; u32 desc_mask = 0; u32 dtype; u8 sub; - memset(rdata, 0, sizeof(*rdata) + sizeof(*frport)); - frport = fcoe_ctlr_rport(rdata); - fiph = (struct fip_header *)skb->data; frport->flags = ntohs(fiph->fip_flags); @@ -2349,15 +2345,17 @@ static int fcoe_ctlr_vn_parse(struct fcoe_ctlr *fip, if (dlen != sizeof(struct fip_wwn_desc)) goto len_err; wwn = (struct fip_wwn_desc *)desc; - rdata->ids.node_name = get_unaligned_be64(&wwn->fd_wwn); + frport->rdata.ids.node_name = + get_unaligned_be64(&wwn->fd_wwn); break; case FIP_DT_VN_ID: if (dlen != sizeof(struct fip_vn_desc)) goto len_err; vn = (struct fip_vn_desc *)desc; memcpy(frport->vn_mac, vn->fd_mac, ETH_ALEN); - rdata->ids.port_id = ntoh24(vn->fd_fc_id); - rdata->ids.port_name = get_unaligned_be64(&vn->fd_wwpn); + frport->rdata.ids.port_id = ntoh24(vn->fd_fc_id); + frport->rdata.ids.port_name = + get_unaligned_be64(&vn->fd_wwpn); break; case FIP_DT_FC4F: if (dlen != sizeof(struct fip_fc4_feat)) @@ -2738,10 +2736,7 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) { struct fip_header *fiph; enum fip_vn2vn_subcode sub; - struct { - struct fc_rport_priv rdata; - struct fcoe_rport frport; - } buf; + struct fcoe_rport buf; int rc, vlan_id = 0; fiph = (struct fip_header *)skb->data; @@ -2757,7 +2752,8 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) goto drop; } - rc = fcoe_ctlr_vn_parse(fip, skb, &buf.rdata); + memset(&buf, 0, sizeof(buf)); + rc = fcoe_ctlr_vn_parse(fip, skb, &buf); if (rc) { LIBFCOE_FIP_DBG(fip, "vn_recv vn_parse error %d\n", rc); goto drop; @@ -2802,22 +2798,18 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) */ static int fcoe_ctlr_vlan_parse(struct fcoe_ctlr *fip, struct sk_buff *skb, - struct fc_rport_priv *rdata) + struct fcoe_rport *frport) { struct fip_header *fiph; struct fip_desc *desc = NULL; struct fip_mac_desc *macd = NULL; struct fip_wwn_desc *wwn = NULL; - struct fcoe_rport *frport; size_t rlen; size_t dlen; u32 desc_mask = 0; u32 dtype; u8 sub; - memset(rdata, 0, sizeof(*rdata) + sizeof(*frport)); - frport = fcoe_ctlr_rport(rdata); - fiph = (struct fip_header *)skb->data; frport->flags = ntohs(fiph->fip_flags); @@ -2871,7 +2863,8 @@ static int fcoe_ctlr_vlan_parse(struct fcoe_ctlr *fip, if (dlen != sizeof(struct fip_wwn_desc)) goto len_err; wwn = (struct fip_wwn_desc *)desc; - rdata->ids.node_name = get_unaligned_be64(&wwn->fd_wwn); + frport->rdata.ids.node_name = + get_unaligned_be64(&wwn->fd_wwn); break; default: LIBFCOE_FIP_DBG(fip, "unexpected descriptor type %x " @@ -2982,15 +2975,13 @@ static int fcoe_ctlr_vlan_recv(struct fcoe_ctlr *fip, struct sk_buff *skb) { struct fip_header *fiph; enum fip_vlan_subcode sub; - struct { - struct fc_rport_priv rdata; - struct fcoe_rport frport; - } buf; + struct fcoe_rport buf; int rc; fiph = (struct fip_header *)skb->data; sub = fiph->fip_subcode; - rc = fcoe_ctlr_vlan_parse(fip, skb, &buf.rdata); + memset(&buf, 0, sizeof(buf)); + rc = fcoe_ctlr_vlan_parse(fip, skb, &buf); if (rc) { LIBFCOE_FIP_DBG(fip, "vlan_recv vlan_parse error %d\n", rc); goto drop; diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c index 0da34c7a6866..45d44c50a620 100644 --- a/drivers/scsi/libfc/fc_rport.c +++ b/drivers/scsi/libfc/fc_rport.c @@ -128,6 +128,7 @@ EXPORT_SYMBOL(fc_rport_lookup); struct fc_rport_priv *fc_rport_create(struct fc_lport *lport, u32 port_id) { struct fc_rport_priv *rdata; + size_t rport_priv_size = sizeof(*rdata); lockdep_assert_held(&lport->disc.disc_mutex); @@ -135,7 +136,9 @@ struct fc_rport_priv *fc_rport_create(struct fc_lport *lport, u32 port_id) if (rdata) return rdata; - rdata = kzalloc(sizeof(*rdata) + lport->rport_priv_size, GFP_KERNEL); + if (lport->rport_priv_size > 0) + rport_priv_size = lport->rport_priv_size; + rdata = kzalloc(rport_priv_size, GFP_KERNEL); if (!rdata) return NULL; diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h index ecf3e5978166..138fb8fba748 100644 --- a/include/scsi/libfcoe.h +++ b/include/scsi/libfcoe.h @@ -229,6 +229,7 @@ struct fcoe_fcf { * @vn_mac: VN_Node assigned MAC address for data */ struct fcoe_rport { + struct fc_rport_priv rdata; unsigned long time; u16 fcoe_len; u16 flags;
Gcc-9 complains for a memset across pointer boundaries, which happens as the code tries to allocate a flexible array on the stack. Turns out we cannot do this without relying on gcc-isms, so with this patch we'll embed the fc_rport_priv structure into fcoe_rport, can use the normal 'container_of' outcast, and will only have to do a memset over one structure. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/scsi/fcoe/fcoe_ctlr.c | 41 ++++++++++++++++------------------------- drivers/scsi/libfc/fc_rport.c | 5 ++++- include/scsi/libfcoe.h | 1 + 3 files changed, 21 insertions(+), 26 deletions(-)