| Message ID | 20220915095757.2861822-2-daniel.machon@microchip.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add PCP selector and new APPTRUST attribute | expand |
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.
Den Mon, Sep 19, 2022 at 11:45:41AM +0200 skrev Petr Machata: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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. The pcp selector will have to fit in 5 bits (0x1f instead of 0xff) to not interfere with the priority in lldapd. Thoughts? / Daniel
<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.
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.
Add new PCP selector for the 8021Qaz APP managed object. The purpose of adding the PCP selector, is to be able to offload PCP-based queue classification to the 8021Q Priority Code Point table, see 6.9.3 of IEEE Std 802.1Q-2018. PCP and DEI is encoded in the protocol field as 8*dei+pcp, so that a mapping of PCP 2 and DEI 1 to priority 3 is encoded as {255, 10, 3}. While PCP is not a standard 8021Qaz selector, it seems very convenient to add it to the APP object, as this is where similar priority mapping is handled, and it perfectly fits the {selector, protocol, priority} triplet. Signed-off-by: Daniel Machon <daniel.machon@microchip.com> --- include/uapi/linux/dcbnl.h | 1 + 1 file changed, 1 insertion(+)