| Message ID | 20230112201554.752144-3-daniel.machon@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce new DCB rewrite table | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 1 this patch: 1 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 122 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Daniel Machon <daniel.machon@microchip.com> writes: > In preparation for DCB rewrite. Add a new function for setting and > deleting both app and rewrite entries. Moving this into a separate > function reduces duplicate code, as both type of entries requires the > same set of checks. The function will now iterate through a configurable > nested attribute (app or rewrite attr), validate each attribute and call > the appropriate set- or delete function. > > Note that this function always checks for nla_len(attr_itr) < > sizeof(struct dcb_app), which was only done in dcbnl_ieee_set and not in > dcbnl_ieee_del prior to this patch. This means, that any userspace tool > that used to shove in data < sizeof(struct dcb_app) would now receive > -ERANGE. Good. > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > --- > net/dcb/dcbnl.c | 104 +++++++++++++++++++++++------------------------- > 1 file changed, 49 insertions(+), 55 deletions(-) > > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c > index a76bdf6f0198..6d19564e19a8 100644 > --- a/net/dcb/dcbnl.c > +++ b/net/dcb/dcbnl.c > @@ -1099,6 +1099,45 @@ static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb) > return err; > } > > +/* Set or delete APP table or rewrite table entries. The APP struct is validated > + * and the appropriate callback function is called. > + */ > +static int dcbnl_apprewr_setdel(struct nlattr *attr, struct net_device *netdev, > + int (*setdel)(struct net_device *dev, > + struct dcb_app *app), > + int (*ops_setdel)(struct net_device *dev, > + struct dcb_app *app)) The name makes it look like it's rewrite-specific. Maybe make it _app_table_? Given both DCB app and DCB rewrite use app table as the database... Dunno. Not a big deal. > +{ > + struct dcb_app *app_data; > + enum ieee_attrs_app type; > + struct nlattr *attr_itr; > + int rem, err; > + > + nla_for_each_nested(attr_itr, attr, rem) { > + type = nla_type(attr_itr); > + > + if (!dcbnl_app_attr_type_validate(type)) > + continue; > + > + if (nla_len(attr_itr) < sizeof(struct dcb_app)) > + return -ERANGE; > + > + app_data = nla_data(attr_itr); > + > + if (!dcbnl_app_selector_validate(type, app_data->selector)) > + return -EINVAL; > + > + if (ops_setdel) > + err = ops_setdel(netdev, app_data); > + else > + err = setdel(netdev, app_data); > + if (err) > + return err; > + } > + > + return 0; > +} > + > /* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */ > static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) > { > @@ -1568,36 +1607,11 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh, > } > > if (ieee[DCB_ATTR_IEEE_APP_TABLE]) { > - struct nlattr *attr; > - int rem; > - > - nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) { > - enum ieee_attrs_app type = nla_type(attr); > - struct dcb_app *app_data; > - > - if (!dcbnl_app_attr_type_validate(type)) > - continue; > - > - if (nla_len(attr) < sizeof(struct dcb_app)) { > - err = -ERANGE; > - goto err; > - } > - > - app_data = nla_data(attr); > - > - if (!dcbnl_app_selector_validate(type, > - app_data->selector)) { > - err = -EINVAL; > - goto err; > - } > - > - if (ops->ieee_setapp) > - err = ops->ieee_setapp(netdev, app_data); > - else > - err = dcb_ieee_setapp(netdev, app_data); > - if (err) > - goto err; > - } > + err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE], > + netdev, dcb_ieee_setapp, > + ops->ieee_setapp); This could pre-resolve the callback to use and pass one pointer: err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE], netdev, ops->ieee_setapp ?: dcb_ieee_setapp); And the same below. > + if (err) > + goto err; > } > > if (ieee[DCB_ATTR_DCB_APP_TRUST_TABLE]) { > @@ -1684,31 +1698,11 @@ static int dcbnl_ieee_del(struct net_device *netdev, struct nlmsghdr *nlh, > return err; > > if (ieee[DCB_ATTR_IEEE_APP_TABLE]) { > - struct nlattr *attr; > - int rem; > - > - nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) { > - enum ieee_attrs_app type = nla_type(attr); > - struct dcb_app *app_data; > - > - if (!dcbnl_app_attr_type_validate(type)) > - continue; > - app_data = nla_data(attr); > - > - if (!dcbnl_app_selector_validate(type, > - app_data->selector)) { > - err = -EINVAL; > - goto err; > - } > - > - if (ops->ieee_delapp) > - err = ops->ieee_delapp(netdev, app_data); > - else > - err = dcb_ieee_delapp(netdev, app_data); > - if (err) > - goto err; > - } > + err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE], > + netdev, dcb_ieee_delapp, > + ops->ieee_delapp); > + if (err) > + goto err; > } > > err:
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > --- > > net/dcb/dcbnl.c | 104 +++++++++++++++++++++++------------------------- > > 1 file changed, 49 insertions(+), 55 deletions(-) > > > > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c > > index a76bdf6f0198..6d19564e19a8 100644 > > --- a/net/dcb/dcbnl.c > > +++ b/net/dcb/dcbnl.c > > @@ -1099,6 +1099,45 @@ static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb) > > return err; > > } > > > > +/* Set or delete APP table or rewrite table entries. The APP struct is validated > > + * and the appropriate callback function is called. > > + */ > > +static int dcbnl_apprewr_setdel(struct nlattr *attr, struct net_device *netdev, > > + int (*setdel)(struct net_device *dev, > > + struct dcb_app *app), > > + int (*ops_setdel)(struct net_device *dev, > > + struct dcb_app *app)) > > The name makes it look like it's rewrite-specific. Maybe make it > _app_table_? Given both DCB app and DCB rewrite use app table as the > database... Dunno. Not a big deal. Yeah, the other shared funcs are called _app_* too, so lets stick with that naming. Agreed. > > /* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */ > > static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) > > { > > @@ -1568,36 +1607,11 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh, > > } > > > > if (ieee[DCB_ATTR_IEEE_APP_TABLE]) { > > - struct nlattr *attr; > > - int rem; > > - > > - nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) { > > - enum ieee_attrs_app type = nla_type(attr); > > - struct dcb_app *app_data; > > - > > - if (!dcbnl_app_attr_type_validate(type)) > > - continue; > > - > > - if (nla_len(attr) < sizeof(struct dcb_app)) { > > - err = -ERANGE; > > - goto err; > > - } > > - > > - app_data = nla_data(attr); > > - > > - if (!dcbnl_app_selector_validate(type, > > - app_data->selector)) { > > - err = -EINVAL; > > - goto err; > > - } > > - > > - if (ops->ieee_setapp) > > - err = ops->ieee_setapp(netdev, app_data); > > - else > > - err = dcb_ieee_setapp(netdev, app_data); > > - if (err) > > - goto err; > > - } > > + err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE], > > + netdev, dcb_ieee_setapp, > > + ops->ieee_setapp); > > This could pre-resolve the callback to use and pass one pointer: > > err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE], netdev, > ops->ieee_setapp ?: dcb_ieee_setapp); Agreed. Will make the suggested changes in v2 - thank you Petr. /Daniel
diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c index a76bdf6f0198..6d19564e19a8 100644 --- a/net/dcb/dcbnl.c +++ b/net/dcb/dcbnl.c @@ -1099,6 +1099,45 @@ static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb) return err; } +/* Set or delete APP table or rewrite table entries. The APP struct is validated + * and the appropriate callback function is called. + */ +static int dcbnl_apprewr_setdel(struct nlattr *attr, struct net_device *netdev, + int (*setdel)(struct net_device *dev, + struct dcb_app *app), + int (*ops_setdel)(struct net_device *dev, + struct dcb_app *app)) +{ + struct dcb_app *app_data; + enum ieee_attrs_app type; + struct nlattr *attr_itr; + int rem, err; + + nla_for_each_nested(attr_itr, attr, rem) { + type = nla_type(attr_itr); + + if (!dcbnl_app_attr_type_validate(type)) + continue; + + if (nla_len(attr_itr) < sizeof(struct dcb_app)) + return -ERANGE; + + app_data = nla_data(attr_itr); + + if (!dcbnl_app_selector_validate(type, app_data->selector)) + return -EINVAL; + + if (ops_setdel) + err = ops_setdel(netdev, app_data); + else + err = setdel(netdev, app_data); + if (err) + return err; + } + + return 0; +} + /* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) { @@ -1568,36 +1607,11 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh, } if (ieee[DCB_ATTR_IEEE_APP_TABLE]) { - struct nlattr *attr; - int rem; - - nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) { - enum ieee_attrs_app type = nla_type(attr); - struct dcb_app *app_data; - - if (!dcbnl_app_attr_type_validate(type)) - continue; - - if (nla_len(attr) < sizeof(struct dcb_app)) { - err = -ERANGE; - goto err; - } - - app_data = nla_data(attr); - - if (!dcbnl_app_selector_validate(type, - app_data->selector)) { - err = -EINVAL; - goto err; - } - - if (ops->ieee_setapp) - err = ops->ieee_setapp(netdev, app_data); - else - err = dcb_ieee_setapp(netdev, app_data); - if (err) - goto err; - } + err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE], + netdev, dcb_ieee_setapp, + ops->ieee_setapp); + if (err) + goto err; } if (ieee[DCB_ATTR_DCB_APP_TRUST_TABLE]) { @@ -1684,31 +1698,11 @@ static int dcbnl_ieee_del(struct net_device *netdev, struct nlmsghdr *nlh, return err; if (ieee[DCB_ATTR_IEEE_APP_TABLE]) { - struct nlattr *attr; - int rem; - - nla_for_each_nested(attr, ieee[DCB_ATTR_IEEE_APP_TABLE], rem) { - enum ieee_attrs_app type = nla_type(attr); - struct dcb_app *app_data; - - if (!dcbnl_app_attr_type_validate(type)) - continue; - - app_data = nla_data(attr); - - if (!dcbnl_app_selector_validate(type, - app_data->selector)) { - err = -EINVAL; - goto err; - } - - if (ops->ieee_delapp) - err = ops->ieee_delapp(netdev, app_data); - else - err = dcb_ieee_delapp(netdev, app_data); - if (err) - goto err; - } + err = dcbnl_apprewr_setdel(ieee[DCB_ATTR_IEEE_APP_TABLE], + netdev, dcb_ieee_delapp, + ops->ieee_delapp); + if (err) + goto err; } err:
In preparation for DCB rewrite. Add a new function for setting and deleting both app and rewrite entries. Moving this into a separate function reduces duplicate code, as both type of entries requires the same set of checks. The function will now iterate through a configurable nested attribute (app or rewrite attr), validate each attribute and call the appropriate set- or delete function. Note that this function always checks for nla_len(attr_itr) < sizeof(struct dcb_app), which was only done in dcbnl_ieee_set and not in dcbnl_ieee_del prior to this patch. This means, that any userspace tool that used to shove in data < sizeof(struct dcb_app) would now receive -ERANGE. Signed-off-by: Daniel Machon <daniel.machon@microchip.com> --- net/dcb/dcbnl.c | 104 +++++++++++++++++++++++------------------------- 1 file changed, 49 insertions(+), 55 deletions(-)