[v1] usb: host: sl811_cs: fix memory leak on probe failure

[PATCH] usb: host: sl811_cs: fix memory leak on probe failure

Posted by Zongmin Zhou 1 month ago

From: Zongmin Zhou <zhouzongmin@kylinos.cn>

sl811_cs_probe() leaks the local_info_t allocated into link->priv when
sl811_cs_config() fails: the only kfree() lives in the remove callback
sl811_cs_detach(), which the PCMCIA core never calls for a device whose
probe failed.

Free the private data when sl811_cs_config() fails.

Fixes: c6de2b64eb57 ("[PATCH] USB: add sl811_cs support")
Signed-off-by: Zongmin Zhou <zhouzongmin@kylinos.cn>
---
 drivers/usb/host/sl811_cs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/sl811_cs.c b/drivers/usb/host/sl811_cs.c
index ada91ca33f65..fd0cd541bf53 100644
--- a/drivers/usb/host/sl811_cs.c
+++ b/drivers/usb/host/sl811_cs.c
@@ -177,6 +177,7 @@ failed:
 static int sl811_cs_probe(struct pcmcia_device *link)
 {
 	local_info_t *local;
+	int ret;
 
 	local = kzalloc_obj(local_info_t);
 	if (!local)
@@ -184,7 +185,11 @@ static int sl811_cs_probe(struct pcmcia_device *link)
 	local->p_dev = link;
 	link->priv = local;
 
-	return sl811_cs_config(link);
+	ret = sl811_cs_config(link);
+	if (ret)
+		kfree(local);
+
+	return ret;
 }
 
 static const struct pcmcia_device_id sl811_ids[] = {
-- 
2.34.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH] usb: host: sl811_cs: fix memory leak on probe failure

Posted by Nikolay Kulikov 1 month ago

On Thu, Aug 27, 2026 at 03:41:20PM +0800, Zongmin Zhou wrote:
> From: Zongmin Zhou <zhouzongmin@kylinos.cn>
> 
> sl811_cs_probe() leaks the local_info_t allocated into link->priv when
> sl811_cs_config() fails: the only kfree() lives in the remove callback
> sl811_cs_detach(), which the PCMCIA core never calls for a device whose
> probe failed.
> 
> Free the private data when sl811_cs_config() fails.
> 
> Fixes: c6de2b64eb57 ("[PATCH] USB: add sl811_cs support")

The code looks good to me, but I have a question regarding the Fixes
tag.

In the commit you cited, a failure triggered a call to
sl811_cs_detach(), which freed that memory. However, that behavior was
changed in commit

f8cfa618dccb ("[PATCH] pcmcia: unify attach, EVENT_CARD_INSERTION handlers into one probe callback")

where the call was removed, leaving the memory unfreed. Shouldn't that
be the commit referenced in the Fixes?

Thanks,
Nikolay

Re: [PATCH] usb: host: sl811_cs: fix memory leak on probe failure

Posted by Zongmin Zhou 3 weeks, 6 days ago

在 2026/8/29 01:36, Nikolay Kulikov 写道:
> On Thu, Aug 27, 2026 at 03:41:20PM +0800, Zongmin Zhou wrote:
>> From: Zongmin Zhou <zhouzongmin@kylinos.cn>
>>
>> sl811_cs_probe() leaks the local_info_t allocated into link->priv when
>> sl811_cs_config() fails: the only kfree() lives in the remove callback
>> sl811_cs_detach(), which the PCMCIA core never calls for a device whose
>> probe failed.
>>
>> Free the private data when sl811_cs_config() fails.
>>
>> Fixes: c6de2b64eb57 ("[PATCH] USB: add sl811_cs support")
> The code looks good to me, but I have a question regarding the Fixes
> tag.
>
> In the commit you cited, a failure triggered a call to
> sl811_cs_detach(), which freed that memory. However, that behavior was
> changed in commit
>
> f8cfa618dccb ("[PATCH] pcmcia: unify attach, EVENT_CARD_INSERTION handlers into one probe callback")
>
> where the call was removed, leaving the memory unfreed. Shouldn't that
> be the commit referenced in the Fixes?
Hi Nikolay,

