[PATCH 1/2] pcmcia: ds: fix refcount leak in pcmcia_device_add()

[PATCH 1/2] pcmcia: ds: fix refcount leak in pcmcia_device_add()

[Yang Yingliang]

If device_register() returns error, the refcount of function_config
need be put.

Fixes: 360b65b95bae ("[PATCH] pcmcia: make config_t independent, add reference counting")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/pcmcia/ds.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
index ace133b9f7d4..7d3258a1f8f8 100644
--- a/drivers/pcmcia/ds.c
+++ b/drivers/pcmcia/ds.c
@@ -574,10 +574,12 @@ static struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s,
 	pcmcia_device_query(p_dev);
 
 	if (device_register(&p_dev->dev))
-		goto err_unreg;
+		goto err_put_ref;
 
 	return p_dev;
 
+ err_put_ref:
+	kref_put(&p_dev->function_config->ref, pcmcia_release_function);
  err_unreg:
 	mutex_lock(&s->ops_mutex);
 	list_del(&p_dev->socket_device_list);
-- 
2.25.1

[PATCH 2/2] pcmcia: ds: fix possible name leak in error path in pcmcia_device_add()

[Yang Yingliang]

Afer commit 1fa5ae857bb1 ("driver core: get rid of struct device's
bus_id string array"), the name of device is allocated dynamically,
it need bee freed.

As comment of device_register() says, it should use put_device() to
give up the reference in the error path, some resources is going to
freed in pcmcia_release_dev(), so don't use error label, just return
NULL after calling put_device().

Before device_initialize(), it can not call put_device(), so move the
dev_set_name() before device_register().

Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/pcmcia/ds.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
index 7d3258a1f8f8..a249d9a0457b 100644
--- a/drivers/pcmcia/ds.c
+++ b/drivers/pcmcia/ds.c
@@ -513,9 +513,6 @@ static struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s,
 	/* by default don't allow DMA */
 	p_dev->dma_mask = 0;
 	p_dev->dev.dma_mask = &p_dev->dma_mask;
-	dev_set_name(&p_dev->dev, "%d.%d", p_dev->socket->sock, p_dev->device_no);
-	if (!dev_name(&p_dev->dev))
-		goto err_free;
 	p_dev->devname = kasprintf(GFP_KERNEL, "pcmcia%s", dev_name(&p_dev->dev));
 	if (!p_dev->devname)
 		goto err_free;
@@ -573,8 +570,17 @@ static struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s,
 
 	pcmcia_device_query(p_dev);
 
-	if (device_register(&p_dev->dev))
+	dev_set_name(&p_dev->dev, "%d.%d", p_dev->socket->sock, p_dev->device_no);
+	if (!dev_name(&p_dev->dev))
 		goto err_put_ref;
+	if (device_register(&p_dev->dev)) {
+		mutex_lock(&s->ops_mutex);
+		list_del(&p_dev->socket_device_list);
+		s->device_count--;
+		mutex_unlock(&s->ops_mutex);
+		put_device(&p_dev->dev);
+		return NULL;
+	}
 
 	return p_dev;
 
-- 
2.25.1

Re: [PATCH 2/2] pcmcia: ds: fix possible name leak in error path in pcmcia_device_add()

[Dominik Brodowski]

Am Sat, Nov 12, 2022 at 05:29:24PM +0800 schrieb Yang Yingliang:
> Afer commit 1fa5ae857bb1 ("driver core: get rid of struct device's
> bus_id string array"), the name of device is allocated dynamically,
> it need bee freed.
> 
> As comment of device_register() says, it should use put_device() to
> give up the reference in the error path, some resources is going to
> freed in pcmcia_release_dev(), so don't use error label, just return
> NULL after calling put_device().
> 
> Before device_initialize(), it can not call put_device(), so move the
> dev_set_name() before device_register().
> 
> Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  drivers/pcmcia/ds.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
> index 7d3258a1f8f8..a249d9a0457b 100644
> --- a/drivers/pcmcia/ds.c
> +++ b/drivers/pcmcia/ds.c
> @@ -513,9 +513,6 @@ static struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s,
>  	/* by default don't allow DMA */
>  	p_dev->dma_mask = 0;
>  	p_dev->dev.dma_mask = &p_dev->dma_mask;
> -	dev_set_name(&p_dev->dev, "%d.%d", p_dev->socket->sock, p_dev->device_no);
> -	if (!dev_name(&p_dev->dev))
> -		goto err_free;
>  	p_dev->devname = kasprintf(GFP_KERNEL, "pcmcia%s", dev_name(&p_dev->dev));
>  	if (!p_dev->devname)
>  		goto err_free;
> @@ -573,8 +570,17 @@ static struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s,
>  
>  	pcmcia_device_query(p_dev);
>  
> -	if (device_register(&p_dev->dev))
> +	dev_set_name(&p_dev->dev, "%d.%d", p_dev->socket->sock, p_dev->device_no);
> +	if (!dev_name(&p_dev->dev))
>  		goto err_put_ref;
> +	if (device_register(&p_dev->dev)) {

Thanks for your patch! I totally see your point. However, err_put_ref puts
the "wrong" reference (to &p_dev->function_config->ref), which doesn't help
with the issue you detected, as that requires &p_dev->dev to be dropped.
What about the following instead?

From: Yang Yingliang <yangyingliang@huawei.com>
Subject: [PATCH] pcmcia: ds: fix possible name leak in error path in
 pcmcia_device_add()

Afer commit 1fa5ae857bb1 ("driver core: get rid of struct device's
bus_id string array"), the name of device is allocated dynamically.
Therefore, it needs to be freed, which is done by the driver core for
us once all references to the device are gone. Therefore, move the
dev_set_name() call immediately before the call device_register(), which
either succeeds (then the freeing will be done upon subsequent remvoal),
or puts the reference in the error call. Also, it is not unusual that the
return value of dev_set_name is not checked.

Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
[linux@dominikbrodowski.net: simplification, commit message modified]
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>

diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
index c90c68dee1e4..b4b8363d1de2 100644
--- a/drivers/pcmcia/ds.c
+++ b/drivers/pcmcia/ds.c
@@ -513,9 +513,6 @@ static struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s,
 	/* by default don't allow DMA */
 	p_dev->dma_mask = 0;
 	p_dev->dev.dma_mask = &p_dev->dma_mask;
-	dev_set_name(&p_dev->dev, "%d.%d", p_dev->socket->sock, p_dev->device_no);
-	if (!dev_name(&p_dev->dev))
-		goto err_free;
 	p_dev->devname = kasprintf(GFP_KERNEL, "pcmcia%s", dev_name(&p_dev->dev));
 	if (!p_dev->devname)
 		goto err_free;
@@ -573,6 +570,7 @@ static struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s,
 
 	pcmcia_device_query(p_dev);
 
+	dev_set_name(&p_dev->dev, "%d.%d", p_dev->socket->sock, p_dev->device_no);
 	if (device_register(&p_dev->dev)) {
 		mutex_lock(&s->ops_mutex);
 		list_del(&p_dev->socket_device_list);

Re: [PATCH 1/2] pcmcia: ds: fix refcount leak in pcmcia_device_add()

[Dominik Brodowski]

Hi,

Am Sat, Nov 12, 2022 at 05:29:23PM +0800 schrieb Yang Yingliang:
> If device_register() returns error, the refcount of function_config
> need be put.
> 
> Fixes: 360b65b95bae ("[PATCH] pcmcia: make config_t independent, add reference counting")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  drivers/pcmcia/ds.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
> index ace133b9f7d4..7d3258a1f8f8 100644
> --- a/drivers/pcmcia/ds.c
> +++ b/drivers/pcmcia/ds.c
> @@ -574,10 +574,12 @@ static struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s,
>  	pcmcia_device_query(p_dev);
>  
>  	if (device_register(&p_dev->dev))
> -		goto err_unreg;
> +		goto err_put_ref;
>  
>  	return p_dev;
>  
> + err_put_ref:
> +	kref_put(&p_dev->function_config->ref, pcmcia_release_function);
>   err_unreg:
>  	mutex_lock(&s->ops_mutex);
>  	list_del(&p_dev->socket_device_list);
> -- 
> 2.25.1
> 

Indeed, there's a reference leak here in this failure path. However, you
rightly note in your subsequent patch:

Am Sat, Nov 12, 2022 at 05:29:24PM +0800 schrieb Yang Yingliang:
> As comment of device_register() says, it should use put_device() to
> give up the reference in the error path, some resources is going to
> freed in pcmcia_release_dev(), so don't use error label, just return
> NULL after calling put_device().

Therefore, I'd suggest to combine both things:


From: Yang Yingliang <yangyingliang@huawei.com>
Subject: [PATCH] pcmcia: ds: fix refcount leak in pcmcia_device_add()

As the comment of device_register() says, it should use put_device()
to give up the reference in the error path. Then, insofar resources
will be freed in pcmcia_release_dev(), the error path is no longer
needed. In particular, this means that the (previously missing) dropping
of the reference to &p_dev->function_config->ref is now handled by
pcmcia_release_dev().

Fixes: 360b65b95bae ("[PATCH] pcmcia: make config_t independent, add reference counting")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
[linux@dominikbrodowski.net: simplification, commit message rewrite]
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>

diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
index d500e5dbbc3f..c90c68dee1e4 100644
--- a/drivers/pcmcia/ds.c
+++ b/drivers/pcmcia/ds.c
@@ -573,8 +573,14 @@ static struct pcmcia_device *pcmcia_device_add(struct pcmcia_socket *s,
 
 	pcmcia_device_query(p_dev);
 
-	if (device_register(&p_dev->dev))
-		goto err_unreg;
+	if (device_register(&p_dev->dev)) {
+		mutex_lock(&s->ops_mutex);
+		list_del(&p_dev->socket_device_list);
+		s->device_count--;
+		mutex_unlock(&s->ops_mutex);
+		put_device(&p_dev->dev);
+		return NULL;
+	}
 
 	return p_dev;
 



What do you think?

Best,
	Dominik