* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.