[PATCH] net: ethernet: Fix memleak in ethoc_probe

[PATCH] net: ethernet: Fix memleak in ethoc_probe

[Dinghao Liu]

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(-)

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);
-- 
2.17.1

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Andrew Lunn]

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

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Jakub Kicinski]

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 :)

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Andrew Lunn]

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

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Jakub Kicinski]

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

:(

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Florian Fainelli]

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
-- 
Florian

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Jakub Kicinski]

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?

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Konstantin Ryabitsev]

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

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Konstantin Ryabitsev]

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

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Florian Fainelli]

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?
-- 
Florian

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Konstantin Ryabitsev]

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

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Jakub Kicinski]

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.

Re: Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[dinghao.liu]

> 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 

Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[Jakub Kicinski]

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.

Re: Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe

[dinghao.liu]

> 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