powerpc/fsl_rio: fix a improper release in fsl_rio_setup()

[PATCH] powerpc/fsl_rio: fix a improper release in fsl_rio_setup()

Posted by Haoxiang Li 1 month, 3 weeks ago

If rio_register_mport() fails, put_device() is the correct way
to drop the device reference.
To prevent a double free, remove put_device() in rio_register_mport()
Also, add a put_device() in tsi721_setup_mport() to prevent reference
leak.

Found by code review.

Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
---
 arch/powerpc/sysdev/fsl_rio.c    | 2 +-
 drivers/rapidio/devices/tsi721.c | 1 +
 drivers/rapidio/rio.c            | 1 -
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_rio.c b/arch/powerpc/sysdev/fsl_rio.c
index f9b214b299e7..f6226b2619ed 100644
--- a/arch/powerpc/sysdev/fsl_rio.c
+++ b/arch/powerpc/sysdev/fsl_rio.c
@@ -697,7 +697,7 @@ static int fsl_rio_setup(struct platform_device *dev)
 		if (rio_register_mport(port)) {
 			release_resource(&port->iores);
 			kfree(priv);
-			kfree(port);
+			put_device(&port->dev);
 			continue;
 		}
 		active_ports++;
diff --git a/drivers/rapidio/devices/tsi721.c b/drivers/rapidio/devices/tsi721.c
index 4b84270a8906..17c2bcf29c5c 100644
--- a/drivers/rapidio/devices/tsi721.c
+++ b/drivers/rapidio/devices/tsi721.c
@@ -2758,6 +2758,7 @@ static int tsi721_setup_mport(struct tsi721_device *priv)
 	err = rio_register_mport(mport);
 	if (err) {
 		tsi721_unregister_dma(priv);
+		put_device(&mport->dev);
 		goto err_exit;
 	}
 
diff --git a/drivers/rapidio/rio.c b/drivers/rapidio/rio.c
index 46daf32ea13b..026b2328afd7 100644
--- a/drivers/rapidio/rio.c
+++ b/drivers/rapidio/rio.c
@@ -2089,7 +2089,6 @@ int rio_register_mport(struct rio_mport *port)
 		mutex_lock(&rio_mport_list_lock);
 		list_del(&port->node);
 		mutex_unlock(&rio_mport_list_lock);
-		put_device(&port->dev);
 	} else {
 		dev_dbg(&port->dev, "RIO: registered mport%d\n", port->id);
 	}
-- 
2.25.1

Re: [PATCH] powerpc/fsl_rio: fix a improper release in fsl_rio_setup()

Posted by Christophe Leroy (CS GROUP) 1 month, 3 weeks ago


Le 19/12/2025 à 15:10, Haoxiang Li a écrit :
> [Vous ne recevez pas souvent de courriers de lihaoxiang@isrc.iscas.ac.cn. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> If rio_register_mport() fails, put_device() is the correct way
> to drop the device reference.
> To prevent a double free, remove put_device() in rio_register_mport()
> Also, add a put_device() in tsi721_setup_mport() to prevent reference
> leak.

Your explanation is unclear. It is correct that put_device() is the 
correct way to drop the device reference, and it is already what 
rio_register_mport() does. Why do you need to move it out of 
rio_register_mport() ? This is what you have to explain.

> 
> Found by code review.

AI code review or human code review ?

> 
> Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
> ---
>   arch/powerpc/sysdev/fsl_rio.c    | 2 +-
>   drivers/rapidio/devices/tsi721.c | 1 +
>   drivers/rapidio/rio.c            | 1 -
>   3 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/fsl_rio.c b/arch/powerpc/sysdev/fsl_rio.c
> index f9b214b299e7..f6226b2619ed 100644
> --- a/arch/powerpc/sysdev/fsl_rio.c
> +++ b/arch/powerpc/sysdev/fsl_rio.c
> @@ -697,7 +697,7 @@ static int fsl_rio_setup(struct platform_device *dev)
>                  if (rio_register_mport(port)) {
>                          release_resource(&port->iores);
>                          kfree(priv);
> -                       kfree(port);

Why do you remove this kfree() ? By doing this you will leak the port 
pointer allocated earlier in the loop by kzalloc()

> +                       put_device(&port->dev);
>                          continue;
>                  }
>                  active_ports++;
> diff --git a/drivers/rapidio/devices/tsi721.c b/drivers/rapidio/devices/tsi721.c
> index 4b84270a8906..17c2bcf29c5c 100644
> --- a/drivers/rapidio/devices/tsi721.c
> +++ b/drivers/rapidio/devices/tsi721.c
> @@ -2758,6 +2758,7 @@ static int tsi721_setup_mport(struct tsi721_device *priv)
>          err = rio_register_mport(mport);
>          if (err) {
>                  tsi721_unregister_dma(priv);
> +               put_device(&mport->dev);
>                  goto err_exit;
>          }
> 
> diff --git a/drivers/rapidio/rio.c b/drivers/rapidio/rio.c
> index 46daf32ea13b..026b2328afd7 100644
> --- a/drivers/rapidio/rio.c
> +++ b/drivers/rapidio/rio.c
> @@ -2089,7 +2089,6 @@ int rio_register_mport(struct rio_mport *port)
>                  mutex_lock(&rio_mport_list_lock);
>                  list_del(&port->node);
>                  mutex_unlock(&rio_mport_list_lock);
> -               put_device(&port->dev);
>          } else {
>                  dev_dbg(&port->dev, "RIO: registered mport%d\n", port->id);
>          }
> --
> 2.25.1
>

