| Message ID | 20231214152247.3482788-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: fix a double-free bug in efx_probe_filters | expand |
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> ...
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 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>
> 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.
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: 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(-)