Re: [bug report] net/mlx5e: Handle FIB events to update tunnel endpoint device

[Ido Schimmel <idosch@idosch.org>]

On Wed, Feb 10, 2021 at 01:55:23PM +0200, Vlad Buslov wrote:
> On Wed 10 Feb 2021 at 13:08, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > Hello Vlad Buslov,
> >
> > The patch 8914add2c9e5: "net/mlx5e: Handle FIB events to update
> > tunnel endpoint device" from Jan 25, 2021, leads to the following
> > static checker warning:
> >
> > 	drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c:1639 mlx5e_tc_tun_init()
> > 	error: passing non negative 1 to ERR_PTR
> >
> > drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
> >   1622  struct mlx5e_tc_tun_encap *mlx5e_tc_tun_init(struct mlx5e_priv *priv)
> >   1623  {
> >   1624          struct mlx5e_tc_tun_encap *encap;
> >   1625          int err;
> >   1626  
> >   1627          encap = kvzalloc(sizeof(*encap), GFP_KERNEL);
> >   1628          if (!encap)
> >   1629                  return ERR_PTR(-ENOMEM);
> >   1630  
> >   1631          encap->priv = priv;
> >   1632          encap->fib_nb.notifier_call = mlx5e_tc_tun_fib_event;
> >   1633          spin_lock_init(&encap->route_lock);
> >   1634          hash_init(encap->route_tbl);
> >   1635          err = register_fib_notifier(dev_net(priv->netdev), &encap->fib_nb,
> >   1636                                      NULL, NULL);
> >
> > register_fib_notifier() calls fib_net_dump() which eventually calls
> > fib6_walk_continue() which can return 1 if "walk is incomplete (i.e.
> > suspended)".
> >
> >   1637          if (err) {
> >   1638                  kvfree(encap);
> >   1639                  return ERR_PTR(err);
> >
> > If this returns 1 it will eventually lead to an Oops.
> 
> Hi Dan,
> 
> Thanks for the bug report!
> 
> This looks a bit strange to me because none of the other users of this
> API handle positive error code in any special way (including reference
> netdevsim implementation). Maybe API itself should be fixed? Jiri, Ido,
> what do you think?

The other functions that call register_fib_notifier() return an int, but
mlx5e_tc_tun_init() returns a pointer. I think that's why it was
flagged: "error: passing non negative 1 to ERR_PTR".

fib6_walk_continue() cannot return a positive value when called from
register_fib_notifier(), but ignoring it means that you will keep
getting the same static analysis error. What about:

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index f43e27555725..ef9d022e693f 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -499,7 +499,7 @@ int fib6_tables_dump(struct net *net, struct notifier_block *nb,
 
                hlist_for_each_entry_rcu(tb, head, tb6_hlist) {
                        err = fib6_table_dump(net, tb, w);
-                       if (err < 0)
+                       if (err)
                                goto out;
                }
        }
@@ -507,7 +507,8 @@ int fib6_tables_dump(struct net *net, struct notifier_block *nb,
 out:
        kfree(w);
 
-       return err;
+       /* The tree traversal function should never return a positive value. */
+       return err > 0 ? -EINVAL : err;
 }
 
 static int fib6_dump_node(struct fib6_walker *w)

> 
> Regards,
> Vlad
> 
> >
> >   1640          }
> >   1641  
> >   1642          return encap;
> >   1643  }
> >
> > regards,
> > dan carpenter
>