netdev - Re: [PATCH net-next v8 3/9] devlink: Enable creation of the devlink-rate nodes from the driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 Nov 2022 15:34:35 +0100
From:   "Wilczynski, Michal" <michal.wilczynski@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>
CC:     <netdev@...r.kernel.org>, <alexandr.lobakin@...el.com>,
        <jacob.e.keller@...el.com>, <jesse.brandeburg@...el.com>,
        <przemyslaw.kitszel@...el.com>, <anthony.l.nguyen@...el.com>,
        <kuba@...nel.org>, <ecree.xilinx@...il.com>
Subject: Re: [PATCH net-next v8 3/9] devlink: Enable creation of the
 devlink-rate nodes from the driver



On 10/31/2022 11:19 AM, Jiri Pirko wrote:
> Fri, Oct 28, 2022 at 12:51:37PM CEST, michal.wilczynski@...el.com wrote:
>> Intel 100G card internal firmware hierarchy for Hierarchicial QoS is very
>> rigid and can't be easily removed. This requires an ability to export
>> default hierarchy to allow user to modify it. Currently the driver is
>> only able to create the 'leaf' nodes, which usually represent the vport.
>> This is not enough for HQoS implemented in Intel hardware.
>>
>> Introduce new function devl_rate_node_create() that allows for creation
>> of the devlink-rate nodes from the driver.
>>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@...el.com>
>> ---
>> include/net/devlink.h |  4 ++++
>> net/core/devlink.c    | 49 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 53 insertions(+)
>>
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index 929cb72ef412..9d0a424712fd 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -98,6 +98,8 @@ struct devlink_port_attrs {
>> 	};
>> };
>>
>> +#define DEVLINK_RATE_NAME_MAX_LEN 30
>> +
>> struct devlink_rate {
>> 	struct list_head list;
>> 	enum devlink_rate_type type;
>> @@ -1601,6 +1603,8 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port,
>> 				   u32 controller, u16 pf, u32 sf,
>> 				   bool external);
>> int devl_rate_leaf_create(struct devlink_port *port, void *priv);
>> +int devl_rate_node_create(struct devlink *devlink, void *priv, char *node_name,
>> +			  char *parent_name);
>> void devl_rate_leaf_destroy(struct devlink_port *devlink_port);
>> void devl_rate_nodes_destroy(struct devlink *devlink);
>> void devlink_port_linecard_set(struct devlink_port *devlink_port,
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index b97c077cf66e..08f1bbd54c43 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -10270,6 +10270,55 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 contro
>> }
>> EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_sf_set);
>>
>> +/**
>> + * devl_rate_node_create - create devlink rate node
>> + * @devlink: devlink instance
>> + * @priv: driver private data
>> + * @node_name: name of the resulting node
>> + * @parent_name: name of the parent node
>> + *
>> + * Create devlink rate object of type node
>> + */
>> +int devl_rate_node_create(struct devlink *devlink, void *priv, char *node_name, char *parent_name)
> Nope, this is certainly incorrect. Do not refer to kernel object by
> string. You also don't have internal kernel api based on ifname to refer
> to struct net_device instance.
>
> Please have "struct devlink_rate *parent" to refer to parent node and
> make this function return "struct devlink_rate *".

Okay, I changed that and re-sent. The downside is I have to
store devlink_rate pointers in the driver instead of just names.

>
>
>> +{
>> +	struct devlink_rate *rate_node;
>> +	struct devlink_rate *parent;
>> +
>> +	rate_node = devlink_rate_node_get_by_name(devlink, node_name);
>> +	if (!IS_ERR(rate_node))
>> +		return -EEXIST;
>> +
>> +	rate_node = kzalloc(sizeof(*rate_node), GFP_KERNEL);
>> +	if (!rate_node)
>> +		return -ENOMEM;
>> +
>> +	if (parent_name) {
>> +		parent = devlink_rate_node_get_by_name(devlink, parent_name);
>> +		if (IS_ERR(parent)) {
>> +			kfree(rate_node);
>> +			return -ENODEV;
>> +		}
>> +		rate_node->parent = parent;
>> +		refcount_inc(&rate_node->parent->refcnt);
>> +	}
>> +
>> +	rate_node->type = DEVLINK_RATE_TYPE_NODE;
>> +	rate_node->devlink = devlink;
>> +	rate_node->priv = priv;
>> +
>> +	rate_node->name = kstrndup(node_name, DEVLINK_RATE_NAME_MAX_LEN, GFP_KERNEL);
> Why do you limit the name length? We don't limit the length passed from
> user, I see no reason to do it for driver.

I thought it's safer to limit this to avoid buffer overflow.
Changed this in v9.

>
>
>> +	if (!rate_node->name) {
>> +		kfree(rate_node);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	refcount_set(&rate_node->refcnt, 1);
>> +	list_add(&rate_node->list, &devlink->rate_list);
>> +	devlink_rate_notify(rate_node, DEVLINK_CMD_RATE_NEW);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devl_rate_node_create);
>> +
>> /**
>>   * devl_rate_leaf_create - create devlink rate leaf
>>   * @devlink_port: devlink port object to create rate object on
>> -- 
>> 2.37.2
>>