[net-next,v3,1/6] net: dcb: add new pcp selector to app object

Message ID 20221024091333.1048061-2-daniel.machon@microchip.com
State New
Headers
Series Add new PCP and APPTRUST attributes to dcbnl |

Commit Message

Daniel Machon Oct. 24, 2022, 9:13 a.m. UTC
  Add new PCP selector for the 8021Qaz APP managed object.

As the PCP selector is not part of the 8021Qaz standard, a new non-std
extension attribute DCB_ATTR_DCB_APP has been introduced. Also two
helper functions to translate between selector and app attribute type
has been added. The new selector has been given a value of 255, to
minimize the risk of future overlap of std- and non-std attributes.

The new DCB_ATTR_DCB_APP is sent alongside the ieee std attribute in the
app table. This means that the dcb_app struct can now both contain std-
and non-std app attributes. Currently there is no overlap between the
selector values of the two attributes.

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}.

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

Comments

Petr Machata Oct. 26, 2022, 10:11 a.m. UTC | #1
Daniel Machon <daniel.machon@microchip.com> writes:

> Add new PCP selector for the 8021Qaz APP managed object.
>
> As the PCP selector is not part of the 8021Qaz standard, a new non-std
> extension attribute DCB_ATTR_DCB_APP has been introduced. Also two
> helper functions to translate between selector and app attribute type
> has been added. The new selector has been given a value of 255, to
> minimize the risk of future overlap of std- and non-std attributes.
>
> The new DCB_ATTR_DCB_APP is sent alongside the ieee std attribute in the
> app table. This means that the dcb_app struct can now both contain std-
> and non-std app attributes. Currently there is no overlap between the
> selector values of the two attributes.
>
> 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}.
>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>

