Re: [RFC PATCH v2 net-next 1/2] net: dcb: add new pcp selector to app object

[Petr Machata <petrm@nvidia.com>]

<Daniel.Machon@microchip.com> writes:

> Den Mon, Sep 19, 2022 at 11:45:41AM +0200 skrev Petr Machata:
>> 
>> Daniel Machon <daniel.machon@microchip.com> writes:
>> 
>> > diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
>> > index a791a94013a6..8eab16e5bc13 100644
>> > --- a/include/uapi/linux/dcbnl.h
>> > +++ b/include/uapi/linux/dcbnl.h
>> > @@ -217,6 +217,7 @@ struct cee_pfc {
>> >  #define IEEE_8021QAZ_APP_SEL_DGRAM   3
>> >  #define IEEE_8021QAZ_APP_SEL_ANY     4
>> >  #define IEEE_8021QAZ_APP_SEL_DSCP       5
>> > +#define IEEE_8021QAZ_APP_SEL_PCP     255
>> >
>> >  /* This structure contains the IEEE 802.1Qaz APP managed object. This
>> >   * object is also used for the CEE std as well.
>> 
>> One more thought: please verify how this behaves with openlldpad.
>> It's a fairly major user of this API.
>> 
>> I guess it is OK if it refuses to run or bails out in face of the PCP
>> APP entries. On its own it will never introduce them, so this clear and
>> noisy diagnostic when a user messes with the system through a different
>> channels is OK IMHO.
>> 
>> But it shouldn't silently reinterpret the 255 to mean something else.
>
> Hi Petr,
>
> Looks like we are in trouble here:
>
> https://github.com/openSUSE/lldpad/blob/master/lldp_8021qaz.c#L911
>
> protocol is shifted and masked with selector to fit in u8. Same u8
> value is being transmitted in the APP TLVs.
>
> A dscp mapping of 10:7 will become (7 << 5) | 5 = e5
> A pcp mapping of 1:1 will become (1 << 5) | ff = ff (always)
>
> Looks like the loop does not even check for DCB_ATTR_IEEE_APP, so putting
> the pcp stuff in a non-standard attribute in the DCB_ATTR_IEEE_APP_TABLE
> wont work either.

Ho hum.

Yeah, they are reconstructing the APP TLV in place. The format is three
bits of priority, two bits reserved, three bits of selector. Hence the
priority << 5.

I guess the question is how far do we go to maintain the exact same
behavior for broken userspace. Attributes exist exactly to make future
extensions possible. If a userspace decides to reinterpret random bytes,
I feel like that's on them. But checking my what-would-Linus-do
wristband, I'm not 100% sure ;)

> The pcp selector will have to fit in 5 bits (0x1f instead of 0xff) to not
> interfere with the priority in lldapd.

Yeah, but then it ends up shifting into the reserved field of the TLV,
which is also a breakage.

Plus, if ever the standard needs more space to support 16 priorities or
16 or 32 selectors, the reserved bits are where they go. So 31 as a
selector value is not far enough from the standard stuff to be safe as
an extension value.

Um, like, I think we are not in the wrong here, and userspace goes above
and beyond to be broken. So adding a new attribute and patching openlldp
to ignore / bounce the non-standard stuff seems OK. Within the new
attribute, we can use a value such as 24, because 24&7 == 0, which is
currently reserved, and IMHO likely to stay that way. So old openlldp on
new Linux with the PCP rules configured would send broken APP TLVs, but
they would be broken in a fairly conspicuous manner.