| Message ID | 20221205061515.115012-1-yuancan@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4fad22a1281c500f15b172c9d261eff347ca634b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dpaa2-switch: Fix memory leak in dpaa2_switch_acl_entry_add() and dpaa2_switch_acl_entry_remove() | expand |
Hello: This patch was applied to netdev/net.git (master) by Paolo Abeni <pabeni@redhat.com>: On Mon, 5 Dec 2022 06:15:15 +0000 you wrote: > The cmd_buff needs to be freed when error happened in > dpaa2_switch_acl_entry_add() and dpaa2_switch_acl_entry_remove(). > > Fixes: 1110318d83e8 ("dpaa2-switch: add tc flower hardware offload on ingress traffic") > Signed-off-by: Yuan Can <yuancan@huawei.com> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c | 4 ++++ > 1 file changed, 4 insertions(+) Here is the summary with links: - dpaa2-switch: Fix memory leak in dpaa2_switch_acl_entry_add() and dpaa2_switch_acl_entry_remove() https://git.kernel.org/netdev/net/c/4fad22a1281c You are awesome, thank you!
Hi Yuan, On Mon, Dec 05, 2022 at 06:15:15AM +0000, Yuan Can wrote: > The cmd_buff needs to be freed when error happened in > dpaa2_switch_acl_entry_add() and dpaa2_switch_acl_entry_remove(). > > Fixes: 1110318d83e8 ("dpaa2-switch: add tc flower hardware offload on ingress traffic") > Signed-off-by: Yuan Can <yuancan@huawei.com> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c > index cacd454ac696..c39b866e2582 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c > @@ -132,6 +132,7 @@ int dpaa2_switch_acl_entry_add(struct dpaa2_switch_filter_block *filter_block, > DMA_TO_DEVICE); > if (unlikely(dma_mapping_error(dev, acl_entry_cfg->key_iova))) { > dev_err(dev, "DMA mapping failed\n"); > + kfree(cmd_buff); > return -EFAULT; > } > > @@ -142,6 +143,7 @@ int dpaa2_switch_acl_entry_add(struct dpaa2_switch_filter_block *filter_block, > DMA_TO_DEVICE); > if (err) { > dev_err(dev, "dpsw_acl_add_entry() failed %d\n", err); > + kfree(cmd_buff); To reduce the number of kfree() calls, this last one can be put right before checking for error, and we could remove the kfree(cmd_buff) call at the very end. I mean that was already the intention, if you look at the dma_unmap_single() call compared to the error checking. Like this: err = dpsw_acl_add_entry(...); dma_unmap_single(dev, acl_entry_cfg->key_iova, sizeof(cmd_buff), DMA_TO_DEVICE); kfree(cmd_buff); if (err) { dev_err(dev, "dpsw_acl_add_entry() failed %d\n", err); return err; } return 0; } > return err; > } > > @@ -172,6 +174,7 @@ dpaa2_switch_acl_entry_remove(struct dpaa2_switch_filter_block *block, > DMA_TO_DEVICE); > if (unlikely(dma_mapping_error(dev, acl_entry_cfg->key_iova))) { > dev_err(dev, "DMA mapping failed\n"); > + kfree(cmd_buff); > return -EFAULT; > } > > @@ -182,6 +185,7 @@ dpaa2_switch_acl_entry_remove(struct dpaa2_switch_filter_block *block, > DMA_TO_DEVICE); > if (err) { > dev_err(dev, "dpsw_acl_remove_entry() failed %d\n", err); > + kfree(cmd_buff); Similar here: err = dpsw_acl_remove_entry(ethsw->mc_io, 0, ethsw->dpsw_handle, block->acl_id, acl_entry_cfg); dma_unmap_single(dev, acl_entry_cfg->key_iova, sizeof(cmd_buff), DMA_TO_DEVICE); kfree(cmd_buff); if (err) { dev_err(dev, "dpsw_acl_remove_entry() failed %d\n", err); return err; } return 0; } > return err; > } > > -- > 2.17.1 >
在 2022/12/7 19:55, Vladimir Oltean 写道: > Hi Yuan, > > On Mon, Dec 05, 2022 at 06:15:15AM +0000, Yuan Can wrote: >> The cmd_buff needs to be freed when error happened in >> dpaa2_switch_acl_entry_add() and dpaa2_switch_acl_entry_remove(). >> >> Fixes: 1110318d83e8 ("dpaa2-switch: add tc flower hardware offload on ingress traffic") >> Signed-off-by: Yuan Can <yuancan@huawei.com> >> --- >> drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c >> index cacd454ac696..c39b866e2582 100644 >> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c >> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c >> @@ -132,6 +132,7 @@ int dpaa2_switch_acl_entry_add(struct dpaa2_switch_filter_block *filter_block, >> DMA_TO_DEVICE); >> if (unlikely(dma_mapping_error(dev, acl_entry_cfg->key_iova))) { >> dev_err(dev, "DMA mapping failed\n"); >> + kfree(cmd_buff); >> return -EFAULT; >> } >> >> @@ -142,6 +143,7 @@ int dpaa2_switch_acl_entry_add(struct dpaa2_switch_filter_block *filter_block, >> DMA_TO_DEVICE); >> if (err) { >> dev_err(dev, "dpsw_acl_add_entry() failed %d\n", err); >> + kfree(cmd_buff); > To reduce the number of kfree() calls, this last one can be put right > before checking for error, and we could remove the kfree(cmd_buff) call at > the very end. I mean that was already the intention, if you look at the > dma_unmap_single() call compared to the error checking. Like this: > > err = dpsw_acl_add_entry(...); > > dma_unmap_single(dev, acl_entry_cfg->key_iova, sizeof(cmd_buff), > DMA_TO_DEVICE); > kfree(cmd_buff); > > if (err) { > dev_err(dev, "dpsw_acl_add_entry() failed %d\n", err); > return err; > } > > return 0; > } Nice! Thanks for the suggestion, as the patch has been merged, let me send another patch to do this job.
On Thu, Dec 08, 2022 at 09:51:45AM +0800, Yuan Can wrote: > Nice! Thanks for the suggestion, as the patch has been merged, let me send > another patch to do this job. The patch was merged at the same time as I was writing my reply. Since the patch went to the "net.git" tree where only bug fixes are accepted, any further rework would have to go to "net-next.git". A resynchronization (merge net into net-next) takes place weekly, one should take place later today or tomorrow, I think. So if you're going to send a follow-up patch, I'd wait until "net-next" contains your change. That being said, it's not all that critical to follow up with another patch. I simply hadn't noticed your patch was merged, but it does the job as well, just not in the same style as the rest of these 2 functions.
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c index cacd454ac696..c39b866e2582 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c @@ -132,6 +132,7 @@ int dpaa2_switch_acl_entry_add(struct dpaa2_switch_filter_block *filter_block, DMA_TO_DEVICE); if (unlikely(dma_mapping_error(dev, acl_entry_cfg->key_iova))) { dev_err(dev, "DMA mapping failed\n"); + kfree(cmd_buff); return -EFAULT; } @@ -142,6 +143,7 @@ int dpaa2_switch_acl_entry_add(struct dpaa2_switch_filter_block *filter_block, DMA_TO_DEVICE); if (err) { dev_err(dev, "dpsw_acl_add_entry() failed %d\n", err); + kfree(cmd_buff); return err; } @@ -172,6 +174,7 @@ dpaa2_switch_acl_entry_remove(struct dpaa2_switch_filter_block *block, DMA_TO_DEVICE); if (unlikely(dma_mapping_error(dev, acl_entry_cfg->key_iova))) { dev_err(dev, "DMA mapping failed\n"); + kfree(cmd_buff); return -EFAULT; } @@ -182,6 +185,7 @@ dpaa2_switch_acl_entry_remove(struct dpaa2_switch_filter_block *block, DMA_TO_DEVICE); if (err) { dev_err(dev, "dpsw_acl_remove_entry() failed %d\n", err); + kfree(cmd_buff); return err; }
The cmd_buff needs to be freed when error happened in dpaa2_switch_acl_entry_add() and dpaa2_switch_acl_entry_remove(). Fixes: 1110318d83e8 ("dpaa2-switch: add tc flower hardware offload on ingress traffic") Signed-off-by: Yuan Can <yuancan@huawei.com> --- drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c | 4 ++++ 1 file changed, 4 insertions(+)