Messages in this thread | | | Date | Mon, 13 Jan 2020 20:25:29 +0300 | From | Cengiz Can <> | Subject | Re: [PATCH] net: mellanox: prevent resource leak on htbl |
| |
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@kernel.wtf> >> --- >> >> 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
| |