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

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

Posted by Arnd Bergmann 7 months ago

From: Arnd Bergmann <arnd@arndb.de>

There are many possible devlink attributes, so having an array of them
on the stack can cause a warning for excessive stack usage:

net/devlink/rate.c: In function 'devlink_nl_rate_tc_bw_parse':
net/devlink/rate.c:382:1: error: the frame size of 1648 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]

Change this to dynamic allocation instead.

Fixes: 566e8f108fc7 ("devlink: Extend devlink rate API with traffic classes bandwidth management")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I see that only two of the many array entries are actually used in this
function: DEVLINK_ATTR_RATE_TC_INDEX and DEVLINK_ATTR_RATE_TC_BW. If there
is an interface to extract just a single entry, using that would be
a little easier than the kcalloc().

v2: use __free() helper to simplify cleanup
---
 net/devlink/rate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/devlink/rate.c b/net/devlink/rate.c
index d39300a9b3d4..3c4689c6cefb 100644
--- a/net/devlink/rate.c
+++ b/net/devlink/rate.c
@@ -346,10 +346,14 @@ 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 __free(kfree) = NULL;
 	u8 tc_index;
 	int err;
 
+	tb = kcalloc(DEVLINK_ATTR_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL);
+	if (!tb)
+		return -ENOMEM;
+
 	err = nla_parse_nested(tb, DEVLINK_ATTR_MAX, parent_nest,
 			       devlink_dl_rate_tc_bws_nl_policy, extack);
 	if (err)
-- 
2.39.5

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

Posted by Jakub Kicinski 7 months ago

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?

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;

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

Posted by Carolina Jubran 7 months ago


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;

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

Posted by Carolina Jubran 6 months, 4 weeks ago


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

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

Posted by Jakub Kicinski 6 months, 4 weeks ago

On Sun, 13 Jul 2025 15:28:11 +0300 Carolina Jubran wrote:
> > Sure, testing it. Will update.
> 
> I have tested and it looks good. Thanks!

Awesome, would you be willing to post the official patch?
Add my as "suggested-by" and with my sign off.
Most of the work here was the testing :)

> >> 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
> >> @@ -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?

Sounds good!

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

Posted by Carolina Jubran 6 months, 3 weeks ago


On 14/07/2025 18:27, Jakub Kicinski wrote:
> On Sun, 13 Jul 2025 15:28:11 +0300 Carolina Jubran wrote:
>>> Sure, testing it. Will update.
>>
>> I have tested and it looks good. Thanks!
> 
> Awesome, would you be willing to post the official patch?
> Add my as "suggested-by" and with my sign off.
> Most of the work here was the testing :)
> 

Sure, I will post it soon.
>>>> 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
>>>> @@ -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?
> 
> Sounds good!