[PATCH] [net] net/mlx5e: fix another -Wmaybe-uninitialized warning

[PATCH] [net] net/mlx5e: fix another -Wmaybe-uninitialized warning

[Arnd Bergmann]

As found by Olof's build bot, today's mainline kernel gained a harmless
warning about a potential uninitalied variable reference:

drivers/net/ethernet/mellanox/mlx5/core/en_tc.c: In function 'parse_tc_fdb_actions':
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:769:13: warning: 'out_dev' may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:811:21: note: 'out_dev' was declared here

This was introduced through the addition of an 'IS_ERR/PTR_ERR' pair that
gcc is unfortunately unable to completely figure out. Replacing it with
PTR_ERR_OR_ZERO makes the code more understandable to gcc so it no longer
warns.

Hadar Hen Zion already attempted to fix the warning earlier by adding
fake initializations, but that ended up just making the code worse without
fully addressing all warnings, so I'm reverting it now that it is no longer
needed.

In order to avoid pulling a variable declaration into the #ifdef, I'm
removing it in favor of a more readable 'if()' statement here that
has the same effect.

Link: http://arm-soc.lixom.net/buildlogs/mainline/v4.10-rc3-98-gcff3b2c/
Fixes: a42485eb0ee4 ("net/mlx5e: TC ipv4 tunnel encap offload error flow fixes")
Fixes: a757d108dc1a ("net/mlx5e: Fix kbuild warnings for uninitialized parameters")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 118cea5..07d83835 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -666,14 +666,15 @@ static int mlx5e_route_lookup_ipv4(struct mlx5e_priv *priv,
 	struct rtable *rt;
 	struct neighbour *n = NULL;
 	int ttl;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_INET))
+		return -EOPNOTSUPP;
 
-#if IS_ENABLED(CONFIG_INET)
 	rt = ip_route_output_key(dev_net(mirred_dev), fl4);
