* [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack
@ 2025-07-09 14:59 Arnd Bergmann
2025-07-10 0:45 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2025-07-09 14:59 UTC (permalink / raw)
To: Jiri Pirko, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mark Bloch, Tariq Toukan, Carolina Jubran
Cc: Arnd Bergmann, Simon Horman, Cosmin Ratiu, Przemek Kitszel,
netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack
2025-07-09 14:59 [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack Arnd Bergmann
@ 2025-07-10 0:45 ` Jakub Kicinski
2025-07-10 7:58 ` Carolina Jubran
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2025-07-10 0:45 UTC (permalink / raw)
To: Arnd Bergmann, Carolina Jubran
Cc: Jiri Pirko, David S. Miller, Eric Dumazet, Paolo Abeni,
Mark Bloch, Tariq Toukan, Arnd Bergmann, Simon Horman,
Cosmin Ratiu, Przemek Kitszel, netdev, linux-kernel
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack
2025-07-10 0:45 ` Jakub Kicinski
@ 2025-07-10 7:58 ` Carolina Jubran
2025-07-13 12:28 ` Carolina Jubran
0 siblings, 1 reply; 6+ messages in thread
From: Carolina Jubran @ 2025-07-10 7:58 UTC (permalink / raw)
To: Jakub Kicinski, Arnd Bergmann
Cc: Jiri Pirko, David S. Miller, Eric Dumazet, Paolo Abeni,
Mark Bloch, Tariq Toukan, Arnd Bergmann, Simon Horman,
Cosmin Ratiu, Przemek Kitszel, netdev, linux-kernel
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack
2025-07-10 7:58 ` Carolina Jubran
@ 2025-07-13 12:28 ` Carolina Jubran
2025-07-14 15:27 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Carolina Jubran @ 2025-07-13 12:28 UTC (permalink / raw)
To: Jakub Kicinski, Arnd Bergmann
Cc: Jiri Pirko, David S. Miller, Eric Dumazet, Paolo Abeni,
Mark Bloch, Tariq Toukan, Arnd Bergmann, Simon Horman,
Cosmin Ratiu, Przemek Kitszel, netdev, linux-kernel
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;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack
2025-07-13 12:28 ` Carolina Jubran
@ 2025-07-14 15:27 ` Jakub Kicinski
2025-07-14 18:29 ` Carolina Jubran
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2025-07-14 15:27 UTC (permalink / raw)
To: Carolina Jubran
Cc: Arnd Bergmann, Jiri Pirko, David S. Miller, Eric Dumazet,
Paolo Abeni, Mark Bloch, Tariq Toukan, Arnd Bergmann,
Simon Horman, Cosmin Ratiu, Przemek Kitszel, netdev, linux-kernel
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack
2025-07-14 15:27 ` Jakub Kicinski
@ 2025-07-14 18:29 ` Carolina Jubran
0 siblings, 0 replies; 6+ messages in thread
From: Carolina Jubran @ 2025-07-14 18:29 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Arnd Bergmann, Jiri Pirko, David S. Miller, Eric Dumazet,
Paolo Abeni, Mark Bloch, Tariq Toukan, Arnd Bergmann,
Simon Horman, Cosmin Ratiu, Przemek Kitszel, netdev, linux-kernel
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-14 18:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-09 14:59 [PATCH] [v2] devlink: move DEVLINK_ATTR_MAX-sized array off stack Arnd Bergmann
2025-07-10 0:45 ` Jakub Kicinski
2025-07-10 7:58 ` Carolina Jubran
2025-07-13 12:28 ` Carolina Jubran
2025-07-14 15:27 ` Jakub Kicinski
2025-07-14 18:29 ` Carolina Jubran
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).