netdev - Re: [PATCH net-next 3/4] sch_htb: Stats for offloaded HTB

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] [day] [month] [year] [list]

Message-ID: <d202fb5e-69a8-c7e3-7fd3-cb973845fc63@nvidia.com>
Date:   Fri, 11 Dec 2020 17:25:34 +0200
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
CC:     <kbuild@...ts.01.org>, Maxim Mikityanskiy <maximmi@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        "Cong Wang" <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>, <lkp@...el.com>,
        <kbuild-all@...ts.01.org>, <netdev@...r.kernel.org>,
        "Saeed Mahameed" <saeedm@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: Re: [PATCH net-next 3/4] sch_htb: Stats for offloaded HTB

On 2020-12-11 10:41, Dan Carpenter wrote:
> On Thu, Dec 10, 2020 at 05:07:28PM +0200, Maxim Mikityanskiy wrote:
>> On 2020-12-10 10:28, Dan Carpenter wrote:
>>> Hi Maxim,
>>>
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Maxim-Mikityanskiy/HTB-offload/20201210-000703
>>> base:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
>>> afae3cc2da100ead3cd6ef4bb1fb8bc9d4b817c5
>>> config: i386-randconfig-m021-20201209 (attached as .config)
>>> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>>>
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <lkp@...el.com>
>>> Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
>>>
>>> smatch warnings:
>>> net/sched/sch_htb.c:1310 htb_dump_class_stats() error: we previously assumed 'cl->leaf.q' could be null (see line 1300)
>>>
>>> vim +1310 net/sched/sch_htb.c
>>>
>>> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1289  static int
>>> 87990467d387f92 Stephen Hemminger     2006-08-10  1290  htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
>>> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1291  {
>>> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1292  	struct htb_class *cl = (struct htb_class *)arg;
>>> 1e0ac0107df684e Maxim Mikityanskiy    2020-12-09  1293  	struct htb_sched *q = qdisc_priv(sch);
>>> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1294  	struct gnet_stats_queue qs = {
>>> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1295  		.drops = cl->drops,
>>> 3c75f6ee139d464 Eric Dumazet          2017-09-18  1296  		.overlimits = cl->overlimits,
>>> 338ed9b4de57c4b Eric Dumazet          2016-06-21  1297  	};
>>> 6401585366326fc John Fastabend        2014-09-28  1298  	__u32 qlen = 0;
>>> ^1da177e4c3f415 Linus Torvalds        2005-04-16  1299
>>> 5dd431b6b92c0db Paolo Abeni           2019-03-28 @1300  	if (!cl->level && cl->leaf.q)
>>>                                                                                     ^^^^^^^^^^
>>> Check for NULL
>>
>> Well, I don't think this is real... I don't see any possibility how
>> cl->leaf.q can be NULL for a leaf class. However, I'll add a similar check
>> below anyway.
>>
> 
> Another option is to remove this check if it's really impossible.

Yes, thanks, I see this option, but between these two options I'd pick 
the one that for sure doesn't make any change to the non-offloaded HTB 
logic. Even though to the best of my knowledge this check isn't needed, 
I might miss something, because I tried tracking down the origin of this 
code, and it was already there in the initial commit of 2005.

Respinning now with the CI issues fixed.

> regards,
> dan carpenter
>