[RFC PATCH v2 net-next 0/2] Add PCP selector and new APPTRUST attribute

This patch series adds support for offloading PCP-based queue
classification and introduces a new APPTRUST extension attribute to the
8021Qaz APP managed object.  Prior to implemenation, it has been
discussed on the netdev mailing list here:

https://lore.kernel.org/netdev/Yv9VO1DYAxNduw6A@DEN-LT-70577/

In summary: there currently exist no conveinent way to offload per-port
PCP-based queue classification to hardware. Similarly, there is no way
to indicate the notion of trust for APP table selectors. This patch
series addresses both topics.

PCP based queue classification:

	8021Q standardizes the Priority Code Point table (see 6.9.3 of
	IEEE Std 802.1Q-2018).  This patch series makes it possible, to
	offload the PCP classification to said table.  The new PCP
	selector is not a standard part of the APP managed object,
	therefore it has been assigned a value of 255 to avoid any
	clashes with future DCB standard extensions.

Selector trust:

	ASIC's often has the notion of trust DSCP and trust PCP. This
	new object makes it possible to specify a trust order of app
	selectors, which drivers can then react on.

Patch #1 introduces a new PCP selector to the APP object, which makes it
possible to encode PCP and DEI in the app triplet and offload it to the
PCP table of the ASIC.

Patch #2 Introduces the new extension attributes
DCB_ATTR_DCB_APP_TRUST_TABLE and DCB_ATTR_DCB_APP_TRUST. Trusted
selectors are passed in the nested DCB_ATTR_DCB_APP_TRUST_TABLE
attribute, and assembled into an array of selectors:

  u8 selectors[256];

where lower indexes has higher precedence.  In the array, selectors are
stored consecutively, starting from index zero. With a maximum number of
256 unique selectors, the list has the same maximum size.

The userspace part of this will be posted in a separate patch series.

================================================================================

RFC v1:
https://lore.kernel.org/netdev/20220908120442.3069771-1-daniel.machon@microchip.com/

RFC v1 -> RFC v2:
  - Added new nested attribute type DCB_ATTR_DCB_APP_TRUST_TABLE.
  - Renamed attributes from DCB_ATTR_IEEE_* to DCB_ATTR_DCB_*
  - Renamed ieee_set/getapptrust to dcbnl_set/getapptrust
  - Added -EOPNOTSUPP if dcbnl_setapptrust is not set.
  - Added sanitization of selector array, before passing to driver.


Daniel Machon (2):
  net: dcb: add new pcp selector to app object
  net: dcb: add new apptrust attribute

 include/net/dcbnl.h        |  5 +++
 include/uapi/linux/dcbnl.h |  5 +++
 net/dcb/dcbnl.c            | 66 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 73 insertions(+), 3 deletions(-)

--
2.34.1

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(+)

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.

-- 
2.34.1

Add new apptrust extension attributes to the 8021Qaz APP managed
object.

Two new attributes, DCB_ATTR_DCB_APP_TRUST_TABLE and
DCB_ATTR_DCB_APP_TRUST, has been added. Trusted selectors are passed in
the nested attribute (TRUST_TABLE), in order of precedence.

The new attributes are meant to allow drivers, whose hw supports the
notion of trust, to be able to set whether a particular app selector is
to be trusted - and also the order of precedence of selectors.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 include/net/dcbnl.h        |  4 +++
 include/uapi/linux/dcbnl.h |  4 +++
 net/dcb/dcbnl.c            | 64 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h
index 2b2d86fb3131..8841ab6c2de7 100644
--- a/include/net/dcbnl.h
+++ b/include/net/dcbnl.h

@@ -109,6 +109,10 @@ struct dcbnl_rtnl_ops {
 	/* buffer settings */
 	int (*dcbnl_getbuffer)(struct net_device *, struct dcbnl_buffer *);
 	int (*dcbnl_setbuffer)(struct net_device *, struct dcbnl_buffer *);
+
+	/* apptrust */
+	int (*dcbnl_setapptrust)(struct net_device *, u8 *, int);
+	int (*dcbnl_getapptrust)(struct net_device *, u8 *, int *);
 };
 
 #endif /* __NET_DCBNL_H__ */

diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
index 8eab16e5bc13..ede72aefd88f 100644
--- a/include/uapi/linux/dcbnl.h
+++ b/include/uapi/linux/dcbnl.h

@@ -248,6 +248,8 @@ struct dcb_app {
 	__u16	protocol;
 };
 
