Messages in this thread | | | From | Arnd Bergmann <> | Date | Thu, 21 Mar 2019 16:25:16 +0100 | Subject | Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c |
| |
On Thu, Mar 21, 2019 at 3:57 PM Jon Maloy <jon.maloy@ericsson.com> wrote: > > -----Original Message----- > > From: Arnd Bergmann <arnd@arndb.de> > > Sent: 21-Mar-19 12:45 > > To: Nick Desaulniers <ndesaulniers@google.com> > > Cc: Nathan Chancellor <natechancellor@gmail.com>; Jon Maloy > > <jon.maloy@ericsson.com>; Ying Xue <ying.xue@windriver.com>; David S. > > Miller <davem@davemloft.net>; tipc-discussion@lists.sourceforge.net; > > Networking <netdev@vger.kernel.org>; LKML <linux- > > kernel@vger.kernel.org>; clang-built-linux@googlegroups.com > > Subject: Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c > > > > On Wed, Mar 20, 2019 at 9:51 PM 'Nick Desaulniers' via Clang Built Linux > > <clang-built-linux@googlegroups.com> wrote: > > > On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor > > > <natechancellor@gmail.com> wrote: > > > > > > The use in tipc_bearer_xmit() isn't even guaranteed to set the in/out > > > parameter, so even if the if is taken doesn't guarantee that maddr is > > > always initialized before calling tipc_bearer_xmit(). > > > > Right, it is only initialized in certain states. It was always initialized until > > commit 598411d70f85 ("tipc: make resetting of links non-atomic"), > > afterwards only if the link was not reset, and as of commit 73f646cec354 > > ("tipc: delay ESTABLISH state event when link is established") only if it's not > > 'establishing' or 'reset'. > > > > > At the minimum, we should initialize maddr to NULL. I think we'd > > > prefer to risk the possibility of a null pointer dereference to the > > > possibility of working with uninitialized memory. To be clear, both > > > are bad, but one is easier to spot/debug later than the other. > > > > I disagree with setting it to NULL, given that it is still an obviously incorrect > > value. We could add a if(maddr) check before calling tipc_bearer_xmit(), but > > I think it would be clearer to check > > skb_queue_empty(xmitq)) if that avoids the warning: > > I may be wrong, but I would be surprised if that would eliminate the warning. > To me, setting maddr to NULL and then testing for it looks both more comprehensible and is guaranteed to fix the warning. > > > > > diff --git a/net/tipc/node.c b/net/tipc/node.c index > > 2dc4919ab23c..147786795e48 100644 > > --- a/net/tipc/node.c > > +++ b/net/tipc/node.c > > @@ -844,7 +844,8 @@ static void tipc_node_link_down(struct tipc_node *n, > > int bearer_id, bool delete) > > tipc_node_write_unlock(n); > > if (delete) > > tipc_mon_remove_peer(n->net, n->addr, old_bearer_id); > > - tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr); > > + if (skb_queue_empty(xmitq)) > > + tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr); > > tipc_sk_rcv(n->net, &le->inputq); } > > > > This duplicates the check inside of skb_queue_empty(), but I don't know if > > the compiler can see through the logic behind the inlined function calls. > > Probably not, but this is not in any way time critical.
I meant it's unclear whether compilers should be expected to see that skb_queue_empty() is true after the call to __skb_queue_head_init() initializes it.
I think recent versions of gcc do see that, not sure about clang, as it does inlining differently. tipc_bearer_xmit() cannot be inlined here (unless we start using LTO).
Arnd
| |