-	if (IS_ERR(rt))
-		return PTR_ERR(rt);
-#else
-	return -EOPNOTSUPP;
-#endif
+	ret = PTR_ERR_OR_ZERO(rt);
+	if (ret)
+		return ret;
 
 	if (!switchdev_port_same_parent_id(priv->netdev, rt->dst.dev)) {
 		pr_warn("%s: can't offload, devices not on same HW e-switch\n", __func__);
@@ -741,8 +742,8 @@ static int mlx5e_create_encap_header_ipv4(struct mlx5e_priv *priv,
 	struct flowi4 fl4 = {};
 	char *encap_header;
 	int encap_size;
-	__be32 saddr = 0;
-	int ttl = 0;
+	__be32 saddr;
+	int ttl;
 	int err;
 
 	encap_header = kzalloc(max_encap_size, GFP_KERNEL);
-- 
2.9.0

Re: [PATCH] [net] net/mlx5e: fix another -Wmaybe-uninitialized warning

[Or Gerlitz]

On 1/11/2017 11:14 PM, Arnd Bergmann wrote:
> @@ -666,14 +666,15 @@ static int mlx5e_route_lookup_ipv4(struct mlx5e_priv *priv,
>   	struct rtable *rt;
>   	struct neighbour *n = NULL;
>   	int ttl;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_INET))
> +		return -EOPNOTSUPP;
>   
> -#if IS_ENABLED(CONFIG_INET)
>   	rt = ip_route_output_key(dev_net(mirred_dev), fl4);
> -	if (IS_ERR(rt))
> -		return PTR_ERR(rt);
> -#else
> -	return -EOPNOTSUPP;
> -#endif
> +	ret = PTR_ERR_OR_ZERO(rt);
> +	if (ret)
> +		return ret;

but this means that if we got NULL from ip_route_output_key, we will 
return success (0) here which is wrong.

Re: [PATCH] [net] net/mlx5e: fix another -Wmaybe-uninitialized warning

[Arnd Bergmann]

On Thursday, January 12, 2017 10:30:24 AM CET Or Gerlitz wrote:
> On 1/11/2017 11:14 PM, Arnd Bergmann wrote:
> > @@ -666,14 +666,15 @@ static int mlx5e_route_lookup_ipv4(struct mlx5e_priv *priv,
> >       struct rtable *rt;
> >       struct neighbour *n = NULL;
> >       int ttl;
> > +     int ret;
> > +
> > +     if (!IS_ENABLED(CONFIG_INET))
> > +             return -EOPNOTSUPP;
> >   
> > -#if IS_ENABLED(CONFIG_INET)
> >       rt = ip_route_output_key(dev_net(mirred_dev), fl4);
> > -     if (IS_ERR(rt))
> > -             return PTR_ERR(rt);
> > -#else
> > -     return -EOPNOTSUPP;
> > -#endif
> > +     ret = PTR_ERR_OR_ZERO(rt);
> > +     if (ret)
> > +             return ret;
> 
> but this means that if we got NULL from ip_route_output_key, we will 
> return success (0) here which is wrong.

I don't think so: if 'rt' is NULL or a valid pointer, then 'ret' is zero
and we will not return here.

	Arnd

Re: [PATCH] [net] net/mlx5e: fix another -Wmaybe-uninitialized warning

[Or Gerlitz]

On 1/11/2017 11:14 PM, Arnd Bergmann wrote:
> As found by Olof's build bot, today's mainline kernel gained a harmless
> warning about a potential uninitalied variable reference:
>
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c: In function 'parse_tc_fdb_actions':
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:769:13: warning: 'out_dev' may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:811:21: note: 'out_dev' was declared here
>
> This was introduced through the addition of an 'IS_ERR/PTR_ERR' pair that
> gcc is unfortunately unable to completely figure out. Replacing it with
> PTR_ERR_OR_ZERO makes the code more understandable to gcc so it no longer
> warns.

can you elaborate on this a little further?

> Hadar Hen Zion already attempted to fix the warning earlier by adding
> fake initializations, but that ended up just making the code worse without
> fully addressing all warnings, so I'm reverting it now that it is no longer needed.

ok, so if your approach eliminates the warning on out_dev and also on 
the variables for which Hadar added the faked initializers, I guess we 
should be fine with this change (saw your reply on my other comment), 
just another question:

> In order to avoid pulling a variable declaration into the #ifdef, I'm
> removing it in favor of a more readable 'if()' statement here that has the same effect.

When I build here without CONFIG_INET in my system, the build goes fine 
with this approach. However, we're pretty sure that in the past we got 
0-day report from the kbuild test robot where he was unhappy that we 
make the ip_route_output_key call without being wrapped with that #if 
IS_ENABLED(CONFIG_INET) -- so, we don't want to go there again... thoughts?

Or.

Re: [PATCH] [net] net/mlx5e: fix another -Wmaybe-uninitialized warning

[Arnd Bergmann]

On Thursday, January 12, 2017 5:21:49 PM CET Or Gerlitz wrote:
> On 1/11/2017 11:14 PM, Arnd Bergmann wrote:
> > As found by Olof's build bot, today's mainline kernel gained a harmless
> > warning about a potential uninitalied variable reference:
> >
> > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c: In function 'parse_tc_fdb_actions':
> > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:769:13: warning: 'out_dev' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:811:21: note: 'out_dev' was declared here
> >
> > This was introduced through the addition of an 'IS_ERR/PTR_ERR' pair that
> > gcc is unfortunately unable to completely figure out. Replacing it with
> > PTR_ERR_OR_ZERO makes the code more understandable to gcc so it no longer
> > warns.
> 
> can you elaborate on this a little further?


The problem is

static int mlx5e_route_lookup_ipv4(struct net_device **out_dev, ...)
{
       ...
       if (IS_ERR(rt))
              return PTR_ERR(rt);
       *out_dev = ...;
       ...
}

static int mlx5e_create_encap_header_ipv4(...)
{
       ...
       err = mlx5e_route_lookup_ipv4(..., out_dev, ...);
       if (err)
               goto out;

       e->out_dev = *out_dev;
       ...
}

I've seen several examples of this, the problem every time is
that gcc cannot tell that if(IS_ERR()) in the first function is
equivalent to if(err) in the second, so it assumes that 'out_dev'
is used here after the first 'return PTR_ERR(rt)'.

The PTR_ERR_OR_ZERO() case by comparison is fairly easy to detect
by gcc, so it can't get that wrong here.

> > Hadar Hen Zion already attempted to fix the warning earlier by adding
> > fake initializations, but that ended up just making the code worse without
> > fully addressing all warnings, so I'm reverting it now that it is no longer needed.
> 
> ok, so if your approach eliminates the warning on out_dev and also on 
> the variables for which Hadar added the faked initializers, I guess we 
> should be fine with this change (saw your reply on my other comment), 

Ok.

> just another question:
> 
> > In order to avoid pulling a variable declaration into the #ifdef, I'm
> > removing it in favor of a more readable 'if()' statement here that has the same effect.
> 
> When I build here without CONFIG_INET in my system, the build goes fine 
> with this approach. However, we're pretty sure that in the past we got 
> 0-day report from the kbuild test robot where he was unhappy that we 
> make the ip_route_output_key call without being wrapped with that #if 
> IS_ENABLED(CONFIG_INET) -- so, we don't want to go there again... thoughts?

I went back and forth between the two versions, either leaving the #if
in place, or using the if(IS_ENABLED()) check to be really sure that
we can't get compile error here.

I did check that ip_route_output_key() is always declared, but now
I see that net/route.h might not always be included from en_tc.c
if CONFIG_INET is disabled (I don't see how it gets included, but
it obviously is when CONFIG_INET is turned on).

Adding an explicit include of that file should probably avoid the
case you ran into earlier, but for I agree it's safer to not rely
on that here for a bugfix, and just leave the #ifdef. Do you want to
modify it yourself, or should I spin a new version with that?

	Arnd

Re: [PATCH] [net] net/mlx5e: fix another -Wmaybe-uninitialized warning

[Or Gerlitz]

On Thu, Jan 12, 2017 at 6:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday, January 12, 2017 5:21:49 PM CET Or Gerlitz wrote:


>> When I build here without CONFIG_INET in my system, the build goes fine
>> with this approach. However, we're pretty sure that in the past we got
>> 0-day report from the kbuild test robot where he was unhappy that we
>> make the ip_route_output_key call without being wrapped with that #if
>> IS_ENABLED(CONFIG_INET) -- so, we don't want to go there again... thoughts?

> I went back and forth between the two versions, either leaving the #if
> in place, or using the if(IS_ENABLED()) check to be really sure that
> we can't get compile error here.

> I did check that ip_route_output_key() is always declared, but now
> I see that net/route.h might not always be included from en_tc.c
> if CONFIG_INET is disabled (I don't see how it gets included, but
> it obviously is when CONFIG_INET is turned on).

> Adding an explicit include of that file should probably avoid the
> case you ran into earlier, but for I agree it's safer to not rely
> on that here for a bugfix, and just leave the #ifdef. Do you want to
> modify it yourself, or should I spin a new version with that?

I can do that next week, thanks