>  static struct sk_buff *dcbnl_newmsg(int type, u8 cmd, u32 port, u32 seq,
>  				    u32 flags, struct nlmsghdr **nlhp)
>  {
> @@ -1116,8 +1143,9 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
>  	spin_lock_bh(&dcb_lock);
>  	list_for_each_entry(itr, &dcb_app_list, list) {
>  		if (itr->ifindex == netdev->ifindex) {
> -			err = nla_put(skb, DCB_ATTR_IEEE_APP, sizeof(itr->app),
> -					 &itr->app);
> +			enum ieee_attrs_app type =
> +				dcbnl_app_attr_type_get(itr->app.selector);
> +			err = nla_put(skb, type, sizeof(itr->app), &itr->app);
>  			if (err) {
>  				spin_unlock_bh(&dcb_lock);
>  				return -EMSGSIZE;
> @@ -1495,7 +1523,7 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
>  		nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
>  			struct dcb_app *app_data;
>  
> -			if (nla_type(attr) != DCB_ATTR_IEEE_APP)
> +			if (!dcbnl_app_attr_type_validate(nla_type(attr)))
>  				continue;
>  
>  			if (nla_len(attr) < sizeof(struct dcb_app)) {
> @@ -1556,7 +1584,7 @@ static int dcbnl_ieee_del(struct net_device *netdev, struct nlmsghdr *nlh,
>  		nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
>  			struct dcb_app *app_data;
>  
> -			if (nla_type(attr) != DCB_ATTR_IEEE_APP)
> +			if (!dcbnl_app_attr_type_validate(nla_type(attr)))
>  				continue;
>  			app_data = nla_data(attr);
>  			if (ops->ieee_delapp)

I'm missing a validation that DCB_APP_SEL_PCP is always sent in
DCB_ATTR_DCB_APP encapsulation. Wouldn't the current code permit
sending it in the IEEE encap? This should be forbidden.

And vice versa: I'm not sure we want to permit sending the standard
attributes in the DCB encap.
  
Daniel Machon Oct. 26, 2022, 11:19 a.m. UTC | #2
> > Add new PCP selector for the 8021Qaz APP managed object.
> >
> > As the PCP selector is not part of the 8021Qaz standard, a new non-std
> > extension attribute DCB_ATTR_DCB_APP has been introduced. Also two
> > helper functions to translate between selector and app attribute type
> > has been added. The new selector has been given a value of 255, to
> > minimize the risk of future overlap of std- and non-std attributes.
> >
> > The new DCB_ATTR_DCB_APP is sent alongside the ieee std attribute in the
> > app table. This means that the dcb_app struct can now both contain std-
> > and non-std app attributes. Currently there is no overlap between the
> > selector values of the two attributes.
> >
> > 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}.
> >
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> 
> >  static struct sk_buff *dcbnl_newmsg(int type, u8 cmd, u32 port, u32 seq,
> >                                   u32 flags, struct nlmsghdr **nlhp)
> >  {
> > @@ -1116,8 +1143,9 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
> >       spin_lock_bh(&dcb_lock);
> >       list_for_each_entry(itr, &dcb_app_list, list) {
> >               if (itr->ifindex == netdev->ifindex) {
> > -                     err = nla_put(skb, DCB_ATTR_IEEE_APP, sizeof(itr->app),
> > -                                      &itr->app);
> > +                     enum ieee_attrs_app type =
> > +                             dcbnl_app_attr_type_get(itr->app.selector);
> > +                     err = nla_put(skb, type, sizeof(itr->app), &itr->app);
> >                       if (err) {
> >                               spin_unlock_bh(&dcb_lock);
> >                               return -EMSGSIZE;
> > @@ -1495,7 +1523,7 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
> >               nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
> >                       struct dcb_app *app_data;
> >
> > -                     if (nla_type(attr) != DCB_ATTR_IEEE_APP)
> > +                     if (!dcbnl_app_attr_type_validate(nla_type(attr)))
> >                               continue;
> >
> >                       if (nla_len(attr) < sizeof(struct dcb_app)) {
> > @@ -1556,7 +1584,7 @@ static int dcbnl_ieee_del(struct net_device *netdev, struct nlmsghdr *nlh,
> >               nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
> >                       struct dcb_app *app_data;
> >
> > -                     if (nla_type(attr) != DCB_ATTR_IEEE_APP)
> > +                     if (!dcbnl_app_attr_type_validate(nla_type(attr)))
> >                               continue;
> >                       app_data = nla_data(attr);
> >                       if (ops->ieee_delapp)
> 
> I'm missing a validation that DCB_APP_SEL_PCP is always sent in
> DCB_ATTR_DCB_APP encapsulation. Wouldn't the current code permit
> sending it in the IEEE encap? This should be forbidden.

Right. Current impl. does not check that the non-std selectors received, are
sent with a DCB_ATTR_DCB_APP type.  We could introduce a new check
dcbnl_app_attr_selector_validate() that checks combination of type and 
selector, after the type and nla_len(attr) has been checked, so that:

 validate type -> validate nla_len(attr) -> validate selector

> And vice versa: I'm not sure we want to permit sending the standard
> attributes in the DCB encap.

dcbnl_app_attr_type_get() in dcbnl_ieee_fill() takes care of this. IEEE are
always sent in DCB_ATTR_IEEE and non-std are sent in DCB_ATTR_DCB.

/ Daniel
  
Petr Machata Oct. 26, 2022, 2:51 p.m. UTC | #3
<Daniel.Machon@microchip.com> writes:

>> I'm missing a validation that DCB_APP_SEL_PCP is always sent in
>> DCB_ATTR_DCB_APP encapsulation. Wouldn't the current code permit
>> sending it in the IEEE encap? This should be forbidden.
>
> Right. Current impl. does not check that the non-std selectors received, are
> sent with a DCB_ATTR_DCB_APP type.  We could introduce a new check
> dcbnl_app_attr_selector_validate() that checks combination of type and 
> selector, after the type and nla_len(attr) has been checked, so that:
>
>  validate type -> validate nla_len(attr) -> validate selector

This needs to be validated, otherwise there's no point in having a
dedicated attribute for the non-standard stuff.

>> And vice versa: I'm not sure we want to permit sending the standard
>> attributes in the DCB encap.
>
> dcbnl_app_attr_type_get() in dcbnl_ieee_fill() takes care of this. IEEE are
> always sent in DCB_ATTR_IEEE and non-std are sent in DCB_ATTR_DCB.

By "sending" I meant userspace sending this to the kernel. So bounce
extended opcodes that are wrapped in IEEE and bounce IEEE opcodes
wrapped in DCB as well.
  
Daniel Machon Oct. 27, 2022, 9 a.m. UTC | #4
> >> I'm missing a validation that DCB_APP_SEL_PCP is always sent in
> >> DCB_ATTR_DCB_APP encapsulation. Wouldn't the current code permit
> >> sending it in the IEEE encap? This should be forbidden.
> >
> > Right. Current impl. does not check that the non-std selectors received, are
> > sent with a DCB_ATTR_DCB_APP type.  We could introduce a new check
> > dcbnl_app_attr_selector_validate() that checks combination of type and
> > selector, after the type and nla_len(attr) has been checked, so that:
> >
> >  validate type -> validate nla_len(attr) -> validate selector
> 
> This needs to be validated, otherwise there's no point in having a
> dedicated attribute for the non-standard stuff.

Agree.

> 
> >> And vice versa: I'm not sure we want to permit sending the standard
> >> attributes in the DCB encap.
> >
> > dcbnl_app_attr_type_get() in dcbnl_ieee_fill() takes care of this. IEEE are
> > always sent in DCB_ATTR_IEEE and non-std are sent in DCB_ATTR_DCB.
> 
> By "sending" I meant userspace sending this to the kernel. So bounce
> extended opcodes that are wrapped in IEEE and bounce IEEE opcodes
> wrapped in DCB as well.

Right. Then we only need to decide what to do with any opcode in-between
(not defined in uapi, neither ieee or extension opcode, 7-254). If they are 
sent in DCB_ATTR_DCB they should be bounced, because we agreed that we can 
interpret data in the new attr), _but_ if they are sent in DCB_ATTR_IEEE I 
guess we should accept them, to not break userspace that is already sending
them.

Here is what that could look like:

	/* Make sure any non-std selectors is always encapsulated in the non-std
	* DCB_ATTR_DCB_APP attribute.
	*/
	static bool dcbnl_app_attr_selector_validate(enum ieee_attrs_app type, u32 selector)
	{
		switch (selector) {
		case DCB_APP_SEL_PCP:
			/* Non-std selector in non-std attr? */
			if (type == DCB_ATTR_DCB_APP)
				return true;
		default:
			/* Std selector in std attr? */
			if (type == DCB_ATTR_IEEE_APP)
				return true;
		}

		return false;
	}

/ Daniel
  
Petr Machata Oct. 27, 2022, 9:59 a.m. UTC | #5
<Daniel.Machon@microchip.com> writes:

>> >> And vice versa: I'm not sure we want to permit sending the standard
>> >> attributes in the DCB encap.
>> >
>> > dcbnl_app_attr_type_get() in dcbnl_ieee_fill() takes care of this. IEEE are
>> > always sent in DCB_ATTR_IEEE and non-std are sent in DCB_ATTR_DCB.
>> 
>> By "sending" I meant userspace sending this to the kernel. So bounce
>> extended opcodes that are wrapped in IEEE and bounce IEEE opcodes
>> wrapped in DCB as well.
>
> Right. Then we only need to decide what to do with any opcode in-between
> (not defined in uapi, neither ieee or extension opcode, 7-254). If they are 
> sent in DCB_ATTR_DCB they should be bounced, because we agreed that we can 
> interpret data in the new attr), _but_ if they are sent in DCB_ATTR_IEEE I 
> guess we should accept them, to not break userspace that is already sending
> them.

I see, it's not currently validating at all. It just relies on the
driver to do the validation, but e.g. bnxt_dcbnl_ieee_setapp(), just
lets nonsense right through.

OK, but this interface is built on standards. The selector has a
well-defined, IEEE-backed meaning with enumerators published in the UAPI
headers. As before, even though this constitutes API breakage, IMHO if
anyone relies on shoving random garbage through this interface, it's on
them...

I think it's kosher to start bouncing undefined selectors.
  

Patch

diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
index a791a94013a6..dc7ef96207ca 100644
--- a/include/uapi/linux/dcbnl.h
+++ b/include/uapi/linux/dcbnl.h
@@ -218,6 +218,9 @@  struct cee_pfc {
 #define IEEE_8021QAZ_APP_SEL_ANY	4
 #define IEEE_8021QAZ_APP_SEL_DSCP       5
 
+/* Non-std selector values */
+#define DCB_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.
  *
@@ -247,6 +250,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
  *
@@ -425,6 +430,7 @@  enum ieee_attrs {
 enum ieee_attrs_app {
 	DCB_ATTR_IEEE_APP_UNSPEC,
 	DCB_ATTR_IEEE_APP,
+	DCB_ATTR_DCB_APP,
 	__DCB_ATTR_IEEE_APP_MAX
 };
 #define DCB_ATTR_IEEE_APP_MAX (__DCB_ATTR_IEEE_APP_MAX - 1)
diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index dc4fb699b56c..92c32bc11374 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -179,6 +179,33 @@  static const struct nla_policy dcbnl_featcfg_nest[DCB_FEATCFG_ATTR_MAX + 1] = {
 static LIST_HEAD(dcb_app_list);
 static DEFINE_SPINLOCK(dcb_lock);
 
+static enum ieee_attrs_app dcbnl_app_attr_type_get(u8 selector)
+{
+	switch (selector) {
+	case IEEE_8021QAZ_APP_SEL_ETHERTYPE:
+	case IEEE_8021QAZ_APP_SEL_STREAM:
+	case IEEE_8021QAZ_APP_SEL_DGRAM:
+	case IEEE_8021QAZ_APP_SEL_ANY:
+	case IEEE_8021QAZ_APP_SEL_DSCP:
+		return DCB_ATTR_IEEE_APP;
+	case DCB_APP_SEL_PCP:
+		return DCB_ATTR_DCB_APP;
+	default:
+		return DCB_ATTR_IEEE_APP_UNSPEC;
+	}
+}
+
+static bool dcbnl_app_attr_type_validate(enum ieee_attrs_app type)
+{
+	switch (type) {
+	case DCB_ATTR_IEEE_APP:
+	case DCB_ATTR_DCB_APP:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static struct sk_buff *dcbnl_newmsg(int type, u8 cmd, u32 port, u32 seq,
 				    u32 flags, struct nlmsghdr **nlhp)
 {
@@ -1116,8 +1143,9 @@  static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
 	spin_lock_bh(&dcb_lock);
 	list_for_each_entry(itr, &dcb_app_list, list) {
 		if (itr->ifindex == netdev->ifindex) {
-			err = nla_put(skb, DCB_ATTR_IEEE_APP, sizeof(itr->app),
-					 &itr->app);
+			enum ieee_attrs_app type =
+				dcbnl_app_attr_type_get(itr->app.selector);
+			err = nla_put(skb, type, sizeof(itr->app), &itr->app);
 			if (err) {
 				spin_unlock_bh(&dcb_lock);
 				return -EMSGSIZE;
@@ -1495,7 +1523,7 @@  static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
 		nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
 			struct dcb_app *app_data;
 
-			if (nla_type(attr) != DCB_ATTR_IEEE_APP)
+			if (!dcbnl_app_attr_type_validate(nla_type(attr)))
 				continue;
 
 			if (nla_len(attr) < sizeof(struct dcb_app)) {
@@ -1556,7 +1584,7 @@  static int dcbnl_ieee_del(struct net_device *netdev, struct nlmsghdr *nlh,
 		nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) {
 			struct dcb_app *app_data;
 
-			if (nla_type(attr) != DCB_ATTR_IEEE_APP)
+			if (!dcbnl_app_attr_type_validate(nla_type(attr)))
 				continue;
 			app_data = nla_data(attr);
 			if (ops->ieee_delapp)