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

[Carolina Jubran <cjubran@nvidia.com>]

On 10/07/2025 10:58, Carolina Jubran wrote:
> 
> 
> 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!
> 

I have tested and it looks good. 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-

Maybe use name-prefix: devlink-attr-rate-tc- instead?

>>       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;
>