| Message ID | 20220908120442.3069771-3-daniel.machon@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add PCP selector and new APPTRUST attribute | expand |
Hi Daniel, On Thu, Sep 08, 2022 at 02:04:42PM +0200, Daniel Machon wrote: > Add a new apptrust extension attribute to the 8021Qaz APP managed > object. > > The new attribute is meant to allow drivers, whose hw supports the > notion of trust, to be able to set whether a particular app selector is > to be trusted - and also the order of precedence of selectors. > > A new structure ieee_apptrust has been created, which contains an array > of selectors, where lower indexes has higher precedence. > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > --- Let's say I have a switch which only looks at VLAN PCP/DEI if the bridge vlan_filtering setting is enabled (otherwise, the switch is completely VLAN unaware, including for QoS purposes). Would it be ok to report through ieee_getapptrust() that the PCP selector is trusted when under a vlan_filtering bridge, not trusted when not under a vlan_filtering bridge, and deny changes to ieee_setapptrust() for the PCP selector? I see the return value is not cached anywhere within the kernel, just passed to the user.
Den Fri, Sep 09, 2022 at 12:29:50PM +0000 skrev Vladimir Oltean: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Daniel, > > On Thu, Sep 08, 2022 at 02:04:42PM +0200, Daniel Machon wrote: > > Add a new apptrust extension attribute to the 8021Qaz APP managed > > object. > > > > The new attribute is meant to allow drivers, whose hw supports the > > notion of trust, to be able to set whether a particular app selector is > > to be trusted - and also the order of precedence of selectors. > > > > A new structure ieee_apptrust has been created, which contains an array > > of selectors, where lower indexes has higher precedence. > > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > --- > > Let's say I have a switch which only looks at VLAN PCP/DEI if the bridge > vlan_filtering setting is enabled (otherwise, the switch is completely > VLAN unaware, including for QoS purposes). > > Would it be ok to report through ieee_getapptrust() that the PCP > selector is trusted when under a vlan_filtering bridge, not trusted when > not under a vlan_filtering bridge, and deny changes to ieee_setapptrust() > for the PCP selector? I see the return value is not cached anywhere > within the kernel, just passed to the user. There *might* be a distinction between enabled and trusted, disabled and not-trusted. For instance, sparx5 switch has this distinction (at least for dscp) - but really that is hw dependent. Therefore, in your particular case, with the vlan_filtering on/off, yes that would be OK IMO. Any concerns? This patch merely provides the means for drivers to implement a user-specified trust order and report it back to the user, just like with many of the other dcb attributes (maxrate, buffer etc.).
Daniel Machon <daniel.machon@microchip.com> writes: > Add a new apptrust extension attribute to the 8021Qaz APP managed > object. > > The new attribute is meant to allow drivers, whose hw supports the > notion of trust, to be able to set whether a particular app selector is > to be trusted - and also the order of precedence of selectors. > > A new structure ieee_apptrust has been created, which contains an array > of selectors, where lower indexes has higher precedence. > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > --- > include/net/dcbnl.h | 2 ++ > include/uapi/linux/dcbnl.h | 14 ++++++++++++++ > net/dcb/dcbnl.c | 17 +++++++++++++++++ > 3 files changed, 33 insertions(+) > > diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h > index 2b2d86fb3131..0c4b0107981d 100644 > --- a/include/net/dcbnl.h > +++ b/include/net/dcbnl.h > @@ -61,6 +61,8 @@ struct dcbnl_rtnl_ops { > int (*ieee_getapp) (struct net_device *, struct dcb_app *); > int (*ieee_setapp) (struct net_device *, struct dcb_app *); > int (*ieee_delapp) (struct net_device *, struct dcb_app *); > + int (*ieee_setapptrust) (struct net_device *, struct ieee_apptrust *); > + int (*ieee_getapptrust) (struct net_device *, struct ieee_apptrust *); > int (*ieee_peer_getets) (struct net_device *, struct ieee_ets *); > int (*ieee_peer_getpfc) (struct net_device *, struct ieee_pfc *); > > diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h > index 8eab16e5bc13..833466dec096 100644 > --- a/include/uapi/linux/dcbnl.h > +++ b/include/uapi/linux/dcbnl.h > @@ -248,6 +248,19 @@ struct dcb_app { > __u16 protocol; > }; > > +#define IEEE_8021QAZ_APP_SEL_MAX 255 > + > +/* This structure contains trust order extension to the IEEE 802.1Qaz APP > + * managed object. > + * > + * @order: contains trust ordering of selector values for the IEEE 802.1Qaz > + * APP managed object. Lower indexes has higher trust. > + */ > +struct ieee_apptrust { Trust level setting is not standard, so this should be something like dcbnl_apptrust. Ditto for DCB_ATTR_IEEE_APP_TRUST below. I believe DCB_ATTR_DCB_BUFFER is in the DCB namespace for that reason, and the trust level artifacts should be as well. Likewise with the DCB ops callbacks, which should IMHO be dcbnl_get/setapptrust. > + __u8 num; Is this supposed to be a count of the "order" items? If yes, I'd call it "count", because then it's clear the value is not just a number, but actually a number of items. > + __u8 order[IEEE_8021QAZ_APP_SEL_MAX]; Should be +1 IMHO, in case the whole u8 selector space is allocated. (In particular, there's no guarantee that IEEE won't ever allocate the selector 0.) But of course this will never get anywhere close to that. We will end up passing maybe one, two entries. So the UAPI seems excessive in how it hands around this large array. I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE payload be an array of bytes, each byte a selector? Or something similar to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of payload attributes? > +}; > + > /** > * struct dcb_peer_app_info - APP feature information sent by the peer > * > @@ -419,6 +432,7 @@ enum ieee_attrs { > DCB_ATTR_IEEE_QCN, > DCB_ATTR_IEEE_QCN_STATS, > DCB_ATTR_DCB_BUFFER, > + DCB_ATTR_IEEE_APP_TRUST, > __DCB_ATTR_IEEE_MAX > }; > #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1) > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c > index dc4fb699b56c..e87f0128c3bd 100644 > --- a/net/dcb/dcbnl.c > +++ b/net/dcb/dcbnl.c > @@ -162,6 +162,7 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = { > [DCB_ATTR_IEEE_ETS] = {.len = sizeof(struct ieee_ets)}, > [DCB_ATTR_IEEE_PFC] = {.len = sizeof(struct ieee_pfc)}, > [DCB_ATTR_IEEE_APP_TABLE] = {.type = NLA_NESTED}, > + [DCB_ATTR_IEEE_APP_TRUST] = {.len = sizeof(struct ieee_apptrust)}, > [DCB_ATTR_IEEE_MAXRATE] = {.len = sizeof(struct ieee_maxrate)}, > [DCB_ATTR_IEEE_QCN] = {.len = sizeof(struct ieee_qcn)}, > [DCB_ATTR_IEEE_QCN_STATS] = {.len = sizeof(struct ieee_qcn_stats)}, > @@ -1133,6 +1134,14 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) > spin_unlock_bh(&dcb_lock); > nla_nest_end(skb, app); > > + if (ops->ieee_getapptrust) { > + struct ieee_apptrust trust; I believe checkpatch warns if there's no empty line between the variable declaration block and the rest of the code. Maybe it's just because this is an RFC, but for the final submission please make sure you run checkpatch.pl --max-line-length=80. The 80-character limit is not really a hard requirement anymore, but it's still strongly preferred in net. > + memset(&trust, 0, sizeof(trust)); > + err = ops->ieee_getapptrust(netdev, &trust); > + if (!err && nla_put(skb, DCB_ATTR_IEEE_APP_TRUST, sizeof(trust), &trust)) > + return -EMSGSIZE; > + } > + > /* get peer info if available */ > if (ops->ieee_peer_getets) { > struct ieee_ets ets; > @@ -1513,6 +1522,14 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh, > } > } > > + if (ieee[DCB_ATTR_IEEE_APP_TRUST] && ops->ieee_setapptrust) { Hmm, weird that none of the set requests are bounced if there's no set callback. That way the request appears to succeed but doesn't actually set anything. I find this very strange. Drivers that do not even have any DCB ops give -EOPNOTSUPP as expected. I think lack of a particular op should do this as well. We can't change this for the existing ones, but IMHO the new OPs should be like that. > + struct ieee_apptrust *trust = > + nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]); Besides invoking the OP, this should validate the payload. E.g. no driver is supposed to accept trust policies that contain invalid selectors. Pretty sure there's no value in repeated entries either. > + err = ops->ieee_setapptrust(netdev, trust); > + if (err) > + goto err; > + } > + > err: > err = nla_put_u8(skb, DCB_ATTR_IEEE, err); > dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0);
<Daniel.Machon@microchip.com> writes: > Den Fri, Sep 09, 2022 at 12:29:50PM +0000 skrev Vladimir Oltean: > >> Let's say I have a switch which only looks at VLAN PCP/DEI if the bridge >> vlan_filtering setting is enabled (otherwise, the switch is completely >> VLAN unaware, including for QoS purposes). >> >> Would it be ok to report through ieee_getapptrust() that the PCP >> selector is trusted when under a vlan_filtering bridge, not trusted when >> not under a vlan_filtering bridge, and deny changes to ieee_setapptrust() >> for the PCP selector? I see the return value is not cached anywhere >> within the kernel, just passed to the user. > > Therefore, in your particular case, with the vlan_filtering on/off, > yes that would be OK IMO. Any concerns? Yeah, it would make sense to me. With the 802.1q bridge, the reported trust level would be [PCP], with 802.1d it would be []. As a service to the user, I would accept set requests that just reassert the only valid configuration, but otherwise it sounds OK to me.
On Mon, Sep 12, 2022 at 06:30:08PM +0200, Petr Machata wrote: > > <Daniel.Machon@microchip.com> writes: > > > Den Fri, Sep 09, 2022 at 12:29:50PM +0000 skrev Vladimir Oltean: > > > >> Let's say I have a switch which only looks at VLAN PCP/DEI if the bridge > >> vlan_filtering setting is enabled (otherwise, the switch is completely > >> VLAN unaware, including for QoS purposes). > >> > >> Would it be ok to report through ieee_getapptrust() that the PCP > >> selector is trusted when under a vlan_filtering bridge, not trusted when > >> not under a vlan_filtering bridge, and deny changes to ieee_setapptrust() > >> for the PCP selector? I see the return value is not cached anywhere > >> within the kernel, just passed to the user. > > > > Therefore, in your particular case, with the vlan_filtering on/off, > > yes that would be OK IMO. Any concerns? > > Yeah, it would make sense to me. With the 802.1q bridge, the reported > trust level would be [PCP], with 802.1d it would be []. > > As a service to the user, I would accept set requests that just reassert > the only valid configuration, but otherwise it sounds OK to me. This sounds good to me too.
Hi Petr, Thank you for your feedback. > Daniel Machon <daniel.machon@microchip.com> writes: > > > Add a new apptrust extension attribute to the 8021Qaz APP managed > > object. > > > > The new attribute is meant to allow drivers, whose hw supports the > > notion of trust, to be able to set whether a particular app selector is > > to be trusted - and also the order of precedence of selectors. > > > > A new structure ieee_apptrust has been created, which contains an array > > of selectors, where lower indexes has higher precedence. > > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > --- > > include/net/dcbnl.h | 2 ++ > > include/uapi/linux/dcbnl.h | 14 ++++++++++++++ > > net/dcb/dcbnl.c | 17 +++++++++++++++++ > > 3 files changed, 33 insertions(+) > > > > diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h > > index 2b2d86fb3131..0c4b0107981d 100644 > > --- a/include/net/dcbnl.h > > +++ b/include/net/dcbnl.h > > @@ -61,6 +61,8 @@ struct dcbnl_rtnl_ops { > > int (*ieee_getapp) (struct net_device *, struct dcb_app *); > > int (*ieee_setapp) (struct net_device *, struct dcb_app *); > > int (*ieee_delapp) (struct net_device *, struct dcb_app *); > > + int (*ieee_setapptrust) (struct net_device *, struct ieee_apptrust *); > > + int (*ieee_getapptrust) (struct net_device *, struct ieee_apptrust *); > > int (*ieee_peer_getets) (struct net_device *, struct ieee_ets *); > > int (*ieee_peer_getpfc) (struct net_device *, struct ieee_pfc *); > > > > diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h > > index 8eab16e5bc13..833466dec096 100644 > > --- a/include/uapi/linux/dcbnl.h > > +++ b/include/uapi/linux/dcbnl.h > > @@ -248,6 +248,19 @@ struct dcb_app { > > __u16 protocol; > > }; > > > > +#define IEEE_8021QAZ_APP_SEL_MAX 255 > > + > > +/* This structure contains trust order extension to the IEEE 802.1Qaz APP > > + * managed object. > > + * > > + * @order: contains trust ordering of selector values for the IEEE 802.1Qaz > > + * APP managed object. Lower indexes has higher trust. > > + */ > > +struct ieee_apptrust { > > Trust level setting is not standard, so this should be something like > dcbnl_apptrust. Ack. > Ditto for DCB_ATTR_IEEE_APP_TRUST below. I believe DCB_ATTR_DCB_BUFFER > is in the DCB namespace for that reason, and the trust level artifacts > should be as well. Likewise with the DCB ops callbacks, which should > IMHO be dcbnl_get/setapptrust. > > > + __u8 num; > > Is this supposed to be a count of the "order" items? > If yes, I'd call it "count", because then it's clear the value is not > just a number, but actually a number of items. Yes, this is the number of selector-occupied indexes. I dont have any strongs feelings on num vs count - we can go with count. > > > + __u8 order[IEEE_8021QAZ_APP_SEL_MAX]; > > Should be +1 IMHO, in case the whole u8 selector space is allocated. (In > particular, there's no guarantee that IEEE won't ever allocate the > selector 0.) Ack. > > But of course this will never get anywhere close to that. We will end up > passing maybe one, two entries. So the UAPI seems excessive in how it > hands around this large array. > > I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE > payload be an array of bytes, each byte a selector? Or something similar > to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of > payload attributes? Hmm. It might seem excessive, but a quick few thoughts on your proposed solution: - We need more code to define and parse the new DCB_ATTR_IEEE_APP_TRUST_TABLE / DCB_ATTR_IEEE_APP_TRUST attributes. - If the selectors are passed individually to the driver, we need a dcbnl_delapptrust(), because now, the driver have to add and del from the driver maintained array. You could of course accumulate selectors in an array before passing them to the driver, but then why go away from the array in the first place. I kind of like the 'set' nature more than the 'add' 'del' nature. What do you think? > > > +}; > > + > > /** > > * struct dcb_peer_app_info - APP feature information sent by the peer > > * > > @@ -419,6 +432,7 @@ enum ieee_attrs { > > DCB_ATTR_IEEE_QCN, > > DCB_ATTR_IEEE_QCN_STATS, > > DCB_ATTR_DCB_BUFFER, > > + DCB_ATTR_IEEE_APP_TRUST, > > __DCB_ATTR_IEEE_MAX > > }; > > #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1) > > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c > > index dc4fb699b56c..e87f0128c3bd 100644 > > --- a/net/dcb/dcbnl.c > > +++ b/net/dcb/dcbnl.c > > @@ -162,6 +162,7 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = { > > [DCB_ATTR_IEEE_ETS] = {.len = sizeof(struct ieee_ets)}, > > [DCB_ATTR_IEEE_PFC] = {.len = sizeof(struct ieee_pfc)}, > > [DCB_ATTR_IEEE_APP_TABLE] = {.type = NLA_NESTED}, > > + [DCB_ATTR_IEEE_APP_TRUST] = {.len = sizeof(struct ieee_apptrust)}, > > [DCB_ATTR_IEEE_MAXRATE] = {.len = sizeof(struct ieee_maxrate)}, > > [DCB_ATTR_IEEE_QCN] = {.len = sizeof(struct ieee_qcn)}, > > [DCB_ATTR_IEEE_QCN_STATS] = {.len = sizeof(struct ieee_qcn_stats)}, > > @@ -1133,6 +1134,14 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) > > spin_unlock_bh(&dcb_lock); > > nla_nest_end(skb, app); > > > > + if (ops->ieee_getapptrust) { > > + struct ieee_apptrust trust; > > I believe checkpatch warns if there's no empty line between the variable > declaration block and the rest of the code. It does give a warning. No empty lines on any of the other declarations though, but lets indeed add one here. > > Maybe it's just because this is an RFC, but for the final submission > please make sure you run checkpatch.pl --max-line-length=80. The > 80-character limit is not really a hard requirement anymore, but it's > still strongly preferred in net. > > > + memset(&trust, 0, sizeof(trust)); > > + err = ops->ieee_getapptrust(netdev, &trust); > > + if (!err && nla_put(skb, DCB_ATTR_IEEE_APP_TRUST, sizeof(trust), &trust)) > > + return -EMSGSIZE; > > + } > > + > > /* get peer info if available */ > > if (ops->ieee_peer_getets) { > > struct ieee_ets ets; > > @@ -1513,6 +1522,14 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh, > > } > > } > > > > + if (ieee[DCB_ATTR_IEEE_APP_TRUST] && ops->ieee_setapptrust) { > > Hmm, weird that none of the set requests are bounced if there's no set > callback. That way the request appears to succeed but doesn't actually > set anything. I find this very strange. > > Drivers that do not even have any DCB ops give -EOPNOTSUPP as expected. > I think lack of a particular op should do this as well. We can't change > this for the existing ones, but IMHO the new OPs should be like that. Agree. > > > + struct ieee_apptrust *trust = > > + nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]); > > Besides invoking the OP, this should validate the payload. E.g. no > driver is supposed to accept trust policies that contain invalid > selectors. Pretty sure there's no value in repeated entries either. Validation (bogus input and unique selectors) is done in userspace (dcb-apptrust). > > > + err = ops->ieee_setapptrust(netdev, trust); > > + if (err) > > + goto err; > > + } > > + > > err: > > err = nla_put_u8(skb, DCB_ATTR_IEEE, err); > > dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0); >
<Daniel.Machon@microchip.com> writes: > > Petr Machata <petrm@nvidia.com> writes: >> >> But of course this will never get anywhere close to that. We will end up >> passing maybe one, two entries. So the UAPI seems excessive in how it >> hands around this large array. >> >> I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE >> payload be an array of bytes, each byte a selector? Or something similar >> to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of >> payload attributes? > > Hmm. It might seem excessive, but a quick few thoughts on your proposed solution: > - We need more code to define and parse the new DCB_ATTR_IEEE_APP_TRUST_TABLE / > DCB_ATTR_IEEE_APP_TRUST attributes. Yes, a bit. But it's not too bad IMHO. Am I forgetting something here? u8 selectors[256]; int nselectors; int rem; nla_for_each_nested(attr, ieee[DCB_ATTR_DCB_APP_TRUST_TABLE], rem) { if (nla_type(attr) != DCB_ATTR_DCB_APP_TRUST || nla_len(attr) != 1 || nselectors >= sizeof(selectors)) { err = -EINVAL; goto err; } selectors[nselectors++] = nla_get_u8(attr); } ... and you have reconstructed the array. > - If the selectors are passed individually to the driver, we need a > dcbnl_delapptrust(), because now, the driver have to add and del from the > driver maintained array. You could of course accumulate selectors in an array > before passing them to the driver, but then why go away from the array in the > first place. I have no problem with using an array for the in-kernel API. There it's easy to change. UAPI can't ever change. >> > + struct ieee_apptrust *trust = >> > + nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]); >> >> Besides invoking the OP, this should validate the payload. E.g. no >> driver is supposed to accept trust policies that contain invalid >> selectors. Pretty sure there's no value in repeated entries either. > > Validation (bogus input and unique selectors) is done in userspace (dcb-apptrust). Using iproute2 dcb is not mandatory, the UAPI is client-agnostic. The kernel needs to bounce bogons as well. Otherwise they will become part of the UAPI with the meaning "this doesn't do anything". And yeah, drivers will validate supported configurations. But still the requests that go to the driver should already be sanitized, so that the driver code doesn't need to worry about this.
Hi Petr, > <Daniel.Machon@microchip.com> writes: > > > > Petr Machata <petrm@nvidia.com> writes: > >> > >> But of course this will never get anywhere close to that. We will end up > >> passing maybe one, two entries. So the UAPI seems excessive in how it > >> hands around this large array. > >> > >> I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE > >> payload be an array of bytes, each byte a selector? Or something similar > >> to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of > >> payload attributes? > > > > Hmm. It might seem excessive, but a quick few thoughts on your proposed solution: > > - We need more code to define and parse the new DCB_ATTR_IEEE_APP_TRUST_TABLE / > > DCB_ATTR_IEEE_APP_TRUST attributes. > > Yes, a bit. But it's not too bad IMHO. Am I forgetting something here? > > u8 selectors[256]; > int nselectors; > int rem; > > nla_for_each_nested(attr, ieee[DCB_ATTR_DCB_APP_TRUST_TABLE], rem) { > if (nla_type(attr) != DCB_ATTR_DCB_APP_TRUST || > nla_len(attr) != 1 || > nselectors >= sizeof(selectors)) { > err = -EINVAL; > goto err; > } > > selectors[nselectors++] = nla_get_u8(attr); > } > > ... and you have reconstructed the array. LGTM. > > > - If the selectors are passed individually to the driver, we need a > > dcbnl_delapptrust(), because now, the driver have to add and del from the > > driver maintained array. You could of course accumulate selectors in an array > > before passing them to the driver, but then why go away from the array in the > > first place. > > I have no problem with using an array for the in-kernel API. There it's > easy to change. UAPI can't ever change. > > >> > + struct ieee_apptrust *trust = > >> > + nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]); > >> > >> Besides invoking the OP, this should validate the payload. E.g. no > >> driver is supposed to accept trust policies that contain invalid > >> selectors. Pretty sure there's no value in repeated entries either. > > > > Validation (bogus input and unique selectors) is done in userspace (dcb-apptrust). > > Using iproute2 dcb is not mandatory, the UAPI is client-agnostic. The > kernel needs to bounce bogons as well. Otherwise they will become part > of the UAPI with the meaning "this doesn't do anything". > > And yeah, drivers will validate supported configurations. But still the > requests that go to the driver should already be sanitized, so that the > driver code doesn't need to worry about this. Good point. Will prepare a v2 with suggested changes.
diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h index 2b2d86fb3131..0c4b0107981d 100644 --- a/include/net/dcbnl.h +++ b/include/net/dcbnl.h @@ -61,6 +61,8 @@ struct dcbnl_rtnl_ops { int (*ieee_getapp) (struct net_device *, struct dcb_app *); int (*ieee_setapp) (struct net_device *, struct dcb_app *); int (*ieee_delapp) (struct net_device *, struct dcb_app *); + int (*ieee_setapptrust) (struct net_device *, struct ieee_apptrust *); + int (*ieee_getapptrust) (struct net_device *, struct ieee_apptrust *); int (*ieee_peer_getets) (struct net_device *, struct ieee_ets *); int (*ieee_peer_getpfc) (struct net_device *, struct ieee_pfc *); diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h index 8eab16e5bc13..833466dec096 100644 --- a/include/uapi/linux/dcbnl.h +++ b/include/uapi/linux/dcbnl.h @@ -248,6 +248,19 @@ struct dcb_app { __u16 protocol; }; +#define IEEE_8021QAZ_APP_SEL_MAX 255 + +/* This structure contains trust order extension to the IEEE 802.1Qaz APP + * managed object. + * + * @order: contains trust ordering of selector values for the IEEE 802.1Qaz + * APP managed object. Lower indexes has higher trust. + */ +struct ieee_apptrust { + __u8 num; + __u8 order[IEEE_8021QAZ_APP_SEL_MAX]; +}; + /** * struct dcb_peer_app_info - APP feature information sent by the peer * @@ -419,6 +432,7 @@ enum ieee_attrs { DCB_ATTR_IEEE_QCN, DCB_ATTR_IEEE_QCN_STATS, DCB_ATTR_DCB_BUFFER, + DCB_ATTR_IEEE_APP_TRUST, __DCB_ATTR_IEEE_MAX }; #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1) diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c index dc4fb699b56c..e87f0128c3bd 100644 --- a/net/dcb/dcbnl.c +++ b/net/dcb/dcbnl.c @@ -162,6 +162,7 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = { [DCB_ATTR_IEEE_ETS] = {.len = sizeof(struct ieee_ets)}, [DCB_ATTR_IEEE_PFC] = {.len = sizeof(struct ieee_pfc)}, [DCB_ATTR_IEEE_APP_TABLE] = {.type = NLA_NESTED}, + [DCB_ATTR_IEEE_APP_TRUST] = {.len = sizeof(struct ieee_apptrust)}, [DCB_ATTR_IEEE_MAXRATE] = {.len = sizeof(struct ieee_maxrate)}, [DCB_ATTR_IEEE_QCN] = {.len = sizeof(struct ieee_qcn)}, [DCB_ATTR_IEEE_QCN_STATS] = {.len = sizeof(struct ieee_qcn_stats)}, @@ -1133,6 +1134,14 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) spin_unlock_bh(&dcb_lock); nla_nest_end(skb, app); + if (ops->ieee_getapptrust) { + struct ieee_apptrust trust; + memset(&trust, 0, sizeof(trust)); + err = ops->ieee_getapptrust(netdev, &trust); + if (!err && nla_put(skb, DCB_ATTR_IEEE_APP_TRUST, sizeof(trust), &trust)) + return -EMSGSIZE; + } + /* get peer info if available */ if (ops->ieee_peer_getets) { struct ieee_ets ets; @@ -1513,6 +1522,14 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh, } } + if (ieee[DCB_ATTR_IEEE_APP_TRUST] && ops->ieee_setapptrust) { + struct ieee_apptrust *trust = + nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]); + err = ops->ieee_setapptrust(netdev, trust); + if (err) + goto err; + } + err: err = nla_put_u8(skb, DCB_ATTR_IEEE, err); dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0);
Add a new apptrust extension attribute to the 8021Qaz APP managed object. The new attribute is meant to allow drivers, whose hw supports the notion of trust, to be able to set whether a particular app selector is to be trusted - and also the order of precedence of selectors. A new structure ieee_apptrust has been created, which contains an array of selectors, where lower indexes has higher precedence. Signed-off-by: Daniel Machon <daniel.machon@microchip.com> --- include/net/dcbnl.h | 2 ++ include/uapi/linux/dcbnl.h | 14 ++++++++++++++ net/dcb/dcbnl.c | 17 +++++++++++++++++ 3 files changed, 33 insertions(+)