* [PATCH] net: ixgbe: Fix memleak in ixgbe_configure_clsu32
@ 2021-01-03 8:08 Dinghao Liu
2021-01-03 8:41 ` [Intel-wired-lan] " Paul Menzel
0 siblings, 1 reply; 3+ messages in thread
From: Dinghao Liu @ 2021-01-03 8:08 UTC (permalink / raw)
To: dinghao.liu, kjlu
Cc: Jesse Brandeburg, Tony Nguyen, David S. Miller, Jakub Kicinski,
intel-wired-lan, netdev, linux-kernel
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(-)
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]))
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Intel-wired-lan] [PATCH] net: ixgbe: Fix memleak in ixgbe_configure_clsu32
2021-01-03 8:08 [PATCH] net: ixgbe: Fix memleak in ixgbe_configure_clsu32 Dinghao Liu
@ 2021-01-03 8:41 ` Paul Menzel
2021-01-03 10:03 ` dinghao.liu
0 siblings, 1 reply; 3+ messages in thread
From: Paul Menzel @ 2021-01-03 8:41 UTC (permalink / raw)
To: Dinghao Liu
Cc: netdev, linux-kernel, intel-wired-lan, Jakub Kicinski,
David S. Miller, kjlu
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Re: [Intel-wired-lan] [PATCH] net: ixgbe: Fix memleak in ixgbe_configure_clsu32
2021-01-03 8:41 ` [Intel-wired-lan] " Paul Menzel
@ 2021-01-03 10:03 ` dinghao.liu
0 siblings, 0 replies; 3+ messages in thread
From: dinghao.liu @ 2021-01-03 10:03 UTC (permalink / raw)
To: Paul Menzel
Cc: netdev, linux-kernel, intel-wired-lan, Jakub Kicinski,
David S. Miller, kjlu
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-03 10:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-03 8:08 [PATCH] net: ixgbe: Fix memleak in ixgbe_configure_clsu32 Dinghao Liu
2021-01-03 8:41 ` [Intel-wired-lan] " Paul Menzel
2021-01-03 10:03 ` dinghao.liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).