+#define IEEE_8021QAZ_APP_SEL_MAX 255
+
 /**
  * struct dcb_peer_app_info - APP feature information sent by the peer
  *

@@ -419,6 +421,8 @@ enum ieee_attrs {
 	DCB_ATTR_IEEE_QCN,
 	DCB_ATTR_IEEE_QCN_STATS,
 	DCB_ATTR_DCB_BUFFER,
+	DCB_ATTR_DCB_APP_TRUST_TABLE,
+	DCB_ATTR_DCB_APP_TRUST,
 	__DCB_ATTR_IEEE_MAX
 };
 #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1)

diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index dc4fb699b56c..6f5d2b295d09 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c

@@ -166,6 +166,7 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = {
 	[DCB_ATTR_IEEE_QCN]         = {.len = sizeof(struct ieee_qcn)},
 	[DCB_ATTR_IEEE_QCN_STATS]   = {.len = sizeof(struct ieee_qcn_stats)},
 	[DCB_ATTR_DCB_BUFFER]       = {.len = sizeof(struct dcbnl_buffer)},
+	[DCB_ATTR_DCB_APP_TRUST_TABLE] = {.type = NLA_NESTED},
 };
 
 /* DCB number of traffic classes nested attributes. */

@@ -1030,11 +1031,11 @@ static int dcbnl_build_peer_app(struct net_device *netdev, struct sk_buff* skb,
 /* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */
 static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
 {
-	struct nlattr *ieee, *app;
+	struct nlattr *ieee, *app, *apptrust;
 	struct dcb_app_type *itr;
 	const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops;
 	int dcbx;
-	int err;
+	int err, i;
 
 	if (nla_put_string(skb, DCB_ATTR_IFNAME, netdev->name))
 		return -EMSGSIZE;

@@ -1133,6 +1134,24 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
 	spin_unlock_bh(&dcb_lock);
 	nla_nest_end(skb, app);
 
+	if (ops->dcbnl_getapptrust) {
+		u8 selectors[IEEE_8021QAZ_APP_SEL_MAX + 1] = {0};
+		int nselectors;
+
+		apptrust = nla_nest_start_noflag(skb,
+						 DCB_ATTR_DCB_APP_TRUST_TABLE);
+		if (!app)
+			return -EMSGSIZE;
+
+		err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors);
+		if (err)
+			return -EMSGSIZE;
+
+		for (i = 0; i < nselectors; i++)
+			nla_put_u8(skb, DCB_ATTR_DCB_APP_TRUST, selectors[i]);
+		nla_nest_end(skb, apptrust);
+	}
+
 	/* get peer info if available */
 	if (ops->ieee_peer_getets) {
 		struct ieee_ets ets;

@@ -1427,7 +1446,7 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
 	const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops;
 	struct nlattr *ieee[DCB_ATTR_IEEE_MAX + 1];
 	int prio;
-	int err;
+	int err, i;
 
 	if (!ops)
 		return -EOPNOTSUPP;

@@ -1513,6 +1532,45 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
 		}
 	}
 
+	if (ieee[DCB_ATTR_DCB_APP_TRUST_TABLE]) {
+		u8 selectors[IEEE_8021QAZ_APP_SEL_MAX + 1] = {0};
+		struct nlattr *attr;
+		int nselectors = 0;
+		u8 selector;
+
+		int rem;
+
+		if (!ops->dcbnl_setapptrust) {
+			err = -EOPNOTSUPP;
+			goto err;
+		}
+
+		nla_for_each_nested(attr, ieee[DCB_ATTR_DCB_APP_TRUST_TABLE],
+				    rem) {
+			if (nla_type(attr) != DCB_ATTR_DCB_APP_TRUST ||
+			    nla_len(attr) != 1 ||
+			    nselectors >= sizeof(selectors)) {
+				err = -EINVAL;
+				goto err;
+			}
+
+			selector = nla_get_u8(attr);
+			/* Duplicate selector ? */
+			for (i = 0; i < nselectors; i++) {
+				if (selectors[i] == selector) {
+					err = -EINVAL;
+					goto err;
+				}
+			}
+
+			selectors[nselectors++] = selector;
+		}
+
+		err = ops->dcbnl_setapptrust(netdev, selectors, nselectors);
+		if (err)
+			goto err;
+	}
+
 err:
 	err = nla_put_u8(skb, DCB_ATTR_IEEE, err);
 	dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0);

