* [PATCH] pata_parport: fix possible memory leak [not found] <3ab89ddc-cb60-47c4-86ad-cdad94a8a3d7@kili.mountain> @ 2023-03-11 18:51 ` Ondrej Zary 2023-03-11 20:19 ` Sergei Shtylyov 0 siblings, 1 reply; 12+ messages in thread From: Ondrej Zary @ 2023-03-11 18:51 UTC (permalink / raw) To: Damien Le Moal Cc: Dan Carpenter, Christoph Hellwig, Sergey Shtylyov, linux-ide, linux-kernel When ida_alloc() fails, "pi" is not freed although the misleading comment says otherwise. Move the ida_alloc() call up so we really don't have to free it. Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <error27@gmail.com> Link: https://lore.kernel.org/r/202303111822.IHNchbkp-lkp@intel.com/ Signed-off-by: Ondrej Zary <linux@zary.sk> --- drivers/ata/pata_parport/pata_parport.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c index 6165ee9aa7da..fb1f10afa722 100644 --- a/drivers/ata/pata_parport/pata_parport.c +++ b/drivers/ata/pata_parport/pata_parport.c @@ -503,18 +503,19 @@ static struct pi_adapter *pi_init_one(struct parport *parport, if (bus_for_each_dev(&pata_parport_bus_type, NULL, &match, pi_find_dev)) return NULL; + id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); + if (id < 0) + return NULL; + pi = kzalloc(sizeof(struct pi_adapter), GFP_KERNEL); if (!pi) - return NULL; + goto out_ida_free; /* set up pi->dev before pi_probe_unit() so it can use dev_printk() */ pi->dev.parent = &pata_parport_bus; pi->dev.bus = &pata_parport_bus_type; pi->dev.driver = &pr->driver; pi->dev.release = pata_parport_dev_release; - id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); - if (id < 0) - return NULL; /* pata_parport_dev_release will do kfree(pi) */ pi->dev.id = id; dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id); if (device_register(&pi->dev)) { -- Ondrej Zary ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] pata_parport: fix possible memory leak 2023-03-11 18:51 ` [PATCH] pata_parport: fix possible memory leak Ondrej Zary @ 2023-03-11 20:19 ` Sergei Shtylyov 2023-03-11 20:23 ` Sergey Shtylyov 0 siblings, 1 reply; 12+ messages in thread From: Sergei Shtylyov @ 2023-03-11 20:19 UTC (permalink / raw) To: Ondrej Zary, Damien Le Moal Cc: Dan Carpenter, Christoph Hellwig, Sergey Shtylyov, linux-ide, linux-kernel On 3/11/23 9:51 PM, Ondrej Zary wrote: > When ida_alloc() fails, "pi" is not freed although the misleading > comment says otherwise. > Move the ida_alloc() call up so we really don't have to free it. > > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <error27@gmail.com> > Link: https://lore.kernel.org/r/202303111822.IHNchbkp-lkp@intel.com/ > Signed-off-by: Ondrej Zary <linux@zary.sk> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pata_parport: fix possible memory leak 2023-03-11 20:19 ` Sergei Shtylyov @ 2023-03-11 20:23 ` Sergey Shtylyov 2023-03-11 21:11 ` Ondrej Zary 0 siblings, 1 reply; 12+ messages in thread From: Sergey Shtylyov @ 2023-03-11 20:23 UTC (permalink / raw) To: Sergei Shtylyov, Ondrej Zary, Damien Le Moal Cc: Dan Carpenter, Christoph Hellwig, linux-ide, linux-kernel On 3/11/23 11:19 PM, Sergei Shtylyov wrote: >> When ida_alloc() fails, "pi" is not freed although the misleading >> comment says otherwise. >> Move the ida_alloc() call up so we really don't have to free it. Wait, but don't we still need to call kfree() in pi_init_one()? >> Reported-by: kernel test robot <lkp@intel.com> >> Reported-by: Dan Carpenter <error27@gmail.com> >> Link: https://lore.kernel.org/r/202303111822.IHNchbkp-lkp@intel.com/ >> Signed-off-by: Ondrej Zary <linux@zary.sk> > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > [...] MBR, Sergey ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pata_parport: fix possible memory leak 2023-03-11 20:23 ` Sergey Shtylyov @ 2023-03-11 21:11 ` Ondrej Zary 2023-03-11 21:39 ` Ondrej Zary 0 siblings, 1 reply; 12+ messages in thread From: Ondrej Zary @ 2023-03-11 21:11 UTC (permalink / raw) To: Sergey Shtylyov Cc: Sergei Shtylyov, Damien Le Moal, Dan Carpenter, Christoph Hellwig, linux-ide, linux-kernel On Saturday 11 March 2023 21:23:25 Sergey Shtylyov wrote: > On 3/11/23 11:19 PM, Sergei Shtylyov wrote: > > >> When ida_alloc() fails, "pi" is not freed although the misleading > >> comment says otherwise. > >> Move the ida_alloc() call up so we really don't have to free it. > > Wait, but don't we still need to call kfree() in pi_init_one()? If it fails at device_register, the dev.release is already set to pata_parport_dev_release which does the kfree(). put_device() should call it. If it fails later, device_unregister() should do it. > >> Reported-by: kernel test robot <lkp@intel.com> > >> Reported-by: Dan Carpenter <error27@gmail.com> > >> Link: https://lore.kernel.org/r/202303111822.IHNchbkp-lkp@intel.com/ > >> Signed-off-by: Ondrej Zary <linux@zary.sk> > > > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > > > [...] > > MBR, Sergey > -- Ondrej Zary ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] pata_parport: fix possible memory leak 2023-03-11 21:11 ` Ondrej Zary @ 2023-03-11 21:39 ` Ondrej Zary 2023-03-11 21:44 ` [PATCH v2] " Ondrej Zary 0 siblings, 1 reply; 12+ messages in thread From: Ondrej Zary @ 2023-03-11 21:39 UTC (permalink / raw) To: Sergey Shtylyov Cc: Sergei Shtylyov, Damien Le Moal, Dan Carpenter, Christoph Hellwig, linux-ide, linux-kernel On Saturday 11 March 2023 22:11:57 Ondrej Zary wrote: > On Saturday 11 March 2023 21:23:25 Sergey Shtylyov wrote: > > On 3/11/23 11:19 PM, Sergei Shtylyov wrote: > > > > >> When ida_alloc() fails, "pi" is not freed although the misleading > > >> comment says otherwise. > > >> Move the ida_alloc() call up so we really don't have to free it. > > > > Wait, but don't we still need to call kfree() in pi_init_one()? > > If it fails at device_register, the dev.release is already set to > pata_parport_dev_release which does the kfree(). put_device() should call > it. If it fails later, device_unregister() should do it. But I see that the ida_free() at the end of pi_init_one() is wrong. It uses pi->dev.id but pi is either uninitialized or already freed. -- Ondrej Zary ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] pata_parport: fix possible memory leak 2023-03-11 21:39 ` Ondrej Zary @ 2023-03-11 21:44 ` Ondrej Zary 2023-03-12 0:56 ` Damien Le Moal 0 siblings, 1 reply; 12+ messages in thread From: Ondrej Zary @ 2023-03-11 21:44 UTC (permalink / raw) To: Damien Le Moal Cc: Christoph Hellwig, Sergey Shtylyov, linux-ide, linux-kernel When ida_alloc() fails, "pi" is not freed although the misleading comment says otherwise. Move the ida_alloc() call up so we really don't have to free it. Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <error27@gmail.com> Link: https://lore.kernel.org/r/202303111822.IHNchbkp-lkp@intel.com/ Signed-off-by: Ondrej Zary <linux@zary.sk> --- drivers/ata/pata_parport/pata_parport.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c index 6165ee9aa7da..a9eff6003098 100644 --- a/drivers/ata/pata_parport/pata_parport.c +++ b/drivers/ata/pata_parport/pata_parport.c @@ -503,18 +503,19 @@ static struct pi_adapter *pi_init_one(struct parport *parport, if (bus_for_each_dev(&pata_parport_bus_type, NULL, &match, pi_find_dev)) return NULL; + id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); + if (id < 0) + return NULL; + pi = kzalloc(sizeof(struct pi_adapter), GFP_KERNEL); if (!pi) - return NULL; + goto out_ida_free; /* set up pi->dev before pi_probe_unit() so it can use dev_printk() */ pi->dev.parent = &pata_parport_bus; pi->dev.bus = &pata_parport_bus_type; pi->dev.driver = &pr->driver; pi->dev.release = pata_parport_dev_release; - id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); - if (id < 0) - return NULL; /* pata_parport_dev_release will do kfree(pi) */ pi->dev.id = id; dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id); if (device_register(&pi->dev)) { @@ -571,7 +572,7 @@ static struct pi_adapter *pi_init_one(struct parport *parport, out_unreg_dev: device_unregister(&pi->dev); out_ida_free: - ida_free(&pata_parport_bus_dev_ids, pi->dev.id); + ida_free(&pata_parport_bus_dev_ids, id); return NULL; } -- Ondrej Zary ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pata_parport: fix possible memory leak 2023-03-11 21:44 ` [PATCH v2] " Ondrej Zary @ 2023-03-12 0:56 ` Damien Le Moal 2023-03-12 21:24 ` Ondrej Zary 0 siblings, 1 reply; 12+ messages in thread From: Damien Le Moal @ 2023-03-12 0:56 UTC (permalink / raw) To: Ondrej Zary; +Cc: Christoph Hellwig, Sergey Shtylyov, linux-ide, linux-kernel On 3/12/23 06:44, Ondrej Zary wrote: > When ida_alloc() fails, "pi" is not freed although the misleading > comment says otherwise. > Move the ida_alloc() call up so we really don't have to free it. Certainly you meant: "so we really do free it in case of error.", no ? > > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <error27@gmail.com> > Link: https://lore.kernel.org/r/202303111822.IHNchbkp-lkp@intel.com/ > Signed-off-by: Ondrej Zary <linux@zary.sk> > --- > drivers/ata/pata_parport/pata_parport.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c > index 6165ee9aa7da..a9eff6003098 100644 > --- a/drivers/ata/pata_parport/pata_parport.c > +++ b/drivers/ata/pata_parport/pata_parport.c > @@ -503,18 +503,19 @@ static struct pi_adapter *pi_init_one(struct parport *parport, > if (bus_for_each_dev(&pata_parport_bus_type, NULL, &match, pi_find_dev)) > return NULL; > > + id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); > + if (id < 0) > + return NULL; > + > pi = kzalloc(sizeof(struct pi_adapter), GFP_KERNEL); > if (!pi) > - return NULL; > + goto out_ida_free; > > /* set up pi->dev before pi_probe_unit() so it can use dev_printk() */ > pi->dev.parent = &pata_parport_bus; > pi->dev.bus = &pata_parport_bus_type; > pi->dev.driver = &pr->driver; > pi->dev.release = pata_parport_dev_release; > - id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); > - if (id < 0) > - return NULL; /* pata_parport_dev_release will do kfree(pi) */ > pi->dev.id = id; > dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id); > if (device_register(&pi->dev)) { > @@ -571,7 +572,7 @@ static struct pi_adapter *pi_init_one(struct parport *parport, > out_unreg_dev: > device_unregister(&pi->dev); Same comment as Sergey: isn't this going to do the ida free ? So shouldn't you return here ? > out_ida_free: > - ida_free(&pata_parport_bus_dev_ids, pi->dev.id); > + ida_free(&pata_parport_bus_dev_ids, id); > return NULL; > } > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pata_parport: fix possible memory leak 2023-03-12 0:56 ` Damien Le Moal @ 2023-03-12 21:24 ` Ondrej Zary 2023-03-12 23:17 ` Damien Le Moal 0 siblings, 1 reply; 12+ messages in thread From: Ondrej Zary @ 2023-03-12 21:24 UTC (permalink / raw) To: Damien Le Moal Cc: Christoph Hellwig, Sergey Shtylyov, linux-ide, linux-kernel On Sunday 12 March 2023 01:56:25 Damien Le Moal wrote: > On 3/12/23 06:44, Ondrej Zary wrote: > > When ida_alloc() fails, "pi" is not freed although the misleading > > comment says otherwise. > > Move the ida_alloc() call up so we really don't have to free it. > > Certainly you meant: "so we really do free it in case of error.", no ? I meant "so we don't have to free pi in case of ida_alloc failure". > > > > Reported-by: kernel test robot <lkp@intel.com> > > Reported-by: Dan Carpenter <error27@gmail.com> > > Link: https://lore.kernel.org/r/202303111822.IHNchbkp-lkp@intel.com/ > > Signed-off-by: Ondrej Zary <linux@zary.sk> > > --- > > drivers/ata/pata_parport/pata_parport.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c > > index 6165ee9aa7da..a9eff6003098 100644 > > --- a/drivers/ata/pata_parport/pata_parport.c > > +++ b/drivers/ata/pata_parport/pata_parport.c > > @@ -503,18 +503,19 @@ static struct pi_adapter *pi_init_one(struct parport *parport, > > if (bus_for_each_dev(&pata_parport_bus_type, NULL, &match, pi_find_dev)) > > return NULL; > > > > + id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); > > + if (id < 0) > > + return NULL; > > + > > pi = kzalloc(sizeof(struct pi_adapter), GFP_KERNEL); > > if (!pi) > > - return NULL; > > + goto out_ida_free; > > > > /* set up pi->dev before pi_probe_unit() so it can use dev_printk() */ > > pi->dev.parent = &pata_parport_bus; > > pi->dev.bus = &pata_parport_bus_type; > > pi->dev.driver = &pr->driver; > > pi->dev.release = pata_parport_dev_release; > > - id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); > > - if (id < 0) > > - return NULL; /* pata_parport_dev_release will do kfree(pi) */ > > pi->dev.id = id; > > dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id); > > if (device_register(&pi->dev)) { > > @@ -571,7 +572,7 @@ static struct pi_adapter *pi_init_one(struct parport *parport, > > out_unreg_dev: > > device_unregister(&pi->dev); > > Same comment as Sergey: isn't this going to do the ida free ? So shouldn't you > return here ? No. device_unregister() calls pata_parport_dev_release() which does only kfree(pi), not ida_free(). But it probably should do ida_free() too. > > > out_ida_free: > > - ida_free(&pata_parport_bus_dev_ids, pi->dev.id); > > + ida_free(&pata_parport_bus_dev_ids, id); > > return NULL; > > } > > > -- Ondrej Zary ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pata_parport: fix possible memory leak 2023-03-12 21:24 ` Ondrej Zary @ 2023-03-12 23:17 ` Damien Le Moal 2023-03-13 7:53 ` Ondrej Zary 2023-03-14 22:58 ` [PATCH v3] pata_parport: fix memory leaks Ondrej Zary 0 siblings, 2 replies; 12+ messages in thread From: Damien Le Moal @ 2023-03-12 23:17 UTC (permalink / raw) To: Ondrej Zary; +Cc: Christoph Hellwig, Sergey Shtylyov, linux-ide, linux-kernel On 3/13/23 06:24, Ondrej Zary wrote: > On Sunday 12 March 2023 01:56:25 Damien Le Moal wrote: >> On 3/12/23 06:44, Ondrej Zary wrote: >>> When ida_alloc() fails, "pi" is not freed although the misleading >>> comment says otherwise. >>> Move the ida_alloc() call up so we really don't have to free it. >> >> Certainly you meant: "so we really do free it in case of error.", no ? > > I meant "so we don't have to free pi in case of ida_alloc failure". That is better. Please rephrase the commit message to this. >>> /* set up pi->dev before pi_probe_unit() so it can use dev_printk() */ >>> pi->dev.parent = &pata_parport_bus; >>> pi->dev.bus = &pata_parport_bus_type; >>> pi->dev.driver = &pr->driver; >>> pi->dev.release = pata_parport_dev_release; >>> - id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); >>> - if (id < 0) >>> - return NULL; /* pata_parport_dev_release will do kfree(pi) */ >>> pi->dev.id = id; >>> dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id); >>> if (device_register(&pi->dev)) { >>> @@ -571,7 +572,7 @@ static struct pi_adapter *pi_init_one(struct parport *parport, >>> out_unreg_dev: >>> device_unregister(&pi->dev); >> >> Same comment as Sergey: isn't this going to do the ida free ? So shouldn't you >> return here ? > > No. device_unregister() calls pata_parport_dev_release() which does only kfree(pi), not ida_free(). But it probably should do ida_free() too. Yes, it should, otherwise you are leaking the ida with the normal (no errors) case. Care to send a fix for that too ? -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pata_parport: fix possible memory leak 2023-03-12 23:17 ` Damien Le Moal @ 2023-03-13 7:53 ` Ondrej Zary 2023-03-14 22:58 ` [PATCH v3] pata_parport: fix memory leaks Ondrej Zary 1 sibling, 0 replies; 12+ messages in thread From: Ondrej Zary @ 2023-03-13 7:53 UTC (permalink / raw) To: Damien Le Moal Cc: Christoph Hellwig, Sergey Shtylyov, linux-ide, linux-kernel On Monday 13 March 2023, Damien Le Moal wrote: > On 3/13/23 06:24, Ondrej Zary wrote: > > On Sunday 12 March 2023 01:56:25 Damien Le Moal wrote: > >> On 3/12/23 06:44, Ondrej Zary wrote: > >>> When ida_alloc() fails, "pi" is not freed although the misleading > >>> comment says otherwise. > >>> Move the ida_alloc() call up so we really don't have to free it. > >> > >> Certainly you meant: "so we really do free it in case of error.", no ? > > > > I meant "so we don't have to free pi in case of ida_alloc failure". > > That is better. Please rephrase the commit message to this. > > >>> /* set up pi->dev before pi_probe_unit() so it can use dev_printk() */ > >>> pi->dev.parent = &pata_parport_bus; > >>> pi->dev.bus = &pata_parport_bus_type; > >>> pi->dev.driver = &pr->driver; > >>> pi->dev.release = pata_parport_dev_release; > >>> - id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); > >>> - if (id < 0) > >>> - return NULL; /* pata_parport_dev_release will do kfree(pi) */ > >>> pi->dev.id = id; > >>> dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id); > >>> if (device_register(&pi->dev)) { > >>> @@ -571,7 +572,7 @@ static struct pi_adapter *pi_init_one(struct parport *parport, > >>> out_unreg_dev: > >>> device_unregister(&pi->dev); > >> > >> Same comment as Sergey: isn't this going to do the ida free ? So shouldn't you > >> return here ? > > > > No. device_unregister() calls pata_parport_dev_release() which does only kfree(pi), not ida_free(). But it probably should do ida_free() too. > > Yes, it should, otherwise you are leaking the ida with the normal (no errors) > case. Care to send a fix for that too ? Yes, I'll send it as soon as I fix a problem that I noticed during testing. The ida is never freed with this fix. And neither "pi" because pata_parport_dev_release is never called (confirmed by adding printk). -- Ondrej Zary ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] pata_parport: fix memory leaks 2023-03-12 23:17 ` Damien Le Moal 2023-03-13 7:53 ` Ondrej Zary @ 2023-03-14 22:58 ` Ondrej Zary 2023-03-16 7:53 ` Damien Le Moal 1 sibling, 1 reply; 12+ messages in thread From: Ondrej Zary @ 2023-03-14 22:58 UTC (permalink / raw) To: Damien Le Moal Cc: Christoph Hellwig, Sergey Shtylyov, linux-ide, linux-kernel When ida_alloc() fails, "pi" is not freed although the misleading comment says otherwise. Move the ida_alloc() call up so we really don't have to free "pi" in case of ida_alloc() failure. Also move ida_free() call from pi_remove_one() to pata_parport_dev_release(). It was dereferencing already freed dev pointer. Testing revealed leak even in non-failure case which was tracked down to missing put_device() call after bus_find_device_by_name(). As a result, pata_parport_dev_release() was never called. Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <error27@gmail.com> Link: https://lore.kernel.org/r/202303111822.IHNchbkp-lkp@intel.com/ Signed-off-by: Ondrej Zary <linux@zary.sk> --- drivers/ata/pata_parport/pata_parport.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c index 6165ee9aa7da..dc77b4c6fcef 100644 --- a/drivers/ata/pata_parport/pata_parport.c +++ b/drivers/ata/pata_parport/pata_parport.c @@ -452,6 +452,7 @@ static void pata_parport_dev_release(struct device *dev) { struct pi_adapter *pi = container_of(dev, struct pi_adapter, dev); + ida_free(&pata_parport_bus_dev_ids, dev->id); kfree(pi); } @@ -503,23 +504,27 @@ static struct pi_adapter *pi_init_one(struct parport *parport, if (bus_for_each_dev(&pata_parport_bus_type, NULL, &match, pi_find_dev)) return NULL; + id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); + if (id < 0) + return NULL; + pi = kzalloc(sizeof(struct pi_adapter), GFP_KERNEL); - if (!pi) + if (!pi) { + ida_free(&pata_parport_bus_dev_ids, id); return NULL; + } /* set up pi->dev before pi_probe_unit() so it can use dev_printk() */ pi->dev.parent = &pata_parport_bus; pi->dev.bus = &pata_parport_bus_type; pi->dev.driver = &pr->driver; pi->dev.release = pata_parport_dev_release; - id = ida_alloc(&pata_parport_bus_dev_ids, GFP_KERNEL); - if (id < 0) - return NULL; /* pata_parport_dev_release will do kfree(pi) */ pi->dev.id = id; dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id); if (device_register(&pi->dev)) { put_device(&pi->dev); - goto out_ida_free; + /* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */ + return NULL; } pi->proto = pr; @@ -534,8 +539,7 @@ static struct pi_adapter *pi_init_one(struct parport *parport, pi->port = parport->base; par_cb.private = pi; - pi->pardev = parport_register_dev_model(parport, DRV_NAME, &par_cb, - pi->dev.id); + pi->pardev = parport_register_dev_model(parport, DRV_NAME, &par_cb, id); if (!pi->pardev) goto out_module_put; @@ -570,8 +574,7 @@ static struct pi_adapter *pi_init_one(struct parport *parport, module_put(pi->proto->owner); out_unreg_dev: device_unregister(&pi->dev); -out_ida_free: - ida_free(&pata_parport_bus_dev_ids, pi->dev.id); + /* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */ return NULL; } @@ -696,8 +699,7 @@ static void pi_remove_one(struct device *dev) pi_disconnect(pi); pi_release(pi); device_unregister(dev); - ida_free(&pata_parport_bus_dev_ids, dev->id); - /* pata_parport_dev_release will do kfree(pi) */ + /* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */ } static ssize_t delete_device_store(struct bus_type *bus, const char *buf, @@ -713,6 +715,7 @@ static ssize_t delete_device_store(struct bus_type *bus, const char *buf, } pi_remove_one(dev); + put_device(dev); mutex_unlock(&pi_mutex); return count; -- Ondrej Zary ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] pata_parport: fix memory leaks 2023-03-14 22:58 ` [PATCH v3] pata_parport: fix memory leaks Ondrej Zary @ 2023-03-16 7:53 ` Damien Le Moal 0 siblings, 0 replies; 12+ messages in thread From: Damien Le Moal @ 2023-03-16 7:53 UTC (permalink / raw) To: Ondrej Zary; +Cc: Christoph Hellwig, Sergey Shtylyov, linux-ide, linux-kernel On 3/15/23 07:58, Ondrej Zary wrote: > When ida_alloc() fails, "pi" is not freed although the misleading > comment says otherwise. > Move the ida_alloc() call up so we really don't have to free "pi" in > case of ida_alloc() failure. > > Also move ida_free() call from pi_remove_one() to > pata_parport_dev_release(). It was dereferencing already freed dev > pointer. > > Testing revealed leak even in non-failure case which was tracked down > to missing put_device() call after bus_find_device_by_name(). As a > result, pata_parport_dev_release() was never called. > > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <error27@gmail.com> > Link: https://lore.kernel.org/r/202303111822.IHNchbkp-lkp@intel.com/ > Signed-off-by: Ondrej Zary <linux@zary.sk> Applied to for-6.3-fixes. Thanks ! -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-03-16 7:53 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <3ab89ddc-cb60-47c4-86ad-cdad94a8a3d7@kili.mountain> 2023-03-11 18:51 ` [PATCH] pata_parport: fix possible memory leak Ondrej Zary 2023-03-11 20:19 ` Sergei Shtylyov 2023-03-11 20:23 ` Sergey Shtylyov 2023-03-11 21:11 ` Ondrej Zary 2023-03-11 21:39 ` Ondrej Zary 2023-03-11 21:44 ` [PATCH v2] " Ondrej Zary 2023-03-12 0:56 ` Damien Le Moal 2023-03-12 21:24 ` Ondrej Zary 2023-03-12 23:17 ` Damien Le Moal 2023-03-13 7:53 ` Ondrej Zary 2023-03-14 22:58 ` [PATCH v3] pata_parport: fix memory leaks Ondrej Zary 2023-03-16 7:53 ` Damien Le Moal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).