| Message ID | 20170818113434.3037484-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 401481e06099533892b3d8f1db498cbc480b5b24 |
| Headers | show |
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....
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
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) {
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. -- 2.9.0