linux-kernel - Re: [PATCH] [v2] net: sched: gred: dynamically allocate tc_gred_qopt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Oct 2021 15:08:54 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Arnd Bergmann <arnd@...db.de>,
        Zheng Yongjun <zhengyongjun3@...wei.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Randy Dunlap <rdunlap@...radead.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [v2] net: sched: gred: dynamically allocate
 tc_gred_qopt_offload

On Tue, 19 Oct 2021 21:15:29 +0200 Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.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@...nel.org>
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
> Hi Jakub,
> 
> Not sure if this is what you had in mind, if not it might be easier
> if you do it yourself. In particular, adding tc_gred_qopt_offload
> to gred_sched directly rather than as a pointer may be easier here,
> but that may have other downsides.

This is exactly what I had in mind, thanks!

Two minor nits if you're willing to respin, if you feel like you've
spent enough time on this already we can marge as is :)

> -		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);
> +
> +	return;

return no longer needed

>  }
>  
>  static int gred_offload_dump_stats(struct Qdisc *sch)

> @@ -754,6 +759,10 @@ static int gred_init(struct Qdisc *sch, struct nlattr *opt,
>  		sch->limit = qdisc_dev(sch)->tx_queue_len
>  		             * psched_mtu(qdisc_dev(sch));

The ops should not change, so I think we can do

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