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

From: <Daniel.Machon@microchip.com>
To: <petrm@nvidia.com>
Cc: <netdev@vger.kernel.org>, <davem@davemloft.net>,
	<maxime.chevallier@bootlin.com>, <thomas.petazzoni@bootlin.com>,
	<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
	<Lars.Povlsen@microchip.com>, <Steen.Hegelund@microchip.com>,
	<UNGLinuxDriver@microchip.com>, <joe@perches.com>,
	<linux@armlinux.org.uk>, <Horatiu.Vultur@microchip.com>,
	<Julia.Lawall@inria.fr>, <vladimir.oltean@nxp.com>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
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	[thread overview]
Message-ID: <YzqH/zuzvh35PVvF@DEN-LT-70577> (raw)
In-Reply-To: <87leq1uiyc.fsf@nvidia.com>

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