| Message ID | 20231222154952.3531636-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] sfc: fix a double-free bug in efx_probe_filters | expand |
On Fri, Dec 22, 2023 at 11:49:52PM +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: > > 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. > --- Everything below the line above (---) will be omitted from the commit message when the patch is applied. > Changelog: > > v2: Correct the call-chain description in commit message and change > patch subject. > > Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins") > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > Reviewed-by: Simon Horman <horms@kernel.org> > Reviewed-by: Edward Cree <ecree.xilinx@gmail.com> Hi Zhipeng Lu, I think that your Signed-off-by should go last when you post a patch. And the Changelog should go below the (first set of) scissors (---). > --- > drivers/net/ethernet/sfc/rx_common.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) With the above in mind, I think you want something like: 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: 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") Reviewed-by: Simon Horman <horms@kernel.org> Reviewed-by: Edward Cree <ecree.xilinx@gmail.com> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> --- Changelog: v2: Correct the call-chain description in commit message and change patch subject. --- drivers/net/ethernet/sfc/rx_common.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
> On Fri, Dec 22, 2023 at 11:49:52PM +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: > > > > 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. > > --- > > Everything below the line above (---) will be omitted from the commit > message when the patch is applied. > > > Changelog: > > > > v2: Correct the call-chain description in commit message and change > > patch subject. > > > > Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins") > > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > > Reviewed-by: Simon Horman <horms@kernel.org> > > Reviewed-by: Edward Cree <ecree.xilinx@gmail.com> > > Hi Zhipeng Lu, > > I think that your Signed-off-by should go last when you post a patch. > > And the Changelog should go below the (first set of) scissors (---). > > > --- > > drivers/net/ethernet/sfc/rx_common.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > With the above in mind, I think you want something like: > > 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: > > 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") > Reviewed-by: Simon Horman <horms@kernel.org> > Reviewed-by: Edward Cree <ecree.xilinx@gmail.com> > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > --- > Changelog: > > v2: Correct the call-chain description in commit message and change > patch subject. > --- > drivers/net/ethernet/sfc/rx_common.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > -- > pw-bot: changes-requested Thank you for your detailed revision and correction! I'll send this patch again with your correction.
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;
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: 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. --- Changelog: v2: Correct the call-chain description in commit message and change patch subject. Fixes: a9dc3d5612ce ("sfc_ef100: RX filter table management and related gubbins") Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> Reviewed-by: Simon Horman <horms@kernel.org> Reviewed-by: Edward Cree <ecree.xilinx@gmail.com> --- drivers/net/ethernet/sfc/rx_common.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)