| Message ID | 20210103080843.25914-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ixgbe: Fix memleak in ixgbe_configure_clsu32 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 12 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Dear Dinghao, Am 03.01.21 um 09:08 schrieb Dinghao Liu: > When ixgbe_fdir_write_perfect_filter_82599() fails, > input allocated by kzalloc() has not been freed, > which leads to memleak. Nice find. Thank you for your patches. Out of curiosity, do you use an analysis tool to find these issues? > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index 393d1c2cd853..e9c2d28efc81 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -9582,8 +9582,10 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter, > ixgbe_atr_compute_perfect_hash_82599(&input->filter, mask); > err = ixgbe_fdir_write_perfect_filter_82599(hw, &input->filter, > input->sw_idx, queue); > - if (!err) > - ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); > + if (err) > + goto err_out_w_lock; > + > + ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); > spin_unlock(&adapter->fdir_perfect_lock); > > if ((uhtid != 0x800) && (adapter->jump_tables[uhtid])) Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> I wonder, in the non-error case, how `input` and `jump` are freed. Old code: > if (!err) > ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); > spin_unlock(&adapter->fdir_perfect_lock); > > if ((uhtid != 0x800) && (adapter->jump_tables[uhtid])) > set_bit(loc - 1, (adapter->jump_tables[uhtid])->child_loc_map); > > kfree(mask); > return err; Should these two lines be replaced with `goto err_out`? (Though the name is confusing then, as it’s the non-error case.) > err_out_w_lock: > spin_unlock(&adapter->fdir_perfect_lock); > err_out: > kfree(mask); > free_input: > kfree(input); > free_jump: > kfree(jump); > return err; > } Kind regards, Paul
> Dear Dinghao, > > > Am 03.01.21 um 09:08 schrieb Dinghao Liu: > > When ixgbe_fdir_write_perfect_filter_82599() fails, > > input allocated by kzalloc() has not been freed, > > which leads to memleak. > > Nice find. Thank you for your patches. Out of curiosity, do you use an > analysis tool to find these issues? > Yes, these bugs are suggested by my static analysis tool. > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > index 393d1c2cd853..e9c2d28efc81 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > @@ -9582,8 +9582,10 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter, > > ixgbe_atr_compute_perfect_hash_82599(&input->filter, mask); > > err = ixgbe_fdir_write_perfect_filter_82599(hw, &input->filter, > > input->sw_idx, queue); > > - if (!err) > > - ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); > > + if (err) > > + goto err_out_w_lock; > > + > > + ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); > > spin_unlock(&adapter->fdir_perfect_lock); > > > > if ((uhtid != 0x800) && (adapter->jump_tables[uhtid])) > > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> > > I wonder, in the non-error case, how `input` and `jump` are freed. > I'm not sure if kfree(jump) will introduce bug. jump is allocated in a for loop and has been passed to adapter->jump_tables. input and mask have new definitions (kzalloc) after this loop, it's reasonable to free them on failure. But jump is different. Maybe we should not free jump after the loop? > Old code: > > > if (!err) > > ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); > > spin_unlock(&adapter->fdir_perfect_lock); > > > > if ((uhtid != 0x800) && (adapter->jump_tables[uhtid])) > > set_bit(loc - 1, (adapter->jump_tables[uhtid])->child_loc_map); > > > > kfree(mask); > > return err; > > Should these two lines be replaced with `goto err_out`? (Though the name > is confusing then, as it’s the non-error case.) > This also makes me confused. It seems that the check against untied is not error handling code, but there is a kfree(mask) after it. Freeing allocated data on success is not reasonable. Regards, Dinghao > > err_out_w_lock: > > spin_unlock(&adapter->fdir_perfect_lock); > > err_out: > > kfree(mask); > > free_input: > > kfree(input); > > free_jump: > > kfree(jump); > > return err; > > } > > Kind regards, > > Paul
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 393d1c2cd853..e9c2d28efc81 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -9582,8 +9582,10 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter, ixgbe_atr_compute_perfect_hash_82599(&input->filter, mask); err = ixgbe_fdir_write_perfect_filter_82599(hw, &input->filter, input->sw_idx, queue); - if (!err) - ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); + if (err) + goto err_out_w_lock; + + ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); spin_unlock(&adapter->fdir_perfect_lock); if ((uhtid != 0x800) && (adapter->jump_tables[uhtid]))
When ixgbe_fdir_write_perfect_filter_82599() fails, input allocated by kzalloc() has not been freed, which leads to memleak. Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)