| Message ID | 20221024091333.1048061-2-daniel.machon@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add new PCP and APPTRUST attributes to dcbnl | expand |
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.
> > 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
<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.
> >> 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
<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.
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)
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(-)