| Message ID | 20201223110615.31389-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: Fix memleak in ethoc_probe | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 4 this patch: 4 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 15 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 4 this patch: 4 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, Dec 23, 2020 at 07:06:12PM +0800, Dinghao Liu wrote: > When mdiobus_register() fails, priv->mdio allocated > by mdiobus_alloc() has not been freed, which leads > to memleak. > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> Fixes: bfa49cfc5262 ("net/ethoc: fix null dereference on error exit path") Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, 23 Dec 2020 16:33:04 +0100 Andrew Lunn wrote: > On Wed, Dec 23, 2020 at 07:06:12PM +0800, Dinghao Liu wrote: > > When mdiobus_register() fails, priv->mdio allocated > > by mdiobus_alloc() has not been freed, which leads > > to memleak. > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > Fixes: bfa49cfc5262 ("net/ethoc: fix null dereference on error exit path") > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Ooof, I applied without looking at your email and I added: Fixes: e7f4dc3536a4 ("mdio: Move allocation of interrupts into core") I believe this is the correct Fixes tag. Before the exit patch was freeing both MDIO and the IRQ depending on the fact that kfree(NULL) is fine. Am I wrong? Not that we can fix it now :)
On Wed, Dec 23, 2020 at 12:32:18PM -0800, Jakub Kicinski wrote: > On Wed, 23 Dec 2020 16:33:04 +0100 Andrew Lunn wrote: > > On Wed, Dec 23, 2020 at 07:06:12PM +0800, Dinghao Liu wrote: > > > When mdiobus_register() fails, priv->mdio allocated > > > by mdiobus_alloc() has not been freed, which leads > > > to memleak. > > > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > > > Fixes: bfa49cfc5262 ("net/ethoc: fix null dereference on error exit path") > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Ooof, I applied without looking at your email and I added: > > Fixes: e7f4dc3536a4 ("mdio: Move allocation of interrupts into core") [Goes and looks deeper] Yes, commit e7f4dc3536a4 looks like it introduced the original problem. bfa49cfc5262 just moved to code around a bit. Does patchwork not automagically add Fixes: lines from full up emails? That seems like a reasonable automation. Andrew
On Wed, 23 Dec 2020 22:00:44 +0100 Andrew Lunn wrote: > On Wed, Dec 23, 2020 at 12:32:18PM -0800, Jakub Kicinski wrote: > > On Wed, 23 Dec 2020 16:33:04 +0100 Andrew Lunn wrote: > > > On Wed, Dec 23, 2020 at 07:06:12PM +0800, Dinghao Liu wrote: > > > > When mdiobus_register() fails, priv->mdio allocated > > > > by mdiobus_alloc() has not been freed, which leads > > > > to memleak. > > > > > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > > > > > Fixes: bfa49cfc5262 ("net/ethoc: fix null dereference on error exit path") > > > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > Ooof, I applied without looking at your email and I added: > > > > Fixes: e7f4dc3536a4 ("mdio: Move allocation of interrupts into core") > > [Goes and looks deeper] > > Yes, commit e7f4dc3536a4 looks like it introduced the original > problem. bfa49cfc5262 just moved to code around a bit. > > Does patchwork not automagically add Fixes: lines from full up emails? > That seems like a reasonable automation. Looks like it's been a TODO for 3 years now: https://github.com/getpatchwork/patchwork/issues/151 :(
On 12/23/2020 1:11 PM, Jakub Kicinski wrote: > On Wed, 23 Dec 2020 22:00:44 +0100 Andrew Lunn wrote: >> On Wed, Dec 23, 2020 at 12:32:18PM -0800, Jakub Kicinski wrote: >>> On Wed, 23 Dec 2020 16:33:04 +0100 Andrew Lunn wrote: >>>> On Wed, Dec 23, 2020 at 07:06:12PM +0800, Dinghao Liu wrote: >>>>> When mdiobus_register() fails, priv->mdio allocated >>>>> by mdiobus_alloc() has not been freed, which leads >>>>> to memleak. >>>>> >>>>> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> >>>> >>>> Fixes: bfa49cfc5262 ("net/ethoc: fix null dereference on error exit path") >>>> >>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> >>> >>> Ooof, I applied without looking at your email and I added: >>> >>> Fixes: e7f4dc3536a4 ("mdio: Move allocation of interrupts into core") >> >> [Goes and looks deeper] >> >> Yes, commit e7f4dc3536a4 looks like it introduced the original >> problem. bfa49cfc5262 just moved to code around a bit. >> >> Does patchwork not automagically add Fixes: lines from full up emails? >> That seems like a reasonable automation. > > Looks like it's been a TODO for 3 years now: > > https://github.com/getpatchwork/patchwork/issues/151 It was proposed before, but rejected. You can have your local patchwork admin take care of that for you though and add custom tags: https://lists.ozlabs.org/pipermail/patchwork/2017-January/003910.html
On Wed, 23 Dec 2020 14:17:29 -0800 Florian Fainelli wrote: > On 12/23/2020 1:11 PM, Jakub Kicinski wrote: > > On Wed, 23 Dec 2020 22:00:44 +0100 Andrew Lunn wrote: > >> On Wed, Dec 23, 2020 at 12:32:18PM -0800, Jakub Kicinski wrote: > >>> On Wed, 23 Dec 2020 16:33:04 +0100 Andrew Lunn wrote: > >>>> On Wed, Dec 23, 2020 at 07:06:12PM +0800, Dinghao Liu wrote: > >>>>> When mdiobus_register() fails, priv->mdio allocated > >>>>> by mdiobus_alloc() has not been freed, which leads > >>>>> to memleak. > >>>>> > >>>>> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > >>>> > >>>> Fixes: bfa49cfc5262 ("net/ethoc: fix null dereference on error exit path") > >>>> > >>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch> > >>> > >>> Ooof, I applied without looking at your email and I added: > >>> > >>> Fixes: e7f4dc3536a4 ("mdio: Move allocation of interrupts into core") > >> > >> [Goes and looks deeper] > >> > >> Yes, commit e7f4dc3536a4 looks like it introduced the original > >> problem. bfa49cfc5262 just moved to code around a bit. > >> > >> Does patchwork not automagically add Fixes: lines from full up emails? > >> That seems like a reasonable automation. > > > > Looks like it's been a TODO for 3 years now: > > > > https://github.com/getpatchwork/patchwork/issues/151 > > It was proposed before, but rejected. You can have your local patchwork > admin take care of that for you though and add custom tags: > > https://lists.ozlabs.org/pipermail/patchwork/2017-January/003910.html Konstantin, would you be willing to mod the kernel.org instance of patchwork to populate Fixes tags in the generated mboxes?
On Wed, Dec 23, 2020 at 05:41:46PM -0800, Jakub Kicinski wrote: > > >> Does patchwork not automagically add Fixes: lines from full up emails? > > >> That seems like a reasonable automation. > > > > > > Looks like it's been a TODO for 3 years now: > > > > > > https://github.com/getpatchwork/patchwork/issues/151 > > > > It was proposed before, but rejected. You can have your local patchwork > > admin take care of that for you though and add custom tags: > > > > https://lists.ozlabs.org/pipermail/patchwork/2017-January/003910.html > > Konstantin, would you be willing to mod the kernel.org instance of > patchwork to populate Fixes tags in the generated mboxes? I'd really rather not -- we try not to diverge from project upstream if at all possible, as this dramatically complicates upgrades. -K
On 12/24/2020 10:06 AM, Konstantin Ryabitsev wrote: > On Wed, Dec 23, 2020 at 05:41:46PM -0800, Jakub Kicinski wrote: >>>>> Does patchwork not automagically add Fixes: lines from full up emails? >>>>> That seems like a reasonable automation. >>>> >>>> Looks like it's been a TODO for 3 years now: >>>> >>>> https://github.com/getpatchwork/patchwork/issues/151 >>> >>> It was proposed before, but rejected. You can have your local patchwork >>> admin take care of that for you though and add custom tags: >>> >>> https://lists.ozlabs.org/pipermail/patchwork/2017-January/003910.html >> >> Konstantin, would you be willing to mod the kernel.org instance of >> patchwork to populate Fixes tags in the generated mboxes? > > I'd really rather not -- we try not to diverge from project upstream if at all > possible, as this dramatically complicates upgrades. Well that is really unfortunate then because the Linux developer community settled on using the Fixes: tag for years now and having patchwork automatically append those tags would greatly help maintainers.
On Thu, Dec 24, 2020 at 01:57:40PM -0800, Florian Fainelli wrote: > >> Konstantin, would you be willing to mod the kernel.org instance of > >> patchwork to populate Fixes tags in the generated mboxes? > > > > I'd really rather not -- we try not to diverge from project upstream if at all > > possible, as this dramatically complicates upgrades. > > Well that is really unfortunate then because the Linux developer > community settled on using the Fixes: tag for years now and having > patchwork automatically append those tags would greatly help maintainers. I agree -- but this is something that needs to be implemented upstream. Picking up a one-off patch just for patchwork.kernel.org is not the right way to go about this. -K
On 12/28/2020 12:23 PM, Konstantin Ryabitsev wrote: > On Thu, Dec 24, 2020 at 01:57:40PM -0800, Florian Fainelli wrote: >>>> Konstantin, would you be willing to mod the kernel.org instance of >>>> patchwork to populate Fixes tags in the generated mboxes? >>> >>> I'd really rather not -- we try not to diverge from project upstream if at all >>> possible, as this dramatically complicates upgrades. >> >> Well that is really unfortunate then because the Linux developer >> community settled on using the Fixes: tag for years now and having >> patchwork automatically append those tags would greatly help maintainers. > > I agree -- but this is something that needs to be implemented upstream. > Picking up a one-off patch just for patchwork.kernel.org is not the right way > to go about this. You should be able to tune this from the patchwork administrative interface and add new tags there, would not that be acceptable?
On Mon, Dec 28, 2020 at 01:05:26PM -0800, Florian Fainelli wrote: > On 12/28/2020 12:23 PM, Konstantin Ryabitsev wrote: > > On Thu, Dec 24, 2020 at 01:57:40PM -0800, Florian Fainelli wrote: > >>>> Konstantin, would you be willing to mod the kernel.org instance of > >>>> patchwork to populate Fixes tags in the generated mboxes? > >>> > >>> I'd really rather not -- we try not to diverge from project upstream if at all > >>> possible, as this dramatically complicates upgrades. > >> > >> Well that is really unfortunate then because the Linux developer > >> community settled on using the Fixes: tag for years now and having > >> patchwork automatically append those tags would greatly help maintainers. > > > > I agree -- but this is something that needs to be implemented upstream. > > Picking up a one-off patch just for patchwork.kernel.org is not the right way > > to go about this. > > You should be able to tune this from the patchwork administrative > interface and add new tags there, would not that be acceptable? Oh, oops, I got confused by the mention of a rejected upstream patch -- I didn't realize that this is already possible with a configuration setting. Sure, I added a match for ^Fixes: -- let me know if it's not doing the right thing. -K
On Mon, 28 Dec 2020 16:14:17 -0500 Konstantin Ryabitsev wrote: > On Mon, Dec 28, 2020 at 01:05:26PM -0800, Florian Fainelli wrote: > > On 12/28/2020 12:23 PM, Konstantin Ryabitsev wrote: > > > On Thu, Dec 24, 2020 at 01:57:40PM -0800, Florian Fainelli wrote: > > >>>> Konstantin, would you be willing to mod the kernel.org instance of > > >>>> patchwork to populate Fixes tags in the generated mboxes? > > >>> > > >>> I'd really rather not -- we try not to diverge from project upstream if at all > > >>> possible, as this dramatically complicates upgrades. > > >> > > >> Well that is really unfortunate then because the Linux developer > > >> community settled on using the Fixes: tag for years now and having > > >> patchwork automatically append those tags would greatly help maintainers. > > > > > > I agree -- but this is something that needs to be implemented upstream. > > > Picking up a one-off patch just for patchwork.kernel.org is not the right way > > > to go about this. > > > > You should be able to tune this from the patchwork administrative > > interface and add new tags there, would not that be acceptable? > > Oh, oops, I got confused by the mention of a rejected upstream patch -- I > didn't realize that this is already possible with a configuration setting. > > Sure, I added a match for ^Fixes: -- let me know if it's not doing the right > thing. I used this one for a test: https://patchwork.kernel.org/project/netdevbpf/patch/1609312994-121032-1-git-send-email-abaci-bugfix@linux.alibaba.com/ I'm not getting the Fixes tag when I download the mbox.
> On Mon, 28 Dec 2020 16:14:17 -0500 Konstantin Ryabitsev wrote: > > On Mon, Dec 28, 2020 at 01:05:26PM -0800, Florian Fainelli wrote: > > > On 12/28/2020 12:23 PM, Konstantin Ryabitsev wrote: > > > > On Thu, Dec 24, 2020 at 01:57:40PM -0800, Florian Fainelli wrote: > > > >>>> Konstantin, would you be willing to mod the kernel.org instance of > > > >>>> patchwork to populate Fixes tags in the generated mboxes? > > > >>> > > > >>> I'd really rather not -- we try not to diverge from project upstream if at all > > > >>> possible, as this dramatically complicates upgrades. > > > >> > > > >> Well that is really unfortunate then because the Linux developer > > > >> community settled on using the Fixes: tag for years now and having > > > >> patchwork automatically append those tags would greatly help maintainers. > > > > > > > > I agree -- but this is something that needs to be implemented upstream. > > > > Picking up a one-off patch just for patchwork.kernel.org is not the right way > > > > to go about this. > > > > > > You should be able to tune this from the patchwork administrative > > > interface and add new tags there, would not that be acceptable? > > > > Oh, oops, I got confused by the mention of a rejected upstream patch -- I > > didn't realize that this is already possible with a configuration setting. > > > > Sure, I added a match for ^Fixes: -- let me know if it's not doing the right > > thing. > > I used this one for a test: > > https://patchwork.kernel.org/project/netdevbpf/patch/1609312994-121032-1-git-send-email-abaci-bugfix@linux.alibaba.com/ > > I'm not getting the Fixes tag when I download the mbox. It seems that automatically generating Fixes tags is a hard work. Both patches and bugs could be complex. Sometimes even human cannot determine which commit introduced a target bug. Is this an already implemented functionality? Regards, Dinghao
On Wed, 6 Jan 2021 18:56:23 +0800 (GMT+08:00) dinghao.liu@zju.edu.cn wrote: > > I used this one for a test: > > > > https://patchwork.kernel.org/project/netdevbpf/patch/1609312994-121032-1-git-send-email-abaci-bugfix@linux.alibaba.com/ > > > > I'm not getting the Fixes tag when I download the mbox. > > It seems that automatically generating Fixes tags is a hard work. > Both patches and bugs could be complex. Sometimes even human cannot > determine which commit introduced a target bug. > > Is this an already implemented functionality? I'm not sure I understand. Indeed finding the right commit to use in a Fixes tag is not always easy, and definitely not easy to automate. Human validation is always required. If we could easily automate finding the commit which introduced a problem we wouldn't need to add the explicit tag, backporters could just run such script locally.. That's why it's best if the author does the digging and provides the right tag. The conversation with Konstantin and Florian was about automatically picking up Fixes tags from the mailing list by the patchwork software, when such tags are posted in reply to the original posting, just like review tags. But the tags are still generated by humans.
> On Wed, 6 Jan 2021 18:56:23 +0800 (GMT+08:00) dinghao.liu@zju.edu.cn > wrote: > > > I used this one for a test: > > > > > > https://patchwork.kernel.org/project/netdevbpf/patch/1609312994-121032-1-git-send-email-abaci-bugfix@linux.alibaba.com/ > > > > > > I'm not getting the Fixes tag when I download the mbox. > > > > It seems that automatically generating Fixes tags is a hard work. > > Both patches and bugs could be complex. Sometimes even human cannot > > determine which commit introduced a target bug. > > > > Is this an already implemented functionality? > > I'm not sure I understand. Indeed finding the right commit to use in > a Fixes tag is not always easy, and definitely not easy to automate. > Human validation is always required. > > If we could easily automate finding the commit which introduced a > problem we wouldn't need to add the explicit tag, backporters could > just run such script locally.. That's why it's best if the author > does the digging and provides the right tag. > > The conversation with Konstantin and Florian was about automatically > picking up Fixes tags from the mailing list by the patchwork software, > when such tags are posted in reply to the original posting, just like > review tags. But the tags are still generated by humans. It's clear to me, thanks. Regards, Dinghao
diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 0981fe9652e5..3d9b0b161e24 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -1211,7 +1211,7 @@ static int ethoc_probe(struct platform_device *pdev) ret = mdiobus_register(priv->mdio); if (ret) { dev_err(&netdev->dev, "failed to register MDIO bus\n"); - goto free2; + goto free3; } ret = ethoc_mdio_probe(netdev); @@ -1243,6 +1243,7 @@ static int ethoc_probe(struct platform_device *pdev) netif_napi_del(&priv->napi); error: mdiobus_unregister(priv->mdio); +free3: mdiobus_free(priv->mdio); free2: clk_disable_unprepare(priv->clk);
When mdiobus_register() fails, priv->mdio allocated by mdiobus_alloc() has not been freed, which leads to memleak. Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> --- drivers/net/ethernet/ethoc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)