Re: [PATCH v10 net-next 3/6] devlink: Implement devlink param multi attribute nested data values

[Paolo Abeni <pabeni@redhat.com>]

On 4/3/26 4:55 AM, Ratheesh Kannoth wrote:
> From: Saeed Mahameed <saeedm@nvidia.com>
> 
> Devlink param value attribute is not defined since devlink is handling
> the value validating and parsing internally, this allows us to implement
> multi attribute values without breaking any policies.
> 
> Devlink param multi-attribute values are considered to be dynamically
> sized arrays of u64 values, by introducing a new devlink param type
> DEVLINK_PARAM_TYPE_U64_ARRAY, driver and user space can set a variable
> count of u32 values into the DEVLINK_ATTR_PARAM_VALUE_DATA attribute.
> 
> Implement get/set parsing and add to the internal value structure passed
> to drivers.
> 
> This is useful for devices that need to configure a list of values for
> a specific configuration.
> 
> example:
> $ devlink dev param show pci/... name multi-value-param
> name multi-value-param type driver-specific
> values:
> cmode permanent value: 0,1,2,3,4,5,6,7
> 
> $ devlink dev param set pci/... name multi-value-param \
> 		value 4,5,6,7,0,1,2,3 cmode permanent
> 
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>
> ---
>  Documentation/netlink/specs/devlink.yaml |  4 ++
>  include/net/devlink.h                    |  8 +++
>  include/uapi/linux/devlink.h             |  1 +
>  net/devlink/netlink_gen.c                |  2 +
>  net/devlink/param.c                      | 91 +++++++++++++++++++-----
>  5 files changed, 89 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
> index b495d56b9137..b619de4fe08a 100644
> --- a/Documentation/netlink/specs/devlink.yaml
> +++ b/Documentation/netlink/specs/devlink.yaml
> @@ -226,6 +226,10 @@ definitions:
>          value: 10
>        -
>          name: binary
> +      -
> +        name: u64-array
> +        value: 129
> +
>    -
>      name: rate-tc-index-max
>      type: const
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 3038af6ec017..3a355fea8189 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -432,6 +432,13 @@ enum devlink_param_type {
>  	DEVLINK_PARAM_TYPE_U64 = DEVLINK_VAR_ATTR_TYPE_U64,
>  	DEVLINK_PARAM_TYPE_STRING = DEVLINK_VAR_ATTR_TYPE_STRING,
>  	DEVLINK_PARAM_TYPE_BOOL = DEVLINK_VAR_ATTR_TYPE_FLAG,
> +	DEVLINK_PARAM_TYPE_U64_ARRAY = DEVLINK_VAR_ATTR_TYPE_U64_ARRAY,
> +};
> +
> +#define __DEVLINK_PARAM_MAX_ARRAY_SIZE 32
> +struct devlink_param_u64_array {
> +	u64 size;
> +	u64 val[__DEVLINK_PARAM_MAX_ARRAY_SIZE];
>  };
>  
>  union devlink_param_value {
> @@ -441,6 +448,7 @@ union devlink_param_value {
>  	u64 vu64;
>  	char vstr[__DEVLINK_PARAM_MAX_STRING_VALUE];
>  	bool vbool;
> +	struct devlink_param_u64_array u64arr;

Sashiko as a couple of relevant remarks here, specifically:

---
Does this increase the size of union devlink_param_value from 32 bytes
to over 264 bytes?
Looking at existing functions like devlink_nl_param_value_fill_one() and
devlink_nl_param_value_put(), they take multiple copies of this union by
value. Passing two of these unions by value consumes over 528 bytes of
stack space, and combined in a call chain this pushes nearly 800 bytes
of arguments onto the stack.
Could this create a risk of hitting CONFIG_FRAME_WARN limits deep in
driver notification contexts? Should the signatures of the internal
functions and exported APIs be updated to pass the unions by pointer
instead?
---

>  };
>  
>  struct devlink_param_gset_ctx {
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index 7de2d8cc862f..5332223dd6d0 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -406,6 +406,7 @@ enum devlink_var_attr_type {
>  	DEVLINK_VAR_ATTR_TYPE_BINARY,
>  	__DEVLINK_VAR_ATTR_TYPE_CUSTOM_BASE = 0x80,
>  	/* Any possible custom types, unrelated to NLA_* values go below */
> +	DEVLINK_VAR_ATTR_TYPE_U64_ARRAY,
>  };
>  
>  enum devlink_attr {
> diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c
> index eb35e80e01d1..7aaf462f27ee 100644
> --- a/net/devlink/netlink_gen.c
> +++ b/net/devlink/netlink_gen.c
> @@ -37,6 +37,8 @@ devlink_attr_param_type_validate(const struct nlattr *attr,
>  	case DEVLINK_VAR_ATTR_TYPE_NUL_STRING:
>  		fallthrough;
>  	case DEVLINK_VAR_ATTR_TYPE_BINARY:
> +		fallthrough;
> +	case DEVLINK_VAR_ATTR_TYPE_U64_ARRAY:
>  		return 0;
>  	}
>  	NL_SET_ERR_MSG_ATTR(extack, attr, "invalid enum value");
> diff --git a/net/devlink/param.c b/net/devlink/param.c
> index cf95268da5b0..2ec85dffd8ac 100644
> --- a/net/devlink/param.c
> +++ b/net/devlink/param.c
> @@ -252,6 +252,14 @@ devlink_nl_param_value_put(struct sk_buff *msg, enum devlink_param_type type,
>  				return -EMSGSIZE;
>  		}
>  		break;
> +	case DEVLINK_PARAM_TYPE_U64_ARRAY:
> +		if (val.u64arr.size > __DEVLINK_PARAM_MAX_ARRAY_SIZE)
> +			return -EMSGSIZE;
> +
> +		for (int i = 0; i < val.u64arr.size; i++)
> +			if (nla_put_uint(msg, nla_type, val.u64arr.val[i]))
> +				return -EMSGSIZE;
> +		break;
>  	}
>  	return 0;
>  }
> @@ -304,56 +312,78 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
>  				 u32 portid, u32 seq, int flags,
>  				 struct netlink_ext_ack *extack)
>  {
> -	union devlink_param_value default_value[DEVLINK_PARAM_CMODE_MAX + 1];
> -	union devlink_param_value param_value[DEVLINK_PARAM_CMODE_MAX + 1];
>  	bool default_value_set[DEVLINK_PARAM_CMODE_MAX + 1] = {};
>  	bool param_value_set[DEVLINK_PARAM_CMODE_MAX + 1] = {};
>  	const struct devlink_param *param = param_item->param;
> -	struct devlink_param_gset_ctx ctx;
> +	union devlink_param_value *default_value;
> +	union devlink_param_value *param_value;
> +	struct devlink_param_gset_ctx *ctx;
>  	struct nlattr *param_values_list;
>  	struct nlattr *param_attr;
>  	void *hdr;
>  	int err;
>  	int i;
>  
> +	default_value = kcalloc(DEVLINK_PARAM_CMODE_MAX + 1,
> +				sizeof(*default_value), GFP_KERNEL);
> +	if (!default_value)
> +		return -ENOMEM;
> +
> +	param_value = kcalloc(DEVLINK_PARAM_CMODE_MAX + 1,
> +			      sizeof(*param_value), GFP_KERNEL);
> +	if (!param_value) {
> +		kfree(default_value);
> +		return -ENOMEM;
> +	}
> +
> +	ctx = kmalloc_obj(*ctx);
> +	if (!ctx) {
> +		kfree(param_value);
> +		kfree(default_value);
> +		return -ENOMEM;
> +	}
> +
>  	/* Get value from driver part to driverinit configuration mode */
>  	for (i = 0; i <= DEVLINK_PARAM_CMODE_MAX; i++) {
>  		if (!devlink_param_cmode_is_supported(param, i))
>  			continue;
>  		if (i == DEVLINK_PARAM_CMODE_DRIVERINIT) {
> -			if (param_item->driverinit_value_new_valid)
> +			if (param_item->driverinit_value_new_valid) {
>  				param_value[i] = param_item->driverinit_value_new;
> -			else if (param_item->driverinit_value_valid)
> +			} else if (param_item->driverinit_value_valid) {
>  				param_value[i] = param_item->driverinit_value;
> -			else
> -				return -EOPNOTSUPP;
> +			} else {
> +				err = -EOPNOTSUPP;
> +				goto get_put_fail;
> +			}
>  
>  			if (param_item->driverinit_value_valid) {
>  				default_value[i] = param_item->driverinit_default;
>  				default_value_set[i] = true;
>  			}
>  		} else {
> -			ctx.cmode = i;
> -			err = devlink_param_get(devlink, param, &ctx, extack);
> +			ctx->cmode = i;
> +			err = devlink_param_get(devlink, param, ctx, extack);
>  			if (err)
> -				return err;
> -			param_value[i] = ctx.val;
> +				goto get_put_fail;
> +			param_value[i] = ctx->val;
>  
> -			err = devlink_param_get_default(devlink, param, &ctx,
> +			err = devlink_param_get_default(devlink, param, ctx,
>  							extack);
>  			if (!err) {
> -				default_value[i] = ctx.val;
> +				default_value[i] = ctx->val;
>  				default_value_set[i] = true;
>  			} else if (err != -EOPNOTSUPP) {
> -				return err;
> +				goto get_put_fail;
>  			}
>  		}
>  		param_value_set[i] = true;
>  	}
>  
> +	err = -EMSGSIZE;
>  	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd);
>  	if (!hdr)
> -		return -EMSGSIZE;
> +		goto get_put_fail;
>  
>  	if (devlink_nl_put_handle(msg, devlink))
>  		goto genlmsg_cancel;
> @@ -393,6 +423,9 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
>  	nla_nest_end(msg, param_values_list);
>  	nla_nest_end(msg, param_attr);
>  	genlmsg_end(msg, hdr);
> +	kfree(default_value);
> +	kfree(param_value);
> +	kfree(ctx);
>  	return 0;
>  
>  values_list_nest_cancel:
> @@ -401,7 +434,11 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
>  	nla_nest_cancel(msg, param_attr);
>  genlmsg_cancel:
>  	genlmsg_cancel(msg, hdr);
> -	return -EMSGSIZE;
> +get_put_fail:
> +	kfree(default_value);
> +	kfree(param_value);
> +	kfree(ctx);
> +	return err;
>  }
>  
>  static void devlink_param_notify(struct devlink *devlink,
> @@ -507,7 +544,7 @@ devlink_param_value_get_from_info(const struct devlink_param *param,
>  				  union devlink_param_value *value)
>  {
>  	struct nlattr *param_data;
> -	int len;
> +	int len, cnt, rem;
>  
>  	param_data = info->attrs[DEVLINK_ATTR_PARAM_VALUE_DATA];
>  
> @@ -547,6 +584,26 @@ devlink_param_value_get_from_info(const struct devlink_param *param,
>  			return -EINVAL;
>  		value->vbool = nla_get_flag(param_data);
>  		break;
> +
> +	case DEVLINK_PARAM_TYPE_U64_ARRAY:
> +		cnt = 0;
> +		nla_for_each_attr_type(param_data,
> +				       DEVLINK_ATTR_PARAM_VALUE_DATA,
> +				       genlmsg_data(info->genlhdr),
> +				       genlmsg_len(info->genlhdr), rem) {
> +			if (cnt >= __DEVLINK_PARAM_MAX_ARRAY_SIZE)
> +				return -EMSGSIZE;
> +
> +			if ((nla_len(param_data) != sizeof(u64)) &&
> +			    (nla_len(param_data) != sizeof(u32)))
> +				return -EINVAL;
> +
> +			value->u64arr.val[cnt] = (u64)nla_get_uint(param_data);
> +			cnt++;
> +		}
> +
> +		value->u64arr.size = cnt;
> +		break;

Sashiko says:

---
Does this make it impossible to set an empty array to clear a
multi-value parameter?
If userspace provides 0 elements, param_data will be NULL. Earlier in
devlink_param_value_get_from_info(), there is a check:
	param_data = info->attrs[DEVLINK_ATTR_PARAM_VALUE_DATA];
	if (param->type != DEVLINK_PARAM_TYPE_BOOL && !param_data)
		return -EINVAL;
If the parameter is a U64_ARRAY and no data is provided, this check will
immediately return -EINVAL.
The kernel can successfully emit an empty array on a GET request if the
size is 0. Should the SET path similarly support receiving 0 elements to
allow userspace to clear a multi-value parameter?
---

There are several others NIC-specific remarks, which IMHO are mostly
pre-existing issues, but please have a look:

https://sashiko.dev/#/patchset/20260403025533.6250-1-rkannoth%40marvell.com

/P