-- 
2.34.1

On Thu, Sep 15, 2022 at 11:57:57AM +0200, Daniel Machon wrote:

+	if (ops->dcbnl_getapptrust) {
+		u8 selectors[IEEE_8021QAZ_APP_SEL_MAX + 1] = {0};
+		int nselectors;
+
+		apptrust = nla_nest_start_noflag(skb,
+						 DCB_ATTR_DCB_APP_TRUST_TABLE);
+		if (!app)
+			return -EMSGSIZE;

Should new code be creating nests without the NLA_F_NESTED flag?

+
+		err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors);
+		if (err)
+			return -EMSGSIZE;
+
+		for (i = 0; i < nselectors; i++)
+			nla_put_u8(skb, DCB_ATTR_DCB_APP_TRUST, selectors[i]);
+		nla_nest_end(skb, apptrust);
+	}

Daniel Machon [off-list ref] writes:

I find it odd that APP_TRUST is at the same level as APP_TRUST_TABLE. I
think it would be better to have a separate enum ala what APP_TABLE has
with enum ieee_attr_app.

Move "err, i" up before dcbx so that it observes the reverse xmass tree
principle, please.

I agree with Vladimir that this should not be _noflag.

RXT this as well, please.

I'm assuming you tested this code, I just wrote it into the email
client, so all guarantees are off :)

+
+			selector = nla_get_u8(attr);
+			/* Duplicate selector ? */
+			for (i = 0; i < nselectors; i++) {
+				if (selectors[i] == selector) {
+					err = -EINVAL;
+					goto err;
+				}
+			}

This should validate the selector values as well IMHO. Maybe something
like this?

			switch (selector) {
			...
			case IEEE_8021QAZ_APP_SEL_DGRAM:
			case IEEE_8021QAZ_APP_SEL_ANY:
			case IEEE_8021QAZ_APP_SEL_DSCP:
			case IEEE_8021QAZ_APP_SEL_PCP:
				break;
			default:
				err = -EINVAL;
				goto err;
			}

+
+			selectors[nselectors++] = selector;
+		}
+
+		err = ops->dcbnl_setapptrust(netdev, selectors, nselectors);
+		if (err)
+			goto err;
+	}
+
 err:
 	err = nla_put_u8(skb, DCB_ATTR_IEEE, err);
 	dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0);

Thanks, this looks good to me overall, despite the several points
Vladimir and I raised. I think it would be good to send this as non-RFC.

Note that for the non-RFC version, an actual user of the interface needs
to be present as well. So one of the offloading drivers should be
adapted to make use of the APP_TRUST and the new PCP selector.
mlxsw would like to make use of both, but I don't know when I will have
time to implement that.

Den Mon, Sep 19, 2022 at 09:54:23AM +0200 skrev Petr Machata:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Thanks, this looks good to me overall, despite the several points
Vladimir and I raised. I think it would be good to send this as non-RFC.

Note that for the non-RFC version, an actual user of the interface needs
to be present as well. So one of the offloading drivers should be
adapted to make use of the APP_TRUST and the new PCP selector.
mlxsw would like to make use of both, but I don't know when I will have
time to implement that.

Sounds good, and thanks for reviewing to you both.
I will go ahead and add support for this in the sparx5 driver - most of it
is already implemented during the tests anyway.

Should the driver support be posted together with said non-RFC patch
series?

/ Daniel

Daniel Machon [off-list ref] writes:

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.

[off-list ref] writes:

Den Mon, Sep 19, 2022 at 09:54:23AM +0200 skrev Petr Machata:

quoted

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Thanks, this looks good to me overall, despite the several points
Vladimir and I raised. I think it would be good to send this as non-RFC.

Note that for the non-RFC version, an actual user of the interface needs
to be present as well. So one of the offloading drivers should be
adapted to make use of the APP_TRUST and the new PCP selector.
mlxsw would like to make use of both, but I don't know when I will have
time to implement that.

Sounds good, and thanks for reviewing to you both.
I will go ahead and add support for this in the sparx5 driver - most of it
is already implemented during the tests anyway.

Should the driver support be posted together with said non-RFC patch
series?

Yes.

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 [off-list ref] writes:

quoted

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

[off-list ref] writes:

Den Mon, Sep 19, 2022 at 11:45:41AM +0200 skrev Petr Machata:

quoted

Daniel Machon [off-list ref] writes:

quoted

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.