| Message ID | 1670401920-7574-1-git-send-email-zhangchangzhong@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f150b63f3fa5fdd81e0dd6151e8850268e29438c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: stmmac: selftests: fix potential memleak in stmmac_test_arpoffload() | expand |
On Wed, Dec 07, 2022 at 04:31:59PM +0800, Zhang Changzhong wrote: > The skb allocated by stmmac_test_get_arp_skb() hasn't been released in > some error handling case, which will lead to a memory leak. Fix this up > by adding kfree_skb() to release skb. > > Compile tested only. > > Fixes: 5e3fb0a6e2b3 ("net: stmmac: selftests: Implement the ARP Offload test") > Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c > index 49af7e7..687f43c 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c > @@ -1654,12 +1654,16 @@ static int stmmac_test_arpoffload(struct stmmac_priv *priv) > } > > ret = stmmac_set_arp_offload(priv, priv->hw, true, ip_addr); > - if (ret) > + if (ret) { > + kfree_skb(skb); > goto cleanup; > + } > > ret = dev_set_promiscuity(priv->dev, 1); > - if (ret) > + if (ret) { > + kfree_skb(skb); > goto cleanup; > + } > > ret = dev_direct_xmit(skb, 0); > if (ret) You should release skb here too. So the better patch will be to write something like that: cleanup: stmmac_set_arp_offload(priv, priv->hw, false, 0x0); if (ret) kfree_skb(skb); Thanks > -- > 2.9.5 >
On 2022/12/7 17:12, Leon Romanovsky wrote: > On Wed, Dec 07, 2022 at 04:31:59PM +0800, Zhang Changzhong wrote: >> The skb allocated by stmmac_test_get_arp_skb() hasn't been released in >> some error handling case, which will lead to a memory leak. Fix this up >> by adding kfree_skb() to release skb. >> >> Compile tested only. >> >> Fixes: 5e3fb0a6e2b3 ("net: stmmac: selftests: Implement the ARP Offload test") >> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c >> index 49af7e7..687f43c 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c >> @@ -1654,12 +1654,16 @@ static int stmmac_test_arpoffload(struct stmmac_priv *priv) >> } >> >> ret = stmmac_set_arp_offload(priv, priv->hw, true, ip_addr); >> - if (ret) >> + if (ret) { >> + kfree_skb(skb); >> goto cleanup; >> + } >> >> ret = dev_set_promiscuity(priv->dev, 1); >> - if (ret) >> + if (ret) { >> + kfree_skb(skb); >> goto cleanup; >> + } >> >> ret = dev_direct_xmit(skb, 0); >> if (ret) > > You should release skb here too. So the better patch will be to write > something like that: > Hi Leon, Thanks for your review, but I don't think we need release skb here, because dev_direct_xmit() is responsible for freeing it. Regards, Changzhong > cleanup: > stmmac_set_arp_offload(priv, priv->hw, false, 0x0); > if (ret) > kfree_skb(skb); >> Thanks > >> -- >> 2.9.5 >> > . >
On Wed, Dec 07, 2022 at 06:07:16PM +0800, Zhang Changzhong wrote: > On 2022/12/7 17:12, Leon Romanovsky wrote: > > On Wed, Dec 07, 2022 at 04:31:59PM +0800, Zhang Changzhong wrote: > >> The skb allocated by stmmac_test_get_arp_skb() hasn't been released in > >> some error handling case, which will lead to a memory leak. Fix this up > >> by adding kfree_skb() to release skb. > >> > >> Compile tested only. > >> > >> Fixes: 5e3fb0a6e2b3 ("net: stmmac: selftests: Implement the ARP Offload test") > >> Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> > >> --- > >> drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c > >> index 49af7e7..687f43c 100644 > >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c > >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c > >> @@ -1654,12 +1654,16 @@ static int stmmac_test_arpoffload(struct stmmac_priv *priv) > >> } > >> > >> ret = stmmac_set_arp_offload(priv, priv->hw, true, ip_addr); > >> - if (ret) > >> + if (ret) { > >> + kfree_skb(skb); > >> goto cleanup; > >> + } > >> > >> ret = dev_set_promiscuity(priv->dev, 1); > >> - if (ret) > >> + if (ret) { > >> + kfree_skb(skb); > >> goto cleanup; > >> + } > >> > >> ret = dev_direct_xmit(skb, 0); > >> if (ret) > > > > You should release skb here too. So the better patch will be to write > > something like that: > > > > Hi Leon, > > Thanks for your review, but I don't think we need release skb here, > because dev_direct_xmit() is responsible for freeing it. Interesting, __dev_direct_xmit() releases skb too. Thanks for the clarification. > > Regards, > Changzhong > > > cleanup: > > stmmac_set_arp_offload(priv, priv->hw, false, 0x0); > > if (ret) > > kfree_skb(skb); > >> Thanks > > > >> -- > >> 2.9.5 > >> > > . > >
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Wed, 7 Dec 2022 16:31:59 +0800 you wrote: > The skb allocated by stmmac_test_get_arp_skb() hasn't been released in > some error handling case, which will lead to a memory leak. Fix this up > by adding kfree_skb() to release skb. > > Compile tested only. > > Fixes: 5e3fb0a6e2b3 ("net: stmmac: selftests: Implement the ARP Offload test") > Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> > > [...] Here is the summary with links: - [net] net: stmmac: selftests: fix potential memleak in stmmac_test_arpoffload() https://git.kernel.org/netdev/net/c/f150b63f3fa5 You are awesome, thank you!
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c index 49af7e7..687f43c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c @@ -1654,12 +1654,16 @@ static int stmmac_test_arpoffload(struct stmmac_priv *priv) } ret = stmmac_set_arp_offload(priv, priv->hw, true, ip_addr); - if (ret) + if (ret) { + kfree_skb(skb); goto cleanup; + } ret = dev_set_promiscuity(priv->dev, 1); - if (ret) + if (ret) { + kfree_skb(skb); goto cleanup; + } ret = dev_direct_xmit(skb, 0); if (ret)
The skb allocated by stmmac_test_get_arp_skb() hasn't been released in some error handling case, which will lead to a memory leak. Fix this up by adding kfree_skb() to release skb. Compile tested only. Fixes: 5e3fb0a6e2b3 ("net: stmmac: selftests: Implement the ARP Offload test") Signed-off-by: Zhang Changzhong <zhangchangzhong@huawei.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)