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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
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 <[email protected]>
Thanks.
Reviewed-by: Shannon Nelson <[email protected]>
> ---
> 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
>
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 <[email protected]>
I liked this v2 better.
Reviewed-by: Breno Leitao <[email protected]>
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 <[email protected]>
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
Reviewed-by: Wojciech Drewek <[email protected]>
> 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)
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 <[email protected]>
Reviewed-by: Shannon Nelson <[email protected]>
> ---
> 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
>
Hello:
This patch was applied to netdev/net.git (main)
by Paolo Abeni <[email protected]>:
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