2023-04-24 11:04:13

by Gencen Gan

[permalink] [raw]
Subject: [PATCH v4] net: amd: Fix link leak when verifying config failed

After failing to verify configuration, it returns directly without
releasing link, which may cause memory leak.

Paolo Abeni thinks that the whole code of this driver is quite
"suboptimal" and looks unmainatained since at least ~15y, so he
suggests that we could simply remove the whole driver, please
take it into consideration.

Simon Horman suggests that the fix label should be set to
"Linux-2.6.12-rc2" considering that the problem has existed
since the driver was introduced and the commit above doesn't
seem to exist in net/net-next.

Fixes: 99c3b0265649 ("Linux-2.6.12-rc2")
Signed-off-by: Gan Gecen <[email protected]>
Reviewed-by: Paolo Abeni <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
---
v3->v4: modify the 'Fixes:' tag to make it more accurate.
drivers/net/ethernet/amd/nmclan_cs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amd/nmclan_cs.c b/drivers/net/ethernet/amd/nmclan_cs.c
index 823a329a921f..0dd391c84c13 100644
--- a/drivers/net/ethernet/amd/nmclan_cs.c
+++ b/drivers/net/ethernet/amd/nmclan_cs.c
@@ -651,7 +651,7 @@ static int nmclan_config(struct pcmcia_device *link)
} else {
pr_notice("mace id not found: %x %x should be 0x40 0x?9\n",
sig[0], sig[1]);
- return -ENODEV;
+ goto failed;
}
}

--
2.34.1


2023-04-24 13:06:41

by Simon Horman

[permalink] [raw]
Subject: Re: [PATCH v4] net: amd: Fix link leak when verifying config failed

On Mon, Apr 24, 2023 at 06:46:43PM +0800, Gencen Gan wrote:
> After failing to verify configuration, it returns directly without
> releasing link, which may cause memory leak.
>
> Paolo Abeni thinks that the whole code of this driver is quite
> "suboptimal" and looks unmainatained since at least ~15y, so he
> suggests that we could simply remove the whole driver, please
> take it into consideration.
>
> Simon Horman suggests that the fix label should be set to
> "Linux-2.6.12-rc2" considering that the problem has existed
> since the driver was introduced and the commit above doesn't
> seem to exist in net/net-next.
>
> Fixes: 99c3b0265649 ("Linux-2.6.12-rc2")

Unless I'm mistaken, this should be:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

> Signed-off-by: Gan Gecen <[email protected]>
> Reviewed-by: Paolo Abeni <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>

I think that tags such as Reviewed-by need to be given explicitly.
And as the above two Reviewed-by tags were not, so it is a bit
odd for them to appear above.

> ---
> v3->v4: modify the 'Fixes:' tag to make it more accurate.
> drivers/net/ethernet/amd/nmclan_cs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/amd/nmclan_cs.c b/drivers/net/ethernet/amd/nmclan_cs.c
> index 823a329a921f..0dd391c84c13 100644
> --- a/drivers/net/ethernet/amd/nmclan_cs.c
> +++ b/drivers/net/ethernet/amd/nmclan_cs.c
> @@ -651,7 +651,7 @@ static int nmclan_config(struct pcmcia_device *link)
> } else {
> pr_notice("mace id not found: %x %x should be 0x40 0x?9\n",
> sig[0], sig[1]);
> - return -ENODEV;
> + goto failed;
> }
> }
>
> --
> 2.34.1
>

2023-04-24 13:34:23

by Dongliang Mu

[permalink] [raw]
Subject: Re: [PATCH v4] net: amd: Fix link leak when verifying config failed



On 4/24/23 21:01, Simon Horman wrote:
> On Mon, Apr 24, 2023 at 06:46:43PM +0800, Gencen Gan wrote:
>> After failing to verify configuration, it returns directly without
>> releasing link, which may cause memory leak.
>>
>> Paolo Abeni thinks that the whole code of this driver is quite
>> "suboptimal" and looks unmainatained since at least ~15y, so he
>> suggests that we could simply remove the whole driver, please
>> take it into consideration.
>>
>> Simon Horman suggests that the fix label should be set to
>> "Linux-2.6.12-rc2" considering that the problem has existed
>> since the driver was introduced and the commit above doesn't
>> seem to exist in net/net-next.
>>
>> Fixes: 99c3b0265649 ("Linux-2.6.12-rc2")
>
> Unless I'm mistaken, this should be:
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>
>> Signed-off-by: Gan Gecen <[email protected]>
>> Reviewed-by: Paolo Abeni <[email protected]>
>> Reviewed-by: Simon Horman <[email protected]>
>
> I think that tags such as Reviewed-by need to be given explicitly.
> And as the above two Reviewed-by tags were not, so it is a bit
> odd for them to appear above.

Hi Semon,

Sorry about the naive mistakes made by Gan Gecen. Our team had
repeatedly talked about this issue and written it in our kernel
contribution guidance.

[1]
https://groups.google.com/g/hust-os-kernel-patches/c/GThhx08kecg/m/5p61iz6KAQAJ

>
>> ---
>> v3->v4: modify the 'Fixes:' tag to make it more accurate.
>> drivers/net/ethernet/amd/nmclan_cs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/amd/nmclan_cs.c b/drivers/net/ethernet/amd/nmclan_cs.c
>> index 823a329a921f..0dd391c84c13 100644
>> --- a/drivers/net/ethernet/amd/nmclan_cs.c
>> +++ b/drivers/net/ethernet/amd/nmclan_cs.c
>> @@ -651,7 +651,7 @@ static int nmclan_config(struct pcmcia_device *link)
>> } else {
>> pr_notice("mace id not found: %x %x should be 0x40 0x?9\n",
>> sig[0], sig[1]);
>> - return -ENODEV;
>> + goto failed;
>> }
>> }
>>
>> --
>> 2.34.1
>>
>