Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190308001723.GA11197@archlinux-ryzen> <20190320190752.GA28744@archlinux-ryzen>
 <CAKwvOd=6916txvLmtEC3BcZgjLTNdtU-ou=YM+_dsd8CC-T_zw@mail.gmail.com>
 <CAK8P3a2pGo8HeHS5zcFnc_jLiLkXSPWKe4AYVm4TAmZxc_h2GA@mail.gmail.com> <BL0PR1501MB2003762D0D1CB08622C9244F9A420@BL0PR1501MB2003.namprd15.prod.outlook.com>
In-Reply-To: <BL0PR1501MB2003762D0D1CB08622C9244F9A420@BL0PR1501MB2003.namprd15.prod.outlook.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 21 Mar 2019 16:25:16 +0100
Message-ID: <CAK8P3a0Pq7qN-D9OsY7NWf=vQwqqSZFFXdEZ+xNckOLLCX40QA@mail.gmail.com>
Subject: Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c
To:     Jon Maloy <jon.maloy@ericsson.com>
Cc:     Nick Desaulniers <ndesaulniers@google.com>,
        Nathan Chancellor <natechancellor@gmail.com>,
        Ying Xue <ying.xue@windriver.com>,
        "David S. Miller" <davem@davemloft.net>,
        "tipc-discussion@lists.sourceforge.net" 
        <tipc-discussion@lists.sourceforge.net>,
        Networking <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "clang-built-linux@googlegroups.com" 
        <clang-built-linux@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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