Re: [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack

[Carolina Jubran <cjubran@nvidia.com>]

On 10/07/2025 3:45, Jakub Kicinski wrote:
> On Wed,  9 Jul 2025 16:59:00 +0200 Arnd Bergmann wrote:
>> -	struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
>> +	struct nlattr **tb __free(kfree) = NULL;
> 
> Ugh, now you triggered me.
> 
>>   	u8 tc_index;
>>   	int err;
>>   
>> +	tb = kcalloc(DEVLINK_ATTR_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL);
>> +	if (!tb)
>> +		return -ENOMEM;
> 
> Cramming all the attributes in a single space is silly, it's better for
> devlink to grow up :/ Carolina could you test this?
> 

Sure, testing it. Will update.
Thanks!

> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
> index 1c4bb0cbe5f0..3d75bc530b30 100644
> --- a/Documentation/netlink/specs/devlink.yaml
> +++ b/Documentation/netlink/specs/devlink.yaml
> @@ -853,18 +853,6 @@ doc: Partial family for Devlink.
>           type: nest
>           multi-attr: true
>           nested-attributes: dl-rate-tc-bws
> -      -
> -        name: rate-tc-index
> -        type: u8
> -        checks:
> -          max: rate-tc-index-max
> -      -
> -        name: rate-tc-bw
> -        type: u32
> -        doc: |
> -             Specifies the bandwidth share assigned to the Traffic Class.
> -             The bandwidth for the traffic class is determined
> -             in proportion to the sum of the shares of all configured classes.
>     -
>       name: dl-dev-stats
>       subset-of: devlink
> @@ -1271,12 +1259,20 @@ doc: Partial family for Devlink.
>           type: flag
>     -
>       name: dl-rate-tc-bws
> -    subset-of: devlink
> +    name-prefix: devlink-attr-
>       attributes:
>         -
>           name: rate-tc-index
> +        type: u8
> +        checks:
> +          max: rate-tc-index-max
>         -
>           name: rate-tc-bw
> +        type: u32
> +        doc: |
> +             Specifies the bandwidth share assigned to the Traffic Class.
> +             The bandwidth for the traffic class is determined
> +             in proportion to the sum of the shares of all configured classes.
>   
>   operations:
>     enum-model: directional
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index e72bcc239afd..169a07499556 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -635,8 +635,6 @@ enum devlink_attr {
>   	DEVLINK_ATTR_REGION_DIRECT,		/* flag */
>   
>   	DEVLINK_ATTR_RATE_TC_BWS,		/* nested */
> -	DEVLINK_ATTR_RATE_TC_INDEX,		/* u8 */
> -	DEVLINK_ATTR_RATE_TC_BW,		/* u32 */
>   
>   	/* Add new attributes above here, update the spec in
>   	 * Documentation/netlink/specs/devlink.yaml and re-generate
> @@ -647,6 +645,14 @@ enum devlink_attr {
>   	DEVLINK_ATTR_MAX = __DEVLINK_ATTR_MAX - 1
>   };
>   
> +enum {
> +	DEVLINK_ATTR_RATE_TC_INDEX = 1,		/* u8 */
> +	DEVLINK_ATTR_RATE_TC_BW,		/* u32 */
> +
> +	__DEVLINK_ATTR_RATE_TC_MAX,
> +	DEVLINK_ATTR_RATE_TC_MAX = __DEVLINK_ATTR_RATE_TC_MAX - 1
> +};
> +
>   /* Mapping between internal resource described by the field and system
>    * structure
>    */
> diff --git a/net/devlink/rate.c b/net/devlink/rate.c
> index d39300a9b3d4..83ca62ce6c63 100644
> --- a/net/devlink/rate.c
> +++ b/net/devlink/rate.c
> @@ -346,11 +346,11 @@ static int devlink_nl_rate_tc_bw_parse(struct nlattr *parent_nest, u32 *tc_bw,
>   				       unsigned long *bitmap,
>   				       struct netlink_ext_ack *extack)
>   {
> -	struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
> +	struct nlattr *tb[DEVLINK_ATTR_RATE_TC_MAX + 1];
>   	u8 tc_index;
>   	int err;
>   
> -	err = nla_parse_nested(tb, DEVLINK_ATTR_MAX, parent_nest,
> +	err = nla_parse_nested(tb, DEVLINK_ATTR_RATE_TC_MAX, parent_nest,
>   			       devlink_dl_rate_tc_bws_nl_policy, extack);
>   	if (err)
>   		return err;