On 6/4/19 7:16 PM, Linus Torvalds wrote:
> On Mon, Jun 3, 2019 at 2:20 PM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> In fact, what people *are* passing that thing is this:
>>
>> struct {
>> struct fc_rport_priv rdata;
>> struct fcoe_rport frport;
>> } buf;
>
> It is in fact worse than that.
>
> Yes, _some_ people pass that combined struct.
>
> But a lot of people pass around a fc_rport_priv that has just been
> allocated with
>
> rdata = kzalloc(sizeof(*rdata) + lport->rport_priv_size, GFP_KERNEL);
>
> in fc_rport_create().
>
> They end up being somewhat equivalent as long as the alignments of
> those sub-structures match, but it does mean that the type is really a
> bit fluid, and it ends up leaking outside of that fcoe_ctlr.c file
> into various other helper functions like that allocation function.
>
> It really looks fairly nasty to change/fix. The code really is passing
> around a 'struct fc_rport_priv' pointer, but that that 'fc_rport_priv'
> type is then associated with the added 'struct fcoe_rport' at the end,
> in a way that is *not* visible to the type system.
>
> So no, it doesn't work to create a new type like
>
> struct fc_rport_combined_data {
>
> because it ends up affecting almost *everything* that works with that
> 'rdata' thing.
>
> I get the feeling that 'struct fc_rport_priv' should just be extended
> to have 'struct fcoe_rport' at the end, but maybe that's not always
> true? It looks like that is only used for FIP_MODE_VN2VN, adn then we
> have some other mode that doesn't have that allocation at all?
>
> So the code seems to be a mix of dynamic typing ("no fcoe_rport at the
> end") and static typing that just assumes that the allocation always
> has the fcoe_rport afterwards.
>
> Would it be ok to make 'struct fc_rport_priv' just always contain that
> fcoe_rport? Getting rid of the "lport->rport_priv_size" thing
> entirely, and just have the type set to the bigger case
> unconditionally?
>
(Not sure if you've seen my patch, but:)
Using that approach would break the attempted layering (fcoe is meant to
be a sub-class of libfc, and as such non of the fcoe structures should
be visible in libfc).
However, I had been thinking of reverting the model with the combined
structures into the 'normal' embedded usage, ie embed fc_rport_priv as
the first element in fcoe_rport. That way we can safely reference switch
back and forth between both types with an outcast, and we should be
alignment safe.
Will be drafting up a patch.
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)
