Re: [PATCH 1/3] [NET-NEXT]: Add DCB netlink interface definition

[Patrick McHardy <kaber@trash.net>]

PJ Waskiewicz wrote:
> +/**
> + * enum dcbnl_perm_hwaddr_attrs - DCB Permanent HW Address nested attributes
> + *
> + * @DCB_PERM_HW_ATTR_UNDEFINED: unspecified attribute to catch errors
> + * @DCB_PERM_HW_ATTR_0: MAC address from receive address 0 (NLA_U8)
> + * @DCB_PERM_HW_ATTR_1: MAC address from receive address 1 (NLA_U8)
> + * @DCB_PERM_HW_ATTR_2: MAC address from receive address 2 (NLA_U8)
> + * @DCB_PERM_HW_ATTR_3: MAC address from receive address 3 (NLA_U8)
> + * @DCB_PERM_HW_ATTR_4: MAC address from receive address 4 (NLA_U8)
> + * @DCB_PERM_HW_ATTR_5: MAC address from receive address 5 (NLA_U8)
> + * @DCB_PERM_HW_ATTR_ALL: apply to all MAC addresses (NLA_FLAG)
> + *
> + * These attributes are used when bonding DCB interfaces together.
> + *
> + */

For these and the other numbered attributes: is the maximum number
fixed and/or defined somewhere? If not, I'd suggest to use lists
of attributes.

> +/**
> + * enum dcbnl_pg_attrs - DCB Priority Group attributes
> + *
> + * @DCB_PG_ATTR_UNDEFINED: unspecified attribute to catch errors
> + * @DCB_PG_ATTR_TC_0: Priority Group Traffic Class 0 configuration (NLA_NESTED)
> + * @DCB_PG_ATTR_TC_1: Priority Group Traffic Class 1 configuration (NLA_NESTED)
> + * @DCB_PG_ATTR_TC_2: Priority Group Traffic Class 2 configuration (NLA_NESTED)
> + * @DCB_PG_ATTR_TC_3: Priority Group Traffic Class 3 configuration (NLA_NESTED)
> + * @DCB_PG_ATTR_TC_4: Priority Group Traffic Class 4 configuration (NLA_NESTED)
> + * @DCB_PG_ATTR_TC_5: Priority Group Traffic Class 5 configuration (NLA_NESTED)
> + * @DCB_PG_ATTR_TC_6: Priority Group Traffic Class 6 configuration (NLA_NESTED)
> + * @DCB_PG_ATTR_TC_7: Priority Group Traffic Class 7 configuration (NLA_NESTED)
> + * @DCB_PG_ATTR_TC_MAX: highest attribute number currently defined
> + * @DCB_PG_ATTR_TC_ALL: apply to all traffic classes (NLA_NESTED)
> + * @DCB_PG_ATTR_BWG_0: Bandwidth group 0 configuration (NLA_U8)
> + * @DCB_PG_ATTR_BWG_1: Bandwidth group 1 configuration (NLA_U8)
> + * @DCB_PG_ATTR_BWG_2: Bandwidth group 2 configuration (NLA_U8)
> + * @DCB_PG_ATTR_BWG_3: Bandwidth group 3 configuration (NLA_U8)
> + * @DCB_PG_ATTR_BWG_4: Bandwidth group 4 configuration (NLA_U8)
> + * @DCB_PG_ATTR_BWG_5: Bandwidth group 5 configuration (NLA_U8)
> + * @DCB_PG_ATTR_BWG_6: Bandwidth group 6 configuration (NLA_U8)
> + * @DCB_PG_ATTR_BWG_7: Bandwidth group 7 configuration (NLA_U8)
> + * @DCB_PG_ATTR_BWG_MAX: highest attribute number currently defined
> + * @DCB_PG_ATTR_BWG_ALL: apply to all bandwidth groups (NLA_FLAG)

And in this case lists of nested attributes consisting of
Priority and Bandwidth, since they seem to belong together.

> +struct dcbnl_genl_ops {
> +	u8   (*getstate)(struct net_device *);
> +	void (*setstate)(struct net_device *, u8);
> +	void (*getpermhwaddr)(struct net_device *, u8 *);

"getpermhwaddr" doesn't seem to belong in this interface but
in rtnetlink and/or ethtool instead.

> +static int dcbnl_getperm_hwaddr(struct sk_buff *skb, struct genl_info *info)
> +{
> ...
> +	dcbnl_skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!dcbnl_skb)
> +		goto err_out;
> ...
> +err:
> +	kfree(dcbnl_skb);

^^^ kfree_skb

The same error is present multiple times

> +static int __dcbnl_pg_setcfg(struct genl_info *info, int dir)
> +{
> +	struct net_device *netdev = NULL;
> +	struct nlattr *pg_tb[DCB_PG_ATTR_MAX + 1];
> +	struct nlattr *param_tb[DCB_TC_ATTR_PARAM_MAX + 1];
> +	int ret = -EINVAL;
> +	int i;
> +	u8 prio = 0, bwg_id = 0, bw_pct = 0, up_map = 0;
> +
> +	if (!info->attrs[DCB_ATTR_IFNAME] || !info->attrs[DCB_ATTR_PG_CFG])
> +		return ret;
> +
> +	netdev = dev_get_by_name(&init_net,
> +				 nla_data(info->attrs[DCB_ATTR_IFNAME]));


The fact that you do this in every handler makes me wonder whether
rtnetlink wouldn't be the better choice, if only because it uses
the rtnl_mutex and configuration changes are thus serialized with
other networking configuration changes.

For example I don't see anything preventing concurrent changes
to the DCB configuration while it is copied between the temporary
configuration and the real one. In one cases its done in a
path holding the rtnl_mutex, in another case its done with
holding the genl_mutex in a genetlink callback.