[net-next PATCH] ipv6: fix false-postive maybe-uninitialized warning

[net-next PATCH] ipv6: fix false-postive maybe-uninitialized warning

[Arnd Bergmann]

Adding a lock around one of the assignments prevents gcc from
tracking the state of the local 'fibmatch' variable, so it can no
longer prove that 'dst' is always initialized, leading to a bogus
warning:

net/ipv6/route.c: In function 'inet6_rtm_getroute':
net/ipv6/route.c:3659:2: error: 'dst' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This moves the other assignment into the same lock to shut up the
warning.

Fixes: 121622dba8da ("ipv6: route: make rtm_getroute not assume rtnl is locked")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 net/ipv6/route.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

This kind of warning involving an unlock between variable initialization
and use is relatively frequent for false-positives. I should try to
seek clarification from the gcc developers on whether this can be
improved.

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index dc021ed6dd37..bec12ae3e6b7 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3624,6 +3624,8 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 
 		if (!fibmatch)
 			dst = ip6_route_input_lookup(net, dev, &fl6, flags);
+		else
+			dst = ip6_route_lookup(net, &fl6, 0);
 
 		rcu_read_unlock();
 	} else {
@@ -3631,10 +3633,10 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 
 		if (!fibmatch)
 			dst = ip6_route_output(net, NULL, &fl6);
+		else
+			dst = ip6_route_lookup(net, &fl6, 0);
 	}
 
-	if (fibmatch)
-		dst = ip6_route_lookup(net, &fl6, 0);
 
 	rt = container_of(dst, struct rt6_info, dst);
 	if (rt->dst.error) {
-- 
2.9.0

Re: [net-next PATCH] ipv6: fix false-postive maybe-uninitialized warning

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 18 Aug 2017 13:34:22 +0200

> Adding a lock around one of the assignments prevents gcc from
> tracking the state of the local 'fibmatch' variable, so it can no
> longer prove that 'dst' is always initialized, leading to a bogus
> warning:
> 
> net/ipv6/route.c: In function 'inet6_rtm_getroute':
> net/ipv6/route.c:3659:2: error: 'dst' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> This moves the other assignment into the same lock to shut up the
> warning.
> 
> Fixes: 121622dba8da ("ipv6: route: make rtm_getroute not assume rtnl is locked")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  net/ipv6/route.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> This kind of warning involving an unlock between variable initialization
> and use is relatively frequent for false-positives. I should try to
> seek clarification from the gcc developers on whether this can be
> improved.

This will have to do for now I suppose.

I guess the issue is that if the local variable ever sits on the stack
then the memory barriers in the locks block the full dataflow
analysis.

But this makes no sense from a dataflow perspective.  Even if the
local variable has a stack slot, there is no "escapability" of that
memory addres to foreign modifications.

If I had a nickel for every uninitialized variable warning we had to
work around....

Re: [net-next PATCH] ipv6: fix false-postive maybe-uninitialized warning

[Arnd Bergmann]

On Fri, Aug 18, 2017 at 7:49 PM, David Miller <davem@davemloft.net> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Fri, 18 Aug 2017 13:34:22 +0200

>>
>> This kind of warning involving an unlock between variable initialization
>> and use is relatively frequent for false-positives. I should try to
>> seek clarification from the gcc developers on whether this can be
>> improved.
>
> This will have to do for now I suppose.
>
> I guess the issue is that if the local variable ever sits on the stack
> then the memory barriers in the locks block the full dataflow
> analysis.
>
> But this makes no sense from a dataflow perspective.  Even if the
> local variable has a stack slot, there is no "escapability" of that
> memory addres to foreign modifications.
>
> If I had a nickel for every uninitialized variable warning we had to
> work around....

Since this pattern has come up so often, I spent most of my working
day today on a reduced testcase, and ended up with this surprising
snippet:

int f(void);
static inline void rcu_read_unlock(void)
{
        static _Bool __warned;
        if (f() && !__warned && !f()) {
                __warned = 1;
        }
}
int inet6_rtm_getroute(void)
{
        int dst;
        int fibmatch = f();

        if (!fibmatch)
                dst = f();
        rcu_read_unlock();
        if (fibmatch)
                dst = 0;

        return dst;
}

So at least in this particular case, the culprit is not actually
a memory barrier, but RCU_LOCKDEP_WARN(). A related
problem is __branch_check__()/__trace_if().

While the maybe-uninitialized warnings are unreliable by
definition, I think that case really should be understood by gcc.

I looked through the gcc bug database which has countless
entries but doesn't seem to have this one yet, so I opened
a new bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81897

Unfortunately the basic behavior shows up in gcc-4.7 already,
so it has no chance of getting fixed on older compilers.

        Arnd