| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YzqH/zuzvh35PVvF@DEN-LT-70577>
Date: Mon, 3 Oct 2022 06:48:06 +0000
From: <Daniel.Machon@...rochip.com>
To: <petrm@...dia.com>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>,
<maxime.chevallier@...tlin.com>, <thomas.petazzoni@...tlin.com>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<Lars.Povlsen@...rochip.com>, <Steen.Hegelund@...rochip.com>,
<UNGLinuxDriver@...rochip.com>, <joe@...ches.com>,
<linux@...linux.org.uk>, <Horatiu.Vultur@...rochip.com>,
<Julia.Lawall@...ia.fr>, <vladimir.oltean@....com>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH net-next v2 1/6] net: dcb: add new pcp selector to app
object
> > 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@...rochip.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.
Powered by blists - more mailing lists