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

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Feb 10, 2021 at 03:21:37PM +0200, Ido Schimmel wrote:
> 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()

Ideally Smatch would be able to figure out this rule automatically.

I see now that fib6_tables_dump() can't return positive because it
returns either zero or the return from the "w->func = fib6_node_dump;"
and fib6_node_dump() doesn't return positive.  

Handling it properly is something I've thought about but I haven't
figured out how.  What I will do is hard code this into Smatch by adding
a line into the smatch_data/db/fixup_kernel.sh file:

delete from return_states where function = 'fib6_tables_dump' and return = '1';

regards,
dan carpenter