On 6/3/19 11:20 PM, Linus Torvalds wrote:
> So gcc-9 has a new warning about doing memset() across pointer boundaries.
>
> We didn't have a lot of these things, and most of them got fixed
> pretty quickly. The main remaining ones are oin FCoE, and look like
> this:
>
> In function ‘memset’,
> inlined from ‘fcoe_ctlr_vlan_parse’ at drivers/scsi/fcoe/fcoe_ctlr.c:2830:2,
> inlined from ‘fcoe_ctlr_vlan_recv’ at drivers/scsi/fcoe/fcoe_ctlr.c:3005:7:
> ./include/linux/string.h:344:9: warning: ‘__builtin_memset’ offset
> [569, 600] from the object at ‘buf’ is out of the bounds of referenced
> subobject ‘rdata’ with type ‘struct fc_rport_priv’ at offset 0
> [-Warray-bounds]
> 344 | return __builtin_memset(p, c, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In function ‘memset’,
> inlined from ‘fcoe_ctlr_vn_parse’ at drivers/scsi/fcoe/fcoe_ctlr.c:2299:2,
> inlined from ‘fcoe_ctlr_vn_recv’ at drivers/scsi/fcoe/fcoe_ctlr.c:2772:7:
> ./include/linux/string.h:344:9: warning: ‘__builtin_memset’ offset
> [569, 600] from the object at ‘buf’ is out of the bounds of referenced
> subobject ‘rdata’ with type ‘struct fc_rport_priv’ at offset 0
> [-Warray-bounds]
> 344 | return __builtin_memset(p, c, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> and honestly, when I look at the code, I cannot help but agree with
> the new warning in this case (we had a few other cases where the
> warning was understandable but annoying, but in the FCoE case it's
> really "yeah, that code is wrong").
>
> In particular, in fcoe_ctlr_vlan_parse(), the function is passed a
> "struct fc_rport_priv *rdata" pointer, and then it does
>
> memset(rdata, 0, sizeof(*rdata) + sizeof(*frport));
>
> and honestly, that should make people go "WTF?". You got passed a
> pointer to one type, and then you clear not just that type, but
> another entirely unrelated type after it. That's just completely bogus
> and wrong.
>
> In fact, what people *are* passing that thing is this:
>
> struct {
> struct fc_rport_priv rdata;
> struct fcoe_rport frport;
> } buf;
>
> but that doesn't actually make that "memset()" any more correct. It's
> still entirely wrong, because it's possible that "struct fcoe_rport"
> could have different alignment requirements etc, so maybe there's a
> gap between the two structures, and the memset() with just adding the
> sizes may be entirely bogus.
>
> Now, in practice I think the code works, but I have to say that in
> this case the compiler warning is really quite correct. The code is
> wrong, even if it might happen to work.
>
> That "combined struct of two types" needs to be made into a type of
> its own, and people need to use that.
>
> I started doing that, but it turns out this mis-feature is deeply
> embedded in that file, and also exposed indirectly in the whole
> "struct fc_rport_operations", which uses a "event_callback" function
> pointer that has the bogus type.
>
> End result: I don't know how to fix it, but the compiler is right.
> This code is wrong, and it needs to be fixed. Maybe "struct
> fc_rport_priv" could just be made to include that "struct fcoe_rport"
> as part of it? Maybe the event_callback() can be typed differently.
> But passing around a "struct fc_rport_priv" pointer that is actually
> *not* a pointer to just that thing, but must be a pointer to the
> combined data, is wrong. Something like
>
> struct fc_rport_combined_data {
> struct fc_rport_priv rdata;
> struct fcoe_rport frport;
> };
>
> may be needed, and then you pass a pointer to that. But it expands
> quite quickly because this seems to be common.
>
> Related to this thing, the whole
>
> static inline struct fcoe_rport *fcoe_ctlr_rport(struct fc_rport_priv *rdata)
>
> is part of the disease, and needs to go away. Passing the right
> "combined data" pointer around would have made that thing useless to
> begin with (ie "fcoe_ctlr_rport(rdata)" should become just
> "&buf->frport" instead if the types were done correctly).
>
> Hmm? Please can some FCoE person look at this, because right now it
> causes tens of lines of warnings that make me go "yeah, the compiler
> is right".
>
To hear is to obey :-)
I'll have a look.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
