* [PATCH] pds_core: Fix possible double free in error handling path
@ 2024-03-01 2:23 hyper
2024-03-01 17:55 ` Nelson, Shannon
0 siblings, 1 reply; 10+ messages in thread
From: hyper @ 2024-03-01 2:23 UTC (permalink / raw
To: shannon.nelson, brett.creeley, davem, edumazet, kuba, pabeni
Cc: netdev, linux-kernel, jitxie, huntazhang, hyper
When auxiliary_device_add() returns error and then calls
auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
calls kfree(padev) to free memory. We shouldn't call kfree(padev)
again in the error handling path.
Fix this by returning error directly after calling
auxiliary_device_uninit().
Fixes: 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
Signed-off-by: hyper <hyperlyzcs@gmail.com>
---
drivers/net/ethernet/amd/pds_core/auxbus.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
index 11c23a7f3172..d6eedd78d5cc 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -174,6 +174,8 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
err_out_uninit:
auxiliary_device_uninit(aux_dev);
+ return ERR_PTR(err);
+
err_out:
kfree(padev);
return ERR_PTR(err);
--
2.36.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] pds_core: Fix possible double free in error handling path
2024-03-01 2:23 [PATCH] pds_core: Fix possible double free in error handling path hyper
@ 2024-03-01 17:55 ` Nelson, Shannon
2024-03-03 8:49 ` [PATCH net V2] net: " hyper
0 siblings, 1 reply; 10+ messages in thread
From: Nelson, Shannon @ 2024-03-01 17:55 UTC (permalink / raw
To: hyper, brett.creeley, davem, edumazet, kuba, pabeni
Cc: netdev, linux-kernel, jitxie, huntazhang
On 2/29/2024 6:23 PM, hyper wrote:
>
Please specify the networking tree in your patch subject, something like
[PATCH net] in this case.
>
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
> calls kfree(padev) to free memory. We shouldn't call kfree(padev)
> again in the error handling path.
>
> Fix this by returning error directly after calling
> auxiliary_device_uninit().
>
> Fixes: 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
> Signed-off-by: hyper <hyperlyzcs@gmail.com>
> ---
> drivers/net/ethernet/amd/pds_core/auxbus.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
> index 11c23a7f3172..d6eedd78d5cc 100644
> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
> @@ -174,6 +174,8 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
>
> err_out_uninit:
> auxiliary_device_uninit(aux_dev);
> + return ERR_PTR(err);
> +
> err_out:
> kfree(padev);
> return ERR_PTR(err);
> --
> 2.36.1
>
Yes, I think you've got the right idea here, and this is probably a
reasonable solution.
However, usually the error handling exit code stacks on itself, but here
it becomes two separate independent chunks - a slightly different
pattern. Since these are both very short bits I'd be tempted to
"enhance" that independence by putting the error handling back to where
the errors happened, something like
diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c
b/drivers/net/ethernet/amd/pds_core/auxbus.c
index a3c79848a69a..2babea110991 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -160,23 +160,19 @@ static struct pds_auxiliary_dev
*pdsc_auxbus_dev_register(struct pdsc *cf,
if (err < 0) {
dev_warn(cf->dev, "auxiliary_device_init of %s failed:
%pe\n",
name, ERR_PTR(err));
- goto err_out;
+ kfree(padev);
+ return ERR_PTR(err);
}
err = auxiliary_device_add(aux_dev);
if (err) {
dev_warn(cf->dev, "auxiliary_device_add of %s failed:
%pe\n",
name, ERR_PTR(err));
- goto err_out_uninit;
+ auxiliary_device_uninit(aux_dev);
+ return ERR_PTR(err);
}
return padev;
-
-err_out_uninit:
- auxiliary_device_uninit(aux_dev);
-err_out:
- kfree(padev);
- return ERR_PTR(err);
}
Some might disagree. I like this a little better, but I could go either way.
Thoughts?
sln
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net V2] net: pds_core: Fix possible double free in error handling path
2024-03-01 17:55 ` Nelson, Shannon
@ 2024-03-03 8:49 ` hyper
2024-03-04 17:21 ` Nelson, Shannon
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: hyper @ 2024-03-03 8:49 UTC (permalink / raw
To: shannon.nelson, brett.creeley, davem, edumazet, kuba, pabeni
Cc: netdev, linux-kernel, jitxie, huntazhang, hyper
When auxiliary_device_add() returns error and then calls
auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
calls kfree(padev) to free memory. We shouldn't call kfree(padev)
again in the error handling path.
Fix this by cleaning up the redundant kfree() and putting
the error handling back to where the errors happened.
Fixes: 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
Signed-off-by: hyper <hyperlyzcs@gmail.com>
---
drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
index 11c23a7f3172..fd1a5149c003 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
if (err < 0) {
dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
name, ERR_PTR(err));
- goto err_out;
+ kfree(padev);
+ return ERR_PTR(err);
}
err = auxiliary_device_add(aux_dev);
if (err) {
dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
name, ERR_PTR(err));
- goto err_out_uninit;
+ auxiliary_device_uninit(aux_dev);
+ return ERR_PTR(err);
}
return padev;
-
-err_out_uninit:
- auxiliary_device_uninit(aux_dev);
-err_out:
- kfree(padev);
- return ERR_PTR(err);
}
int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
--
2.36.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net V2] net: pds_core: Fix possible double free in error handling path
2024-03-03 8:49 ` [PATCH net V2] net: " hyper
@ 2024-03-04 17:21 ` Nelson, Shannon
2024-03-04 20:26 ` Breno Leitao
2024-03-05 14:58 ` Paolo Abeni
2 siblings, 0 replies; 10+ messages in thread
From: Nelson, Shannon @ 2024-03-04 17:21 UTC (permalink / raw
To: hyper, brett.creeley, davem, edumazet, kuba, pabeni
Cc: netdev, linux-kernel, jitxie, huntazhang
On 3/3/2024 12:49 AM, hyper wrote:
>
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
> calls kfree(padev) to free memory. We shouldn't call kfree(padev)
> again in the error handling path.
>
> Fix this by cleaning up the redundant kfree() and putting
> the error handling back to where the errors happened.
>
> Fixes: 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
> Signed-off-by: hyper <hyperlyzcs@gmail.com>
Thanks.
Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
> drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
> index 11c23a7f3172..fd1a5149c003 100644
> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
> @@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
> if (err < 0) {
> dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
> name, ERR_PTR(err));
> - goto err_out;
> + kfree(padev);
> + return ERR_PTR(err);
> }
>
> err = auxiliary_device_add(aux_dev);
> if (err) {
> dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
> name, ERR_PTR(err));
> - goto err_out_uninit;
> + auxiliary_device_uninit(aux_dev);
> + return ERR_PTR(err);
> }
>
> return padev;
> -
> -err_out_uninit:
> - auxiliary_device_uninit(aux_dev);
> -err_out:
> - kfree(padev);
> - return ERR_PTR(err);
> }
>
> int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net V2] net: pds_core: Fix possible double free in error handling path
2024-03-03 8:49 ` [PATCH net V2] net: " hyper
2024-03-04 17:21 ` Nelson, Shannon
@ 2024-03-04 20:26 ` Breno Leitao
2024-03-05 14:58 ` Paolo Abeni
2 siblings, 0 replies; 10+ messages in thread
From: Breno Leitao @ 2024-03-04 20:26 UTC (permalink / raw
To: hyper
Cc: shannon.nelson, brett.creeley, davem, edumazet, kuba, pabeni,
netdev, linux-kernel, jitxie, huntazhang
On Sun, Mar 03, 2024 at 04:49:54PM +0800, hyper wrote:
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
> calls kfree(padev) to free memory. We shouldn't call kfree(padev)
> again in the error handling path.
>
> Fix this by cleaning up the redundant kfree() and putting
> the error handling back to where the errors happened.
>
> Fixes: 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
> Signed-off-by: hyper <hyperlyzcs@gmail.com>
I liked this v2 better.
Reviewed-by: Breno Leitao <leitao@debian.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net V2] net: pds_core: Fix possible double free in error handling path
2024-03-03 8:49 ` [PATCH net V2] net: " hyper
2024-03-04 17:21 ` Nelson, Shannon
2024-03-04 20:26 ` Breno Leitao
@ 2024-03-05 14:58 ` Paolo Abeni
2024-03-06 10:57 ` [PATCH net V3] " Yongzhi Liu
2 siblings, 1 reply; 10+ messages in thread
From: Paolo Abeni @ 2024-03-05 14:58 UTC (permalink / raw
To: hyper, shannon.nelson, brett.creeley, davem, edumazet, kuba
Cc: netdev, linux-kernel, jitxie, huntazhang
On Sun, 2024-03-03 at 16:49 +0800, hyper wrote:
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
> calls kfree(padev) to free memory. We shouldn't call kfree(padev)
> again in the error handling path.
>
> Fix this by cleaning up the redundant kfree() and putting
> the error handling back to where the errors happened.
>
> Fixes: 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
> Signed-off-by: hyper <hyperlyzcs@gmail.com>
Note that submitters are required to use real identity:
https://elixir.bootlin.com/linux/v6.8-rc7/source/Documentation/process/submitting-patches.rst#L438
Could you please repost avoiding the nick name?
You can retain the already collected acks.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net V3] net: pds_core: Fix possible double free in error handling path
2024-03-05 14:58 ` Paolo Abeni
@ 2024-03-06 10:57 ` Yongzhi Liu
2024-03-06 13:44 ` Wojciech Drewek
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Yongzhi Liu @ 2024-03-06 10:57 UTC (permalink / raw
To: pabeni, shannon.nelson, brett.creeley, davem, edumazet, kuba
Cc: netdev, linux-kernel, jitxie, huntazhang, Yongzhi Liu
When auxiliary_device_add() returns error and then calls
auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
calls kfree(padev) to free memory. We shouldn't call kfree(padev)
again in the error handling path.
Fix this by cleaning up the redundant kfree() and putting
the error handling back to where the errors happened.
Fixes: 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
Signed-off-by: Yongzhi Liu <hyperlyzcs@gmail.com>
---
drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
index 11c23a7f3172..fd1a5149c003 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
if (err < 0) {
dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
name, ERR_PTR(err));
- goto err_out;
+ kfree(padev);
+ return ERR_PTR(err);
}
err = auxiliary_device_add(aux_dev);
if (err) {
dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
name, ERR_PTR(err));
- goto err_out_uninit;
+ auxiliary_device_uninit(aux_dev);
+ return ERR_PTR(err);
}
return padev;
-
-err_out_uninit:
- auxiliary_device_uninit(aux_dev);
-err_out:
- kfree(padev);
- return ERR_PTR(err);
}
int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
--
2.36.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net V3] net: pds_core: Fix possible double free in error handling path
2024-03-06 10:57 ` [PATCH net V3] " Yongzhi Liu
@ 2024-03-06 13:44 ` Wojciech Drewek
2024-03-06 17:05 ` Nelson, Shannon
2024-03-07 11:10 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 10+ messages in thread
From: Wojciech Drewek @ 2024-03-06 13:44 UTC (permalink / raw
To: Yongzhi Liu, pabeni, shannon.nelson, brett.creeley, davem,
edumazet, kuba
Cc: netdev, linux-kernel, jitxie, huntazhang
On 06.03.2024 11:57, Yongzhi Liu wrote:
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
> calls kfree(padev) to free memory. We shouldn't call kfree(padev)
> again in the error handling path.
>
> Fix this by cleaning up the redundant kfree() and putting
> the error handling back to where the errors happened.
>
> Fixes: 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
> Signed-off-by: Yongzhi Liu <hyperlyzcs@gmail.com>
> ---
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
> index 11c23a7f3172..fd1a5149c003 100644
> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
> @@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
> if (err < 0) {
> dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
> name, ERR_PTR(err));
> - goto err_out;
> + kfree(padev);
> + return ERR_PTR(err);
> }
>
> err = auxiliary_device_add(aux_dev);
> if (err) {
> dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
> name, ERR_PTR(err));
> - goto err_out_uninit;
> + auxiliary_device_uninit(aux_dev);
> + return ERR_PTR(err);
> }
>
> return padev;
> -
> -err_out_uninit:
> - auxiliary_device_uninit(aux_dev);
> -err_out:
> - kfree(padev);
> - return ERR_PTR(err);
> }
>
> int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net V3] net: pds_core: Fix possible double free in error handling path
2024-03-06 10:57 ` [PATCH net V3] " Yongzhi Liu
2024-03-06 13:44 ` Wojciech Drewek
@ 2024-03-06 17:05 ` Nelson, Shannon
2024-03-07 11:10 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 10+ messages in thread
From: Nelson, Shannon @ 2024-03-06 17:05 UTC (permalink / raw
To: Yongzhi Liu, pabeni, brett.creeley, davem, edumazet, kuba
Cc: netdev, linux-kernel, jitxie, huntazhang
On 3/6/2024 2:57 AM, Yongzhi Liu wrote:
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
> calls kfree(padev) to free memory. We shouldn't call kfree(padev)
> again in the error handling path.
>
> Fix this by cleaning up the redundant kfree() and putting
> the error handling back to where the errors happened.
>
> Fixes: 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
> Signed-off-by: Yongzhi Liu <hyperlyzcs@gmail.com>
Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
> drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
> index 11c23a7f3172..fd1a5149c003 100644
> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
> @@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
> if (err < 0) {
> dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
> name, ERR_PTR(err));
> - goto err_out;
> + kfree(padev);
> + return ERR_PTR(err);
> }
>
> err = auxiliary_device_add(aux_dev);
> if (err) {
> dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
> name, ERR_PTR(err));
> - goto err_out_uninit;
> + auxiliary_device_uninit(aux_dev);
> + return ERR_PTR(err);
> }
>
> return padev;
> -
> -err_out_uninit:
> - auxiliary_device_uninit(aux_dev);
> -err_out:
> - kfree(padev);
> - return ERR_PTR(err);
> }
>
> int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net V3] net: pds_core: Fix possible double free in error handling path
2024-03-06 10:57 ` [PATCH net V3] " Yongzhi Liu
2024-03-06 13:44 ` Wojciech Drewek
2024-03-06 17:05 ` Nelson, Shannon
@ 2024-03-07 11:10 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-03-07 11:10 UTC (permalink / raw
To: Yongzhi Liu
Cc: pabeni, shannon.nelson, brett.creeley, davem, edumazet, kuba,
netdev, linux-kernel, jitxie, huntazhang
Hello:
This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:
On Wed, 6 Mar 2024 18:57:14 +0800 you wrote:
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release
> calls kfree(padev) to free memory. We shouldn't call kfree(padev)
> again in the error handling path.
>
> Fix this by cleaning up the redundant kfree() and putting
> the error handling back to where the errors happened.
>
> [...]
Here is the summary with links:
- [net,V3] net: pds_core: Fix possible double free in error handling path
https://git.kernel.org/netdev/net/c/ba18deddd6d5
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-03-07 11:10 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-01 2:23 [PATCH] pds_core: Fix possible double free in error handling path hyper
2024-03-01 17:55 ` Nelson, Shannon
2024-03-03 8:49 ` [PATCH net V2] net: " hyper
2024-03-04 17:21 ` Nelson, Shannon
2024-03-04 20:26 ` Breno Leitao
2024-03-05 14:58 ` Paolo Abeni
2024-03-06 10:57 ` [PATCH net V3] " Yongzhi Liu
2024-03-06 13:44 ` Wojciech Drewek
2024-03-06 17:05 ` Nelson, Shannon
2024-03-07 11:10 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.