Messages in this thread Patch in this message | | | Date | Sun, 3 Sep 2023 22:43:59 +0200 | From | Dominik Brodowski <> | Subject | Re: [PATCH 1/2] pcmcia: ds: fix refcount leak in pcmcia_device_add() |
| |
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
| |