On Wed 10 Feb 2021 at 15:21, Ido Schimmel <idosch@xxxxxxxxxx> 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@xxxxxxxxxx> 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".
Actually, some of them do. I mentioned netdevsim specifically because
nsim_fib_create() returns pointer and also casts return value of
register_fib_notifier() with 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)
>
Makes sense. You want me to send the fix or you will do it?
>>
>> Regards,
>> Vlad
>>
>> >
>> > 1640 }
>> > 1641
>> > 1642 return encap;
>> > 1643 }
>> >
>> > regards,
>> > dan carpenter
>>