You're right that c6de2b64eb57 did not introduce the leak -- but neither 
did f8cfa618dccb.
The leak was introduced by 15b99ac17295 ("[PATCH] pcmcia: add return 
value to _config() functions")

f8cfa618dccb dropped the detach-on-failure path because registration
moved into the core, and its probe ended with
     sl811_cs_config(link);
     return 0;
A config failure was never propagated, so probe() always succeeded
after the allocation and it remained paired with the kfree() in
remove(). No leak either.

15b99ac17295 made sl811_cs_config() return -ENODEV and the probe return
that value -- the first time probe could fail after the allocation.
Since the core never calls ->remove() for a failed probe, link->priv
leaked from then on.

If you agree, I'll send a v2 with
     Fixes: 15b99ac17295 ("[PATCH] pcmcia: add return value to _config() 
functions")

Thanks,
Zongmin
>
>
> Thanks,
> Nikolay

Re: [PATCH] usb: host: sl811_cs: fix memory leak on probe failure

Posted by Nikolay Kulikov 3 weeks, 6 days ago

On Mon, Aug 31, 2026 at 02:11:16PM +0800, Zongmin Zhou wrote:
> 
> 在 2026/8/29 01:36, Nikolay Kulikov 写道:
> > On Thu, Aug 27, 2026 at 03:41:20PM +0800, Zongmin Zhou wrote:
> > > From: Zongmin Zhou <zhouzongmin@kylinos.cn>
> > > 
> > > sl811_cs_probe() leaks the local_info_t allocated into link->priv when
> > > sl811_cs_config() fails: the only kfree() lives in the remove callback
> > > sl811_cs_detach(), which the PCMCIA core never calls for a device whose
> > > probe failed.
> > > 
> > > Free the private data when sl811_cs_config() fails.
> > > 
> > > Fixes: c6de2b64eb57 ("[PATCH] USB: add sl811_cs support")
> > The code looks good to me, but I have a question regarding the Fixes
> > tag.
> > 
> > In the commit you cited, a failure triggered a call to
> > sl811_cs_detach(), which freed that memory. However, that behavior was
> > changed in commit
> > 
> > f8cfa618dccb ("[PATCH] pcmcia: unify attach, EVENT_CARD_INSERTION handlers into one probe callback")
> > 
> > where the call was removed, leaving the memory unfreed. Shouldn't that
> > be the commit referenced in the Fixes?
> Hi Nikolay,
> 
> You're right that c6de2b64eb57 did not introduce the leak -- but neither did
> f8cfa618dccb.
> The leak was introduced by 15b99ac17295 ("[PATCH] pcmcia: add return value
> to _config() functions")
> 
> f8cfa618dccb dropped the detach-on-failure path because registration
> moved into the core, and its probe ended with
>     sl811_cs_config(link);
>     return 0;
> A config failure was never propagated, so probe() always succeeded
> after the allocation and it remained paired with the kfree() in
> remove(). No leak either.
> 
> 15b99ac17295 made sl811_cs_config() return -ENODEV and the probe return
> that value -- the first time probe could fail after the allocation.
> Since the core never calls ->remove() for a failed probe, link->priv
> leaked from then on.
> 
> If you agree, I'll send a v2 with
>     Fixes: 15b99ac17295 ("[PATCH] pcmcia: add return value to _config()
> functions")

Yes, it really should be 15b99ac17295.
In v2, you can add my

Reviewed-by: Nikolay Kulikov <nikolayof23@gmail.com>

> 
> Thanks,
> Zongmin
> > 
> > 
> > Thanks,
> > Nikolay
> 
>

[PATCH v2] usb: host: sl811_cs: fix memory leak on probe failure

Posted by Zongmin Zhou 3 weeks, 6 days ago

From: Zongmin Zhou <zhouzongmin@kylinos.cn>

sl811_cs_probe() leaks the local_info_t allocated into link->priv when
sl811_cs_config() fails: the only kfree() lives in the remove callback
sl811_cs_detach(), which the PCMCIA core never calls for a device whose
probe failed.

Free the private data when sl811_cs_config() fails.

Fixes: 15b99ac17295 ("[PATCH] pcmcia: add return value to _config() functions")
Reviewed-by: Nikolay Kulikov <nikolayof23@gmail.com>
Signed-off-by: Zongmin Zhou <zhouzongmin@kylinos.cn>
---
Changes in v2:
- Fix the Fixes tag to 15b99ac17295, the commit that made
  sl811_cs_config() failures observable as probe failures.
- Add Reviewed-by from Nikolay Kulikov.

 drivers/usb/host/sl811_cs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/sl811_cs.c b/drivers/usb/host/sl811_cs.c
index ada91ca33f65..fd0cd541bf53 100644
--- a/drivers/usb/host/sl811_cs.c
+++ b/drivers/usb/host/sl811_cs.c
@@ -177,6 +177,7 @@ failed:
 static int sl811_cs_probe(struct pcmcia_device *link)
 {
 	local_info_t *local;
+	int ret;

 	local = kzalloc_obj(local_info_t);
 	if (!local)
@@ -184,7 +185,11 @@ static int sl811_cs_probe(struct pcmcia_device *link)
 	local->p_dev = link;
 	link->priv = local;

-	return sl811_cs_config(link);
+	ret = sl811_cs_config(link);
+	if (ret)
+		kfree(local);
+
+	return ret;
 }

 static const struct pcmcia_device_id sl811_ids[] = {
--
2.34.1

No virus found
		Checked by Hillstone Network AntiVirus