Messages in this thread | | | From | <> | Subject | Re: [PATCH net-next v2 1/6] net: dcb: add new pcp selector to app object | Date | Mon, 3 Oct 2022 06:48:06 +0000 |
| |
> > 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 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. > > Just a note: the "dcb app" block deals with packet prioritization. > Classification is handled through "dcb ets prio-tc", or offloaded egress > qdiscs or whatever, regardless of how the priority was derived. > > > 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}. > > It would be good to shout out that the new selector value is 255. > Also it would be good to be explicit about how the same struct dcb_app > is used for both standard and non-standard attributes, because there > currently is no overlap between the two namespaces. > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > --- > > include/uapi/linux/dcbnl.h | 6 +++++ > > net/dcb/dcbnl.c | 49 ++++++++++++++++++++++++++++++++++---- > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h > > index a791a94013a6..9f68dc501cc1 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 24 > > + > > /* 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 > > This is only necessary for the trust table code, so I would punt this > into the next patch.
Will be fixed in next v.
> > > + > > /** > > * 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..580d26acfc84 100644 > > --- a/net/dcb/dcbnl.c > > +++ b/net/dcb/dcbnl.c > > @@ -179,6 +179,46 @@ 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 int dcbnl_app_attr_type_get(u8 selector) > > The return value can be directly enum ieee_attrs_app;
Will be fixed in next v.
> > > +{ > > + enum ieee_attrs_app type; > > + > > + 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: > > + type = DCB_ATTR_IEEE_APP; > > + break; > > Just return DCB_ATTR_IEEE_APP? Similarly below.
That's fine.
> > > + case DCB_APP_SEL_PCP: > > + type = DCB_ATTR_DCB_APP; > > + break; > > + default: > > + type = DCB_ATTR_IEEE_APP_UNSPEC; > > + break; > > + } > > + > > + return type; > > +} > > + > > +static int dcbnl_app_attr_type_validate(enum ieee_attrs_app type) > > +{ > > + bool ret; > > + > > + switch (type) { > > + case DCB_ATTR_IEEE_APP: > > + case DCB_ATTR_DCB_APP: > > + ret = true; > > + break; > > + default: > > + ret = false; > > + break; > > + } > > + > > + return ret; > > +} > > + > > static struct sk_buff *dcbnl_newmsg(int type, u8 cmd, u32 port, u32 seq, > > u32 flags, struct nlmsghdr **nlhp) > > { > > @@ -1116,8 +1156,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 +1536,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))) > > Oh no! It wasn't validating the DCB_ATTR_IEEE_APP_TABLE nest against a > policy! Instead it was just skipping whatever is not DCB_ATTR_IEEE_APP. > > So userspace was permitted to shove random crap down here, and it would > just quietly be ignored. We can't start reinterpreting some of that crap > as information. We also can't start bouncing it. > > This needs to be done differently. > > One API "hole" that I see is that payload with size < struct dcb_app > gets bounced. > > We can pack the new stuff into a smaller payload. The inner attribute > would not be DCB_ATTR_DCB_APP, but say DCB_ATTR_DCB_PCP, which would > imply the selector. The payload can be struct { u8 prio; u16 proto; }. > This would have been bounced by the old UAPI, so we know no userspace > makes use of that.
Right, I see your point. But. First thought; this starts to look a little hackish.
Looking through the 802.1Q-2018 std again, sel bits 0, 6 and 7 are reserved (implicit for future standard implementation?). Do we know of any cases, where a new standard version would introduce new values beyond what was reserved in the first place for future use? I dont know myself.
I am just trying to raise a question of whether using the std APP attr with a new high (255) selector, really could be preferred over this new non-std APP attr with new packed payload.
| |