netdev - Re: [GIT] Networking

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] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 8 Dec 2019 13:35:08 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     David Miller <davem@...hat.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT] Networking

On Sun, Dec 8, 2019 at 1:20 AM David Miller <davem@...hat.com> wrote:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git

Grr,

This introduces a new warning for me:

    drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c: In function
‘mlx5e_tc_tun_create_header_ipv6’:
    drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c:332:20:
warning: ‘n’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
      332 |  struct neighbour *n;
          |                    ^

which is very annoying.

The cause is commit 6c8991f41546 ("net: ipv6_stub: use
ip6_dst_lookup_flow instead of ip6_dst_lookup") which changed
mlx5e_route_lookup_ipv6() to use ipv6_dst_lookup_flow() which returns
an error pointer, so it then does

        if (IS_ERR(dst))
                return PTR_ERR(dst);

instead of

        if (ret < 0)
                return ret;

in the old code.

And that then means that the caller, which does

        err = mlx5e_route_lookup_ipv6(priv, mirred_dev, &out_dev, &route_dev,
                                      &fl6, &n, &ttl);
        if (err)
                return err;

and now gcc no longer sees that 'n' is always initialized when 'err'
is zero. Because gcc apparently thinks that the

        if (IS_ERR(dst))
                return PTR_ERR(dst);

thing can result in a zero return value (I guess the cast from a
64-bit pointer to an 'int' is where it thinks "ok, that cast might
lose high bits and become zero even when the original was tested to
have a range where that will not happen).

Anyway - the code is not buggy, but the new warning is simply not
acceptable. We keep the build warning free even if it's gcc not being
clever enough to see that "if it's uninitialized, we never get to the
location where it's used".

I don't know what the networking pattern for this is, but I did this
in the merge

--      struct neighbour *n;
++      struct neighbour *n = NULL;

and I'm not happy about having gotten a pull request that has this
kind of shit in it, especially since it was _known_ shit.

It could have been that people had compilers that didn't see this
problem. But no.

I see that Stephen Rothwell reported it four days ago, and David seems
to have even answered it.

And yet that warning was still there in the pull request.

WTF?

David, this is not acceptable.  You don't introduce new warnings in the tree.

It doesn't matter one whit whether the warning is a false positive. A
false positive warning will then be the cause of ignoring subsequent
real warnings, which makes false positive warnings completely
unacceptable.

Fix your mindset, and stop sending me garbage.

                   Linus