netdev - Re: [PATCH] [v2] devlink: move DEVLINK_ATTR

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]
Message-ID: <5ca43852-0586-4811-bc45-99e19232ce9d@nvidia.com>
Date: Sun, 13 Jul 2025 15:28:11 +0300
From: Carolina Jubran <cjubran@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>, Arnd Bergmann <arnd@...nel.org>
Cc: Jiri Pirko <jiri@...nulli.us>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Mark Bloch <mbloch@...dia.com>, Tariq Toukan <tariqt@...dia.com>,
 Arnd Bergmann <arnd@...db.de>, Simon Horman <horms@...nel.org>,
 Cosmin Ratiu <cratiu@...dia.com>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack



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