diff mbox series

[net-next,v3] net: sched: gred: dynamically allocate tc_gred_qopt_offload

Message ID 20211026100711.nalhttf6mbe6sudx@linutronix.de (mailing list archive)
State Accepted
Commit f25c0515c521375154c62c72447869f40218c861
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v3] net: sched: gred: dynamically allocate tc_gred_qopt_offload | expand

Checks

Context Check Description
netdev/cover_letter success Single patches do not need cover letters
netdev/fixes_present success Fixes tag not required for -next series
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Fixes tag looks correct
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 96 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success No static functions without inline keyword in header files

Commit Message

Sebastian Andrzej Siewior Oct. 26, 2021, 10:07 a.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

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 <kuba@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
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(-)

Comments

Denis Kirjanov Oct. 26, 2021, 10:42 a.m. UTC | #1 
10/26/21 1:07 PM, Sebastian Andrzej Siewior пишет:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <kuba@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> 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 = {
>
Sebastian Andrzej Siewior Oct. 26, 2021, 10:51 a.m. UTC | #2 
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
Denis Kirjanov Oct. 26, 2021, 11:16 a.m. UTC | #3 
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
>
Sebastian Andrzej Siewior Oct. 26, 2021, 11:43 a.m. UTC | #4 
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
patchwork-bot+netdevbpf@kernel.org Oct. 27, 2021, 7:20 p.m. UTC | #5 
Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 26 Oct 2021 12:07:11 +0200 you wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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!
diff mbox series

Patch

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 = {