From: Arnd Bergmann <[email protected]>
The tc_gred_qopt_offload structure has grown too big to be on the
stack for 32-bit architectures after recent changes.
net/sched/sch_gred.c:903:13: error: stack frame size (1180) exceeds limit (1024) in 'gred_destroy' [-Werror,-Wframe-larger-than]
net/sched/sch_gred.c:310:13: error: stack frame size (1212) exceeds limit (1024) in 'gred_offload' [-Werror,-Wframe-larger-than]
Use dynamic allocation per qdisc to avoid this.
Fixes: 50dc9a8572aa ("net: sched: Merge Qdisc::bstats and Qdisc::cpu_bstats data types")
Fixes: 67c9e6270f30 ("net: sched: Protect Qdisc::bstats with u64_stats")
Suggested-by: Jakub Kicinski <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
v2…v3:
- drop not needed return statement in gred_offload() (Jakub)
- use kzalloc(sizeof(*table->opt) in gred_init() (Eric)
- Make the allocation conditional on ->ndo_setup_tc (Jakub).
net/sched/sch_gred.c | 50 ++++++++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index 72de08ef8335e..1073c76d05c45 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -56,6 +56,7 @@ struct gred_sched {
u32 DPs;
u32 def;
struct red_vars wred_set;
+ struct tc_gred_qopt_offload *opt;
};
static inline int gred_wred_mode(struct gred_sched *table)
@@ -311,42 +312,43 @@ static void gred_offload(struct Qdisc *sch, enum tc_gred_command command)
{
struct gred_sched *table = qdisc_priv(sch);
struct net_device *dev = qdisc_dev(sch);
- struct tc_gred_qopt_offload opt = {
- .command = command,
- .handle = sch->handle,
- .parent = sch->parent,
- };
+ struct tc_gred_qopt_offload *opt = table->opt;
if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
return;
+ memset(opt, 0, sizeof(*opt));
+ opt->command = command;
+ opt->handle = sch->handle;
+ opt->parent = sch->parent;
+
if (command == TC_GRED_REPLACE) {
unsigned int i;
- opt.set.grio_on = gred_rio_mode(table);
- opt.set.wred_on = gred_wred_mode(table);
- opt.set.dp_cnt = table->DPs;
- opt.set.dp_def = table->def;
+ opt->set.grio_on = gred_rio_mode(table);
+ opt->set.wred_on = gred_wred_mode(table);
+ opt->set.dp_cnt = table->DPs;
+ opt->set.dp_def = table->def;
for (i = 0; i < table->DPs; i++) {
struct gred_sched_data *q = table->tab[i];
if (!q)
continue;
- opt.set.tab[i].present = true;
- opt.set.tab[i].limit = q->limit;
- opt.set.tab[i].prio = q->prio;
- opt.set.tab[i].min = q->parms.qth_min >> q->parms.Wlog;
- opt.set.tab[i].max = q->parms.qth_max >> q->parms.Wlog;
- opt.set.tab[i].is_ecn = gred_use_ecn(q);
- opt.set.tab[i].is_harddrop = gred_use_harddrop(q);
- opt.set.tab[i].probability = q->parms.max_P;
- opt.set.tab[i].backlog = &q->backlog;
+ opt->set.tab[i].present = true;
+ opt->set.tab[i].limit = q->limit;
+ opt->set.tab[i].prio = q->prio;
+ opt->set.tab[i].min = q->parms.qth_min >> q->parms.Wlog;
+ opt->set.tab[i].max = q->parms.qth_max >> q->parms.Wlog;
+ opt->set.tab[i].is_ecn = gred_use_ecn(q);
+ opt->set.tab[i].is_harddrop = gred_use_harddrop(q);
+ opt->set.tab[i].probability = q->parms.max_P;
+ opt->set.tab[i].backlog = &q->backlog;
}
- opt.set.qstats = &sch->qstats;
+ opt->set.qstats = &sch->qstats;
}
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_GRED, &opt);
+ dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_GRED, opt);
}
static int gred_offload_dump_stats(struct Qdisc *sch)
@@ -731,6 +733,7 @@ static int gred_change(struct Qdisc *sch, struct nlattr *opt,
static int gred_init(struct Qdisc *sch, struct nlattr *opt,
struct netlink_ext_ack *extack)
{
+ struct gred_sched *table = qdisc_priv(sch);
struct nlattr *tb[TCA_GRED_MAX + 1];
int err;
@@ -754,6 +757,12 @@ static int gred_init(struct Qdisc *sch, struct nlattr *opt,
sch->limit = qdisc_dev(sch)->tx_queue_len
* psched_mtu(qdisc_dev(sch));
+ if (qdisc_dev(sch)->netdev_ops->ndo_setup_tc) {
+ table->opt = kzalloc(sizeof(*table->opt), GFP_KERNEL);
+ if (!table->opt)
+ return -ENOMEM;
+ }
+
return gred_change_table_def(sch, tb[TCA_GRED_DPS], extack);
}
@@ -910,6 +919,7 @@ static void gred_destroy(struct Qdisc *sch)
gred_destroy_vq(table->tab[i]);
}
gred_offload(sch, TC_GRED_DESTROY);
+ kfree(table->opt);
}
static struct Qdisc_ops gred_qdisc_ops __read_mostly = {
--
2.33.1
On 2021-10-26 13:42:24 [+0300], Denis Kirjanov wrote:
> > diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
> > index 72de08ef8335e..1073c76d05c45 100644
> > --- a/net/sched/sch_gred.c
> > +++ b/net/sched/sch_gred.c
> > @@ -311,42 +312,43 @@ static void gred_offload(struct Qdisc *sch, enum tc_gred_command command)
> > {
> > struct gred_sched *table = qdisc_priv(sch);
> > struct net_device *dev = qdisc_dev(sch);
> > - struct tc_gred_qopt_offload opt = {
> > - .command = command,
> > - .handle = sch->handle,
> > - .parent = sch->parent,
> > - };
> > + struct tc_gred_qopt_offload *opt = table->opt;
> > if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
> > return;
> > + memset(opt, 0, sizeof(*opt));
>
> It's zeroed in kzalloc()
but it is not limited to a single invocation?
Sebastian
10/26/21 1:07 PM, Sebastian Andrzej Siewior пишет:
> From: Arnd Bergmann <[email protected]>
>
> The tc_gred_qopt_offload structure has grown too big to be on the
> stack for 32-bit architectures after recent changes.
>
> net/sched/sch_gred.c:903:13: error: stack frame size (1180) exceeds limit (1024) in 'gred_destroy' [-Werror,-Wframe-larger-than]
> net/sched/sch_gred.c:310:13: error: stack frame size (1212) exceeds limit (1024) in 'gred_offload' [-Werror,-Wframe-larger-than]
>
> Use dynamic allocation per qdisc to avoid this.
>
> Fixes: 50dc9a8572aa ("net: sched: Merge Qdisc::bstats and Qdisc::cpu_bstats data types")
> Fixes: 67c9e6270f30 ("net: sched: Protect Qdisc::bstats with u64_stats")
> Suggested-by: Jakub Kicinski <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> ---
> v2…v3:
> - drop not needed return statement in gred_offload() (Jakub)
> - use kzalloc(sizeof(*table->opt) in gred_init() (Eric)
> - Make the allocation conditional on ->ndo_setup_tc (Jakub).
>
> net/sched/sch_gred.c | 50 ++++++++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
> index 72de08ef8335e..1073c76d05c45 100644
> --- a/net/sched/sch_gred.c
> +++ b/net/sched/sch_gred.c
> @@ -56,6 +56,7 @@ struct gred_sched {
> u32 DPs;
> u32 def;
> struct red_vars wred_set;
> + struct tc_gred_qopt_offload *opt;
> };
>
> static inline int gred_wred_mode(struct gred_sched *table)
> @@ -311,42 +312,43 @@ static void gred_offload(struct Qdisc *sch, enum tc_gred_command command)
> {
> struct gred_sched *table = qdisc_priv(sch);
> struct net_device *dev = qdisc_dev(sch);
> - struct tc_gred_qopt_offload opt = {
> - .command = command,
> - .handle = sch->handle,
> - .parent = sch->parent,
> - };
> + struct tc_gred_qopt_offload *opt = table->opt;
>
> if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
> return;
>
> + memset(opt, 0, sizeof(*opt));
It's zeroed in kzalloc()
> + opt->command = command;
> + opt->handle = sch->handle;
> + opt->parent = sch->parent;
> +
> if (command == TC_GRED_REPLACE) {
> unsigned int i;
>
> - opt.set.grio_on = gred_rio_mode(table);
> - opt.set.wred_on = gred_wred_mode(table);
> - opt.set.dp_cnt = table->DPs;
> - opt.set.dp_def = table->def;
> + opt->set.grio_on = gred_rio_mode(table);
> + opt->set.wred_on = gred_wred_mode(table);
> + opt->set.dp_cnt = table->DPs;
> + opt->set.dp_def = table->def;
>
> for (i = 0; i < table->DPs; i++) {
> struct gred_sched_data *q = table->tab[i];
>
> if (!q)
> continue;
> - opt.set.tab[i].present = true;
> - opt.set.tab[i].limit = q->limit;
> - opt.set.tab[i].prio = q->prio;
> - opt.set.tab[i].min = q->parms.qth_min >> q->parms.Wlog;
> - opt.set.tab[i].max = q->parms.qth_max >> q->parms.Wlog;
> - opt.set.tab[i].is_ecn = gred_use_ecn(q);
> - opt.set.tab[i].is_harddrop = gred_use_harddrop(q);
> - opt.set.tab[i].probability = q->parms.max_P;
> - opt.set.tab[i].backlog = &q->backlog;
> + opt->set.tab[i].present = true;
> + opt->set.tab[i].limit = q->limit;
> + opt->set.tab[i].prio = q->prio;
> + opt->set.tab[i].min = q->parms.qth_min >> q->parms.Wlog;
> + opt->set.tab[i].max = q->parms.qth_max >> q->parms.Wlog;
> + opt->set.tab[i].is_ecn = gred_use_ecn(q);
> + opt->set.tab[i].is_harddrop = gred_use_harddrop(q);
> + opt->set.tab[i].probability = q->parms.max_P;
> + opt->set.tab[i].backlog = &q->backlog;
> }
> - opt.set.qstats = &sch->qstats;
> + opt->set.qstats = &sch->qstats;
> }
>
> - dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_GRED, &opt);
> + dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_GRED, opt);
> }
>
> static int gred_offload_dump_stats(struct Qdisc *sch)
> @@ -731,6 +733,7 @@ static int gred_change(struct Qdisc *sch, struct nlattr *opt,
> static int gred_init(struct Qdisc *sch, struct nlattr *opt,
> struct netlink_ext_ack *extack)
> {
> + struct gred_sched *table = qdisc_priv(sch);
> struct nlattr *tb[TCA_GRED_MAX + 1];
> int err;
>
> @@ -754,6 +757,12 @@ static int gred_init(struct Qdisc *sch, struct nlattr *opt,
> sch->limit = qdisc_dev(sch)->tx_queue_len
> * psched_mtu(qdisc_dev(sch));
>
> + if (qdisc_dev(sch)->netdev_ops->ndo_setup_tc) {
> + table->opt = kzalloc(sizeof(*table->opt), GFP_KERNEL);
> + if (!table->opt)
> + return -ENOMEM;
> + }
> +
> return gred_change_table_def(sch, tb[TCA_GRED_DPS], extack);
> }
>
> @@ -910,6 +919,7 @@ static void gred_destroy(struct Qdisc *sch)
> gred_destroy_vq(table->tab[i]);
> }
> gred_offload(sch, TC_GRED_DESTROY);
> + kfree(table->opt);
> }
>
> static struct Qdisc_ops gred_qdisc_ops __read_mostly = {
>
10/26/21 1:51 PM, Sebastian Andrzej Siewior пишет:
> On 2021-10-26 13:42:24 [+0300], Denis Kirjanov wrote:
>>> diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
>>> index 72de08ef8335e..1073c76d05c45 100644
>>> --- a/net/sched/sch_gred.c
>>> +++ b/net/sched/sch_gred.c
>>> @@ -311,42 +312,43 @@ static void gred_offload(struct Qdisc *sch, enum tc_gred_command command)
>>> {
>>> struct gred_sched *table = qdisc_priv(sch);
>>> struct net_device *dev = qdisc_dev(sch);
>>> - struct tc_gred_qopt_offload opt = {
>>> - .command = command,
>>> - .handle = sch->handle,
>>> - .parent = sch->parent,
>>> - };
>>> + struct tc_gred_qopt_offload *opt = table->opt;
>>> if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
>>> return;
>>> + memset(opt, 0, sizeof(*opt));
>>
>> It's zeroed in kzalloc()
>
> but it is not limited to a single invocation?
I meant that all fields are set in the function as it was with the stack
storage.
>
> Sebastian
>
On 2021-10-26 14:16:16 [+0300], Denis Kirjanov wrote:
> 10/26/21 1:51 PM, Sebastian Andrzej Siewior пишет:
> > On 2021-10-26 13:42:24 [+0300], Denis Kirjanov wrote:
> > > > diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
> > > > index 72de08ef8335e..1073c76d05c45 100644
> > > > --- a/net/sched/sch_gred.c
> > > > +++ b/net/sched/sch_gred.c
> > > > @@ -311,42 +312,43 @@ static void gred_offload(struct Qdisc *sch, enum tc_gred_command command)
> > > > {
> > > > struct gred_sched *table = qdisc_priv(sch);
> > > > struct net_device *dev = qdisc_dev(sch);
> > > > - struct tc_gred_qopt_offload opt = {
> > > > - .command = command,
> > > > - .handle = sch->handle,
> > > > - .parent = sch->parent,
> > > > - };
> > > > + struct tc_gred_qopt_offload *opt = table->opt;
> > > > if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
> > > > return;
> > > > + memset(opt, 0, sizeof(*opt));
> > >
> > > It's zeroed in kzalloc()
> >
> > but it is not limited to a single invocation?
>
> I meant that all fields are set in the function as it was with the stack
> storage.
What about?
| for (i = 0; i < table->DPs; i++) {
| struct gred_sched_data *q = table->tab[i];
|
| if (!q)
| continue;
The stack storage version has an implicit memset().
Sebastian
Hello:
This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <[email protected]>:
On Tue, 26 Oct 2021 12:07:11 +0200 you wrote:
> From: Arnd Bergmann <[email protected]>
>
> The tc_gred_qopt_offload structure has grown too big to be on the
> stack for 32-bit architectures after recent changes.
>
> net/sched/sch_gred.c:903:13: error: stack frame size (1180) exceeds limit (1024) in 'gred_destroy' [-Werror,-Wframe-larger-than]
> net/sched/sch_gred.c:310:13: error: stack frame size (1212) exceeds limit (1024) in 'gred_offload' [-Werror,-Wframe-larger-than]
>
> [...]
Here is the summary with links:
- [net-next,v3] net: sched: gred: dynamically allocate tc_gred_qopt_offload
https://git.kernel.org/netdev/net-next/c/f25c0515c521
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html