Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
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
Thread-Topic: [PATCH net-next v2 1/6] net: dcb: add new pcp selector to app
 object
Thread-Index: AQHY1DNRRCKA3W+lZk2MiTdViI2XRa335cAAgARc1IA=
Date:   Mon, 3 Oct 2022 06:48:06 +0000
Message-ID: <YzqH/zuzvh35PVvF@DEN-LT-70577>
References: <20220929185207.2183473-1-daniel.machon@microchip.com>
 <20220929185207.2183473-2-daniel.machon@microchip.com>
 <87leq1uiyc.fsf@nvidia.com>
In-Reply-To: <87leq1uiyc.fsf@nvidia.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?+PAu7QPEkYZdcOuULnZYCwO++t3U/ugzL9rsq7FPnoyQ++667wl0PA1ya2kp?=
 =?us-ascii?Q?7A59JnrCp17W7GvetFKBKQf7OQX8pfoyrCcmMiG/gVC+Fx+UlmxI+tnuhj/g?=
 =?us-ascii?Q?RxJFTKFICvFOaEpu05H080NyqtCiwe/Xu0GyGAGXQTmi5nE0bhDnhhW9AZxv?=
 =?us-ascii?Q?2eUqt21wI8roWEDLivaxFUNpVoR4fmotYMPvAd9NvdYbZE1KmfJpykoy5wHO?=
 =?us-ascii?Q?+coNTMGvlCX6yqzrl4A11lM4AosufwJJqIZqA8ysFOC9YWGshKmIbap55ZwD?=
 =?us-ascii?Q?zslK3moxgX56y8quBgRa5AVoE9xcj1geGsBGmMJ/lOWKRxPQVPKYgH9FSr55?=
 =?us-ascii?Q?gxBxOYfTzxfVMRxC71upmitqEKc6YRxTRpEKPGZsujdAzEqGgd458Ez+kV2x?=
 =?us-ascii?Q?V4VC6FlyOjzaZKuDf0SXpPorVo65qknNlJUGEm9w+NpTvLoNDPGWFiKN0ECt?=
 =?us-ascii?Q?GLOy0j1gft+sb1VrVCaVhqp7lGvFCfI5Z/hbwdFBbANxFAdCJ5OdxDivgnZc?=
 =?us-ascii?Q?l70nOq0dJqYkNwu1+JUrBed1bjDB+ZQUtgS+IiOPbh3BSwvw44oeHShyWMb0?=
 =?us-ascii?Q?NSIsHroZjVpZnL+DCJQszAruo5fo6+HzgjCFe40EI/ESJiR1ts2s9K7SegTL?=
 =?us-ascii?Q?ctwM4JrgwkO4/uKoZBEiMVzeVjpDgqxl53L8cCvp/WDX1H0cE4LH03j6TOG8?=
 =?us-ascii?Q?ohZsGouxXOF9PnpQ4p7RWFpLPGnT3M+2qbwogoqRDesU6u0SLRKazjtl7Fji?=
 =?us-ascii?Q?UHJiEFMZiOoiIvgDBs9JhJ0EyHPT3Gx2we7INJz1KxBrYDqy4tS1wioqTkM0?=
 =?us-ascii?Q?m/HcJffuhDGbd2s97FcPmEhS8rf/Vt+GnJ17B7HO/nM031rGVOr0HPZCroJL?=
 =?us-ascii?Q?iBDfgv/LMRQMBoA3ZI7F2u7PI+E+Q7mfcRgPqI+dJ7s5bNtvmfO2p522bfMl?=
 =?us-ascii?Q?sT12D2X4oQ17aSgxgoegWpy+ZgzNvcRL9pYZBLvI3x5M2vrAowtd2K6Y8WVh?=
 =?us-ascii?Q?e47IjqFBQQnK9xGRFFfq9dDy9mUbAtL76GeNW1QLfP6LkQXpEY8lSh8XfAmx?=
 =?us-ascii?Q?Z2kP4UGal4OCU1DWSSd6xmNXytLDSwTLutji/ydE73hyn+o/ee0XL8b/FWiv?=
 =?us-ascii?Q?hzqoROrg3nBDczxP1n/YzWW/l78J4JnFAoTjj4bdnMrR+No2IWhir178d4kj?=
 =?us-ascii?Q?QiWs0OVbqHuxgkblGafmQUrhAWOOifs/SHu0f2VKaBm2I++fKGv49nl/6eNY?=
 =?us-ascii?Q?aXNT64jKM8lugT2/lNJG3Bd8TNgBXqx8ufBR6wy2HXJT4PLM4YEpF5/movd8?=
 =?us-ascii?Q?Qrs/F8Wb1kZ4isnKkoMmP+kq2dFbak+hOV02C1zB/7gn3rK5Igz6TI/g4+iK?=
 =?us-ascii?Q?Y+DSAtfuVnPm2JanTN9224i//2siUpgp1wGKA780iPLERKE8UbcUt4MrUQ0o?=
 =?us-ascii?Q?CGHVIPIV5+7c5qOZj4LyFY8kfHdlNcAhfBJks2N1FBMW028FzoyGhz23859E?=
 =?us-ascii?Q?Nf6ZZMiNR1TfC1YCvBbjuSzx6hSAt7VERrWVEjWt5j0bur8XSpAmTTRWHUd4?=
 =?us-ascii?Q?O3THDAMZM8KSxrjezUTdc7raz/Xdh0Zb0qpRx3Maja60SCHw81IwGGEsSb+F?=
 =?us-ascii?Q?Ab0tctuT+pgv1zT+xyTL8Pk=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8D8D508C558C4A48AE546AC05ADAAD22@namprd11.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5580.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0014b5c0-6c3d-4de7-f35c-08daa50b3a12
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Oct 2022 06:48:06.3265
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: wNXov+Lfcu5j6ms/RUndQ4dYgFuRElG5khGdJkEf7mhofKNAUGnzGlzPy+PoSriRbMb2XvdYbUHABRE+MJjwii3Nuif0th45pxgTCuWahhc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5118
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,
        SPF_PASS autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > 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.