Re: [PATCH] powerpc/fsl_rio: fix a improper release in fsl_rio_setup()

Posted by Haoxiang Li 1 month, 3 weeks ago

On Sat, 20 Dec 2025 11:45:10 +0100, Christophe Leroy wrote:
> Your explanation is unclear. It is correct that put_device() is the 
> correct way to drop the device reference, and it is already what 
> rio_register_mport() does. Why do you need to move it out of 
> rio_register_mport() ? This is what you have to explain.

Sorry for the unclear changelog. I think if device_register() in
rio_register_mport() fails, put_device() is already called. Then,
the error path in fsl_rio_setup() also kfree() the port. I suspected
a potential double-free issue here, although I am unsure whether this interpretation is correct.

> AI code review or human code review ?

Human review.

> Why do you remove this kfree() ? By doing this you will leak the port 
> pointer allocated earlier in the loop by kzalloc()

I just thought after device_register(), the cleanup should be done by
put_device().

I would appreciate your clarification.

Thanks,
Haoxiang Li

Re: [PATCH] powerpc/fsl_rio: fix a improper release in fsl_rio_setup()

Posted by Christophe Leroy (CS GROUP) 1 month, 2 weeks ago


Le 20/12/2025 à 14:42, Haoxiang Li a écrit :
> [Vous ne recevez pas souvent de courriers de lihaoxiang@isrc.iscas.ac.cn. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Sat, 20 Dec 2025 11:45:10 +0100, Christophe Leroy wrote:
>> Your explanation is unclear. It is correct that put_device() is the
>> correct way to drop the device reference, and it is already what
>> rio_register_mport() does. Why do you need to move it out of
>> rio_register_mport() ? This is what you have to explain.
> 
> Sorry for the unclear changelog. I think if device_register() in
> rio_register_mport() fails, put_device() is already called. Then,
> the error path in fsl_rio_setup() also kfree() the port. I suspected
> a potential double-free issue here, although I am unsure whether this interpretation is correct.

The put_device() in rio_register_mport() releases the reference to 
port->dev taken by device_register(). See the documentation in 
https://elixir.bootlin.com/linux/v6.18/source/drivers/base/core.c#L3750

The kfree(port) in fsl_rio_setup() is to free the memory allocated by 
the kzalloc() at 
https://elixir.bootlin.com/linux/v6.18/source/arch/powerpc/sysdev/fsl_rio.c#L583

Those are two independant things.

> 
>> AI code review or human code review ?
> 
> Human review.
> 
>> Why do you remove this kfree() ? By doing this you will leak the port
>> pointer allocated earlier in the loop by kzalloc()
> 
> I just thought after device_register(), the cleanup should be done by
> put_device().

You are talking about the cleanup of port->dev. Once you have cleaned up 
port->dev you also have to cleanup port.

> 
> I would appreciate your clarification.

See above.

I might be wrong but from my point of view the existing code is correct.

Christophe