Re: [PATCH] net: mellanox: prevent resource leak on htbl

[Cengiz Can]

On 2020-01-13 17:49, Alex Vesker wrote:
> On 1/13/2020 3:46 PM, Cengiz Can wrote:
> > According to a Coverity static analysis tool,
> > `drivers/net/mellanox/mlx5/core/steering/dr_rule.c#63` leaks a
> > `struct mlx5dr_ste_htbl *` named `new_htbl` while returning from
> > `dr_rule_create_collision_htbl` function.
> >
> > A annotated snippet of the possible resource leak follows:
> >
> > ```
> > static struct mlx5dr_ste *
> > dr_rule_create_collision_htbl(struct mlx5dr_matcher *matcher,
> >                               struct mlx5dr_matcher_rx_tx 
> > *nic_matcher,
> >                               u8 *hw_ste)
> >    /* ... */
> >    /* ... */
> >
> >    /* Storage is returned from allocation function 
> > mlx5dr_ste_htbl_alloc. */
> >    /* Assigning: new_htbl = storage returned from 
> > mlx5dr_ste_htbl_alloc(..) */
> >         new_htbl = mlx5dr_ste_htbl_alloc(dmn->ste_icm_pool,
> >                                          DR_CHUNK_SIZE_1,
> >                                          MLX5DR_STE_LU_TYPE_DONT_CARE,
> >                                          0);
> >    /* Condition !new_htbl, taking false branch. */
> >         if (!new_htbl) {
> >                 mlx5dr_dbg(dmn, "Failed allocating collision 
> > table\n");
> >                 return NULL;
> >         }
> >
> >         /* One and only entry, never grows */
> >         ste = new_htbl->ste_arr;
> >         mlx5dr_ste_set_miss_addr(hw_ste, 
> > nic_matcher->e_anchor->chunk->icm_addr);
> >    /* Resource new_htbl is not freed or pointed-to in mlx5dr_htbl_get 
> > */
> >         mlx5dr_htbl_get(new_htbl);
> >
> >    /* Variable new_htbl going out of scope leaks the storage it points 
> > to. */
> >         return ste;
> > ```
> >
> > There's a caller of this function which does refcounting and free'ing 
> > by
> > itself but that function also skips free'ing `new_htbl` due to missing
> > jump to error label. (referring to `dr_rule_create_collision_entry 
> > lines
> > 75-77. They don't jump to `free_tbl`)
> >
> > Added a `kfree(new_htbl)` just before returning `ste` pointer to fix 
> > the
> > leak.
> >
> > Signed-off-by: Cengiz Can <cengiz@xxxxxxxxxx>
> > ---
> >
> > This might be totally breaking the refcounting logic in the file so
> > please provide any feedback so I can evolve this into something more
> > suitable.
> >
> > For the record, Coverity scan id is CID 1457773.
> >
> >  drivers/net/ethernet/mellanox/mlx5/core/steering/dr_rule.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git 
> > a/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_rule.c 
> > b/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_rule.c
> > index e4cff7abb348..047b403c61db 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_rule.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_rule.c
> > @@ -60,6 +60,8 @@ dr_rule_create_collision_htbl(struct mlx5dr_matcher 
> > *matcher,
> >  	mlx5dr_ste_set_miss_addr(hw_ste, 
> > nic_matcher->e_anchor->chunk->icm_addr);
> >  	mlx5dr_htbl_get(new_htbl);
> >
> > +	kfree(new_htbl);
> > +
> >  	return ste;
> >  }
> >
> > --
> > 2.24.1
> The fix looks incorrect to me.
> The table is pointed by each ste in the ste_arr, ste->new_htbl and 
> being
> freed on mlx5dr_htbl_put.
> We tested kmemleak a few days ago and came clean.
> usually coverity is not wrong, but in this case I don't see the bug...

Hello Alex,

To my experience, the refcounting logic is complex and spread out to 
multiple files.

It might be a false positive on Coverity's side.

If it certainly sounds wrong, we can ignore this.

Thanks

--
Cengiz Can
@cengiz_io