>=20
> 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.
>=20
> > 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}.
>=20
> 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.
>=20
> > 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
>=20
> This is only necessary for the trust table code, so I would punt this
> into the next patch.

Will be fixed in next v.

>=20
> > +
> >  /**
> >   * 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] =3D {
> >  static LIST_HEAD(dcb_app_list);
> >  static DEFINE_SPINLOCK(dcb_lock);
> >
> > +static int dcbnl_app_attr_type_get(u8 selector)
>=20
> The return value can be directly enum ieee_attrs_app;

Will be fixed in next v.

>=20
> > +{
> > +     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 =3D DCB_ATTR_IEEE_APP;
> > +             break;
>=20
> Just return DCB_ATTR_IEEE_APP? Similarly below.

That's fine.

>=20
> > +     case DCB_APP_SEL_PCP:
> > +             type =3D DCB_ATTR_DCB_APP;
> > +             break;
> > +     default:
> > +             type =3D 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 =3D true;
> > +             break;
> > +     default:
> > +             ret =3D false;
> > +             break;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  static struct sk_buff *dcbnl_newmsg(int type, u8 cmd, u32 port, u32 se=
q,
> >                                   u32 flags, struct nlmsghdr **nlhp)
> >  {
> > @@ -1116,8 +1156,9 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, s=
truct net_device *netdev)
> >       spin_lock_bh(&dcb_lock);
> >       list_for_each_entry(itr, &dcb_app_list, list) {
> >               if (itr->ifindex =3D=3D netdev->ifindex) {
> > -                     err =3D nla_put(skb, DCB_ATTR_IEEE_APP, sizeof(it=
r->app),
> > -                                      &itr->app);
> > +                     enum ieee_attrs_app type =3D
> > +                             dcbnl_app_attr_type_get(itr->app.selector=
);
> > +                     err =3D 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 *netd=
ev, struct nlmsghdr *nlh,
> >               nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], =
rem) {
> >                       struct dcb_app *app_data;
> >
> > -                     if (nla_type(attr) !=3D DCB_ATTR_IEEE_APP)
> > +                     if (!dcbnl_app_attr_type_validate(nla_type(attr))=
)
>=20
> 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.
>=20
> 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.
>=20
> This needs to be done differently.
>=20
> One API "hole" that I see is that payload with size < struct dcb_app
> gets bounced.
>=20
> 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=20
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.=