linux-kernel - Re: sch_generic: warning: the comparison will always evaluate as ‘true’ for the address of ‘noop

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: <1313343598.2798.12.camel@edumazet-laptop>
Date:	Sun, 14 Aug 2011 19:39:58 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Kevin Winchester <kjwinchester@...il.com>
Cc:	hadi@...erus.ca, davem@...emloft.net, netdev@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: sch_generic: warning: the comparison will always evaluate as
 ‘true’ for the address of
 ‘noop_qdisc’ will never be NULL

Le dimanche 14 août 2011 à 10:44 -0300, Kevin Winchester a écrit :
> With:
> 
> gcc (GCC) 4.6.1
> 
> I noticed the following warning appearing in my build:
> 
> net/sched/sch_generic.c: In function ‘dev_graft_qdisc’:
> net/sched/sch_generic.c:678:2: warning: the comparison will always
> evaluate as ‘true’ for the address of ‘noop_qdisc’ will never be NULL
> [-Waddress]
> 
> The code in question runs:
> 
> 
>         /* ... and graft new one */
>         if (qdisc == NULL)
>                 qdisc = &noop_qdisc;
>         dev_queue->qdisc_sleeping = qdisc;
>         rcu_assign_pointer(dev_queue->qdisc, &noop_qdisc);
> 
> where rcu_assign_pointer has a null check that does not apply to
> noop_qdisc, which will never be null.
> 

gcc is a bit stupid here. Of course we know &noop_qdisc cannot be NULL.

> My question is, should that really be assigning &noop_qdisc in there?
> It seems odd to assign &noop_qdisc to qdisc only if qdisc is null, and
> then unconditionally assign &noop_qdisc into dev_queue->qdisc.
> 
> Should the line be:
> 
>         rcu_assign_pointer(dev_queue->qdisc, qdisc);
> 
> instead?
> 
> Just curious,
> 

This was already taken into account, the trick is to make rcu_assign()
not trying to be smart, and use RCU_INIT_POINTER() in places we want to
assign NULL pointers.

So one patch is carried by Paul McKenney (RCU maintainer) for next
kernel, and other one in net-next :

http://git2.kernel.org/?p=linux/kernel/git/paulmck/linux-2.6-rcu.git;a=commitdiff;h=a7590ddfc2c855e75111ef18147a86578fe136e4

http://git2.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commitdiff;h=a9b3cd7f323b2e57593e7215362a7b02fc933e3a



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/