In efx_probe_filters, the channel->rps_flow_id is freed in a
efx_for_each_channel marco  when success equals to 0.
However, after the following call chain:

efx_probe_filters
  |-> ef100_net_open
        |-> ef100_net_stop
              |-> efx_remove_filters

The channel->rps_flow_id is freed again in the efx_for_each_channel of
efx_remove_filters, triggering a double-free bug.

Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>
---
 drivers/net/ethernet/sfc/rx_common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
index d2f35ee15eff..fac227d372db 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -823,8 +823,10 @@ int efx_probe_filters(struct efx_nic *efx)
 		}
 
 		if (!success) {
-			efx_for_each_channel(channel, efx)
+			efx_for_each_channel(channel, efx) {
 				kfree(channel->rps_flow_id);
+				channel->rps_flow_id = NULL;
+			}
 			efx->type->filter_table_remove(efx);
 			rc = -ENOMEM;
 			goto out_unlock;
-- 
2.34.1

On 14/12/2023 15:22, Zhipeng Lu wrote:
> In efx_probe_filters, the channel->rps_flow_id is freed in a
> efx_for_each_channel marco  when success equals to 0.
> However, after the following call chain:
> 
> efx_probe_filters
>   |-> ef100_net_open
>         |-> ef100_net_stop
>               |-> efx_remove_filters
> 
> The channel->rps_flow_id is freed again in the efx_for_each_channel of
> efx_remove_filters, triggering a double-free bug.
> 
> Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>

Subject line should probably say [PATCH net] to specify the tree.
Modulo that, and Simon's correction to the commit message,

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

Hi,

On Thu, 2023-12-14 at 23:22 +0800, Zhipeng Lu wrote:
> In efx_probe_filters, the channel->rps_flow_id is freed in a
> efx_for_each_channel marco  when success equals to 0.
> However, after the following call chain:
> 
> efx_probe_filters
>   |-> ef100_net_open
>         |-> ef100_net_stop
>               |-> efx_remove_filters
> 
> The channel->rps_flow_id is freed again in the efx_for_each_channel of
> efx_remove_filters, triggering a double-free bug.
> 
> Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>

The patch LGTM, but could you please update the commit message as per
Simon's suggestions make it more consistent? You can retain Simon's RB
tag.

Thanks!

Paolo

On Thu, Dec 14, 2023 at 11:22:46PM +0800, Zhipeng Lu wrote:
> In efx_probe_filters, the channel->rps_flow_id is freed in a
> efx_for_each_channel marco  when success equals to 0.
> However, after the following call chain:
> 
> efx_probe_filters
>   |-> ef100_net_open
>         |-> ef100_net_stop
>               |-> efx_remove_filters

I think the call chain may be a bit more like:

ef100_net_open
  |-> efx_probe_filters
  |-> ef100_net_stop
        |-> efx_remove_filters

> 
> The channel->rps_flow_id is freed again in the efx_for_each_channel of
> efx_remove_filters, triggering a double-free bug.
> 
> Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>

The above nit not withstanding, I agree with your reasoning.
And that the problem was introduced in the cited commit.

Reviewed-by: Simon Horman <horms@kernel.org>

...

> On Thu, Dec 14, 2023 at 11:22:46PM +0800, Zhipeng Lu wrote:
> > In efx_probe_filters, the channel->rps_flow_id is freed in a
> > efx_for_each_channel marco  when success equals to 0.
> > However, after the following call chain:
> > 
> > efx_probe_filters
> >   |-> ef100_net_open
> >         |-> ef100_net_stop
> >               |-> efx_remove_filters
> 
> I think the call chain may be a bit more like:
> 
> ef100_net_open
>   |-> efx_probe_filters
>   |-> ef100_net_stop
>         |-> efx_remove_filters
> 
> > 
> > The channel->rps_flow_id is freed again in the efx_for_each_channel of
> > efx_remove_filters, triggering a double-free bug.
> > 
> > Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins")
> > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>
> 
> The above nit not withstanding, I agree with your reasoning.
> And that the problem was introduced in the cited commit.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>

Sorry for the call-chain's problem, I was not familiar with it at that time.
Thanks for Simon's correction. Appreciate!
I'll soon send a v2 patch with the corrected call-chain and RB tags.