* [PATCH 0/2] mtd: use put_device() if device_register fail
@ 2018-03-09 10:50 Arvind Yadav
2018-03-09 10:50 ` [PATCH 1/2] " Arvind Yadav
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Arvind Yadav @ 2018-03-09 10:50 UTC (permalink / raw)
To: dwmw2, computersforpeace, boris.brezillon, marek.vasut, richard,
cyrille.pitchen, dedekind1
Cc: linux-kernel, linux-mtd
if device_register() returned an error! Always use put_device()
to give up the reference initialized.
Arvind Yadav (2):
[PATCH 1/2] mtd: use put_device() if device_register fail
[PATCH 2/2] mtd: ubi: use put_device() if device_register fail
drivers/mtd/mtdcore.c | 1 +
drivers/mtd/ubi/vmt.c | 1 +
2 files changed, 2 insertions(+)
--
1.9.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/2] mtd: use put_device() if device_register fail 2018-03-09 10:50 [PATCH 0/2] mtd: use put_device() if device_register fail Arvind Yadav @ 2018-03-09 10:50 ` Arvind Yadav 2018-03-14 14:36 ` Boris Brezillon 2018-03-09 10:50 ` [PATCH 2/2] mtd: ubi: " Arvind Yadav 2018-03-11 19:35 ` [PATCH 0/2] mtd: " Richard Weinberger 2 siblings, 1 reply; 13+ messages in thread From: Arvind Yadav @ 2018-03-09 10:50 UTC (permalink / raw) To: dwmw2, computersforpeace, boris.brezillon, marek.vasut, richard, cyrille.pitchen, dedekind1 Cc: linux-kernel, linux-mtd if device_register() returned an error! Always use put_device() to give up the reference initialized. Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> --- drivers/mtd/mtdcore.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 28553c8..4d77ca2 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -586,6 +586,7 @@ int add_mtd_device(struct mtd_info *mtd) return 0; fail_added: + put_device(&mtd->dev); of_node_put(mtd_get_of_node(mtd)); idr_remove(&mtd_idr, i); fail_locked: -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] mtd: use put_device() if device_register fail 2018-03-09 10:50 ` [PATCH 1/2] " Arvind Yadav @ 2018-03-14 14:36 ` Boris Brezillon 2018-03-17 9:45 ` arvindY 0 siblings, 1 reply; 13+ messages in thread From: Boris Brezillon @ 2018-03-14 14:36 UTC (permalink / raw) To: Arvind Yadav Cc: dwmw2, computersforpeace, boris.brezillon, marek.vasut, richard, cyrille.pitchen, dedekind1, linux-kernel, linux-mtd On Fri, 9 Mar 2018 16:20:48 +0530 Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: > if device_register() returned an error! Always use put_device() > to give up the reference initialized. > > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> > --- > drivers/mtd/mtdcore.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 28553c8..4d77ca2 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -586,6 +586,7 @@ int add_mtd_device(struct mtd_info *mtd) > return 0; > > fail_added: > + put_device(&mtd->dev); Not sure this is a good idea: the put_device() call will trigger an mtd_devtype->release(), which will in turn call device_destroy() on something that does not exist yet. Not sure if this is a real problem, but it does not look like the right thing to do. > of_node_put(mtd_get_of_node(mtd)); You're referencing an object that is supposed to have been freed/released by the put_device() call. Again, it's not really a problem because in our case ->release() does not free the mtd object (as is usually done in other parts of the kernel), but it still looks wrong. It's probably better to move the of_node_put() and the below idr_remove() call in the ->release() hook if you want to use put_device(). > idr_remove(&mtd_idr, i); > fail_locked: -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] mtd: use put_device() if device_register fail 2018-03-14 14:36 ` Boris Brezillon @ 2018-03-17 9:45 ` arvindY 2018-03-19 10:43 ` Martin Habets 0 siblings, 1 reply; 13+ messages in thread From: arvindY @ 2018-03-17 9:45 UTC (permalink / raw) To: Boris Brezillon Cc: dwmw2, computersforpeace, boris.brezillon, marek.vasut, richard, cyrille.pitchen, dedekind1, linux-kernel, linux-mtd On Wednesday 14 March 2018 08:06 PM, Boris Brezillon wrote: > On Fri, 9 Mar 2018 16:20:48 +0530 > Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: > >> if device_register() returned an error! Always use put_device() >> to give up the reference initialized. >> >> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> >> --- >> drivers/mtd/mtdcore.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c >> index 28553c8..4d77ca2 100644 >> --- a/drivers/mtd/mtdcore.c >> +++ b/drivers/mtd/mtdcore.c >> @@ -586,6 +586,7 @@ int add_mtd_device(struct mtd_info *mtd) >> return 0; >> >> fail_added: >> + put_device(&mtd->dev); > Not sure this is a good idea: the put_device() call will trigger > an mtd_devtype->release(), which will in turn call device_destroy() on > something that does not exist yet. Not sure if this is a real problem, > but it does not look like the right thing to do. > yes, you are correct. No need to call put_device(). which can cause other problem. >> of_node_put(mtd_get_of_node(mtd)); > You're referencing an object that is supposed to have been > freed/released by the put_device() call. Again, it's not really a > problem because in our case ->release() does not free the mtd object > (as is usually done in other parts of the kernel), but it still looks > wrong. It's probably better to move the of_node_put() and the below > idr_remove() call in the ->release() hook if you want to use > put_device(). > >> idr_remove(&mtd_idr, i); Sure, we can move put_device() below this. But need to check how we can add hook in release. > >> fail_locked: > > ~arvind ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] mtd: use put_device() if device_register fail 2018-03-17 9:45 ` arvindY @ 2018-03-19 10:43 ` Martin Habets [not found] ` <5AAFF9C6.2010800@gmail.com> 0 siblings, 1 reply; 13+ messages in thread From: Martin Habets @ 2018-03-19 10:43 UTC (permalink / raw) To: arvind.yadav.cs; +Cc: linux-mtd On 17/03/18 09:45, arvindY wrote: >>> of_node_put(mtd_get_of_node(mtd)); >> You're referencing an object that is supposed to have been >> freed/released by the put_device() call. Again, it's not really a >> problem because in our case ->release() does not free the mtd object >> (as is usually done in other parts of the kernel), but it still looks >> wrong. It's probably better to move the of_node_put() and the below >> idr_remove() call in the ->release() hook if you want to use >> put_device(). >> >>> idr_remove(&mtd_idr, i); > Sure, we can move put_device() below this. But need to check > how we can add hook in release. My guess is that you would need this: http://lists.infradead.org/pipermail/linux-mtd/2017-May/074373.html Martin >>> fail_locked: >> >> > ~arvind > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <5AAFF9C6.2010800@gmail.com>]
* Re: [PATCH 1/2] mtd: use put_device() if device_register fail [not found] ` <5AAFF9C6.2010800@gmail.com> @ 2018-03-21 10:08 ` Martin Habets 0 siblings, 0 replies; 13+ messages in thread From: Martin Habets @ 2018-03-21 10:08 UTC (permalink / raw) To: arvindY; +Cc: linux-mtd Hi Arvind, On 19/03/18 17:56, arvindY wrote: > > > On Monday 19 March 2018 04:13 PM, Martin Habets wrote: >> On 17/03/18 09:45, arvindY wrote: >>>>> of_node_put(mtd_get_of_node(mtd)); >>>> You're referencing an object that is supposed to have been >>>> freed/released by the put_device() call. Again, it's not really a >>>> problem because in our case ->release() does not free the mtd object >>>> (as is usually done in other parts of the kernel), but it still looks >>>> wrong. It's probably better to move the of_node_put() and the below >>>> idr_remove() call in the ->release() hook if you want to use >>>> put_device(). >>>> >>>>> idr_remove(&mtd_idr, i); >>> Sure, we can move put_device() below this. But need to check >>> how we can add hook in release. >> My guess is that you would need this: >> http://lists.infradead.org/pipermail/linux-mtd/2017-May/074373.html > > we should not removes(device_destroy) a MTD device in > release. MTD device should be removes when > deleting(unregister) a MTD device. No, deleting an MTD device should only decrement a refcounter. At this point there can still be other processes with a /dev/mtd* device open. When there are no more users release gets called to remove it. > MTD device should decrement refcount of a node and > Remove MTD from IDR in dev->release(). You could be right about this, I'm not sure. My patch allows the caller to free the mtd_info memory. This is needed since the caller allocated the memory in the first place, and because the caller has no other of knowing that the last MTD user is gone. Martin ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] mtd: ubi: use put_device() if device_register fail 2018-03-09 10:50 [PATCH 0/2] mtd: use put_device() if device_register fail Arvind Yadav 2018-03-09 10:50 ` [PATCH 1/2] " Arvind Yadav @ 2018-03-09 10:50 ` Arvind Yadav 2018-03-14 18:56 ` Boris Brezillon 2018-03-11 19:35 ` [PATCH 0/2] mtd: " Richard Weinberger 2 siblings, 1 reply; 13+ messages in thread From: Arvind Yadav @ 2018-03-09 10:50 UTC (permalink / raw) To: dwmw2, computersforpeace, boris.brezillon, marek.vasut, richard, cyrille.pitchen, dedekind1 Cc: linux-kernel, linux-mtd if device_register() returned an error! Always use put_device() to give up the reference initialized. Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> --- drivers/mtd/ubi/vmt.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c index 3fd8d7f..db85b68 100644 --- a/drivers/mtd/ubi/vmt.c +++ b/drivers/mtd/ubi/vmt.c @@ -609,6 +609,7 @@ int ubi_add_volume(struct ubi_device *ubi, struct ubi_volume *vol) return err; out_cdev: + put_device(&vol->dev); cdev_del(&vol->cdev); return err; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mtd: ubi: use put_device() if device_register fail 2018-03-09 10:50 ` [PATCH 2/2] mtd: ubi: " Arvind Yadav @ 2018-03-14 18:56 ` Boris Brezillon 2018-03-14 19:25 ` Richard Weinberger 0 siblings, 1 reply; 13+ messages in thread From: Boris Brezillon @ 2018-03-14 18:56 UTC (permalink / raw) To: Arvind Yadav Cc: dwmw2, computersforpeace, boris.brezillon, marek.vasut, richard, cyrille.pitchen, dedekind1, linux-mtd, linux-kernel On Fri, 9 Mar 2018 16:20:49 +0530 Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: > if device_register() returned an error! Always use put_device() > to give up the reference initialized. > > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> > --- > drivers/mtd/ubi/vmt.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c > index 3fd8d7f..db85b68 100644 > --- a/drivers/mtd/ubi/vmt.c > +++ b/drivers/mtd/ubi/vmt.c > @@ -609,6 +609,7 @@ int ubi_add_volume(struct ubi_device *ubi, struct ubi_volume *vol) > return err; > > out_cdev: > + put_device(&vol->dev); > cdev_del(&vol->cdev); use-after-free bug here: put_device() has freed the vol obj, and you're dereferencing the pointer just after that. > return err; > } -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mtd: ubi: use put_device() if device_register fail 2018-03-14 18:56 ` Boris Brezillon @ 2018-03-14 19:25 ` Richard Weinberger 2018-03-15 6:41 ` Arvind Yadav 0 siblings, 1 reply; 13+ messages in thread From: Richard Weinberger @ 2018-03-14 19:25 UTC (permalink / raw) To: Boris Brezillon Cc: Arvind Yadav, dwmw2, computersforpeace, boris.brezillon, marek.vasut, cyrille.pitchen, dedekind1, linux-mtd, linux-kernel Am Mittwoch, 14. März 2018, 19:56:52 CET schrieb Boris Brezillon: > On Fri, 9 Mar 2018 16:20:49 +0530 > > Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: > > if device_register() returned an error! Always use put_device() > > to give up the reference initialized. > > > > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> > > --- > > > > drivers/mtd/ubi/vmt.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c > > index 3fd8d7f..db85b68 100644 > > --- a/drivers/mtd/ubi/vmt.c > > +++ b/drivers/mtd/ubi/vmt.c > > @@ -609,6 +609,7 @@ int ubi_add_volume(struct ubi_device *ubi, struct > > ubi_volume *vol)> > > return err; > > > > out_cdev: > > + put_device(&vol->dev); > > > > cdev_del(&vol->cdev); > > use-after-free bug here: put_device() has freed the vol obj, and you're > dereferencing the pointer just after that. eeek, thanks for looking at more context. Arvind, while you are right that put_device() is missing, please double check that freeing the devices is also correct. Thanks, //richard ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mtd: ubi: use put_device() if device_register fail 2018-03-14 19:25 ` Richard Weinberger @ 2018-03-15 6:41 ` Arvind Yadav 0 siblings, 0 replies; 13+ messages in thread From: Arvind Yadav @ 2018-03-15 6:41 UTC (permalink / raw) To: Richard Weinberger, Boris Brezillon Cc: dwmw2, computersforpeace, boris.brezillon, marek.vasut, cyrille.pitchen, dedekind1, linux-mtd, linux-kernel On Thursday 15 March 2018 12:55 AM, Richard Weinberger wrote: > Am Mittwoch, 14. März 2018, 19:56:52 CET schrieb Boris Brezillon: >> On Fri, 9 Mar 2018 16:20:49 +0530 >> >> Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: >>> if device_register() returned an error! Always use put_device() >>> to give up the reference initialized. >>> >>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> >>> --- >>> >>> drivers/mtd/ubi/vmt.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c >>> index 3fd8d7f..db85b68 100644 >>> --- a/drivers/mtd/ubi/vmt.c >>> +++ b/drivers/mtd/ubi/vmt.c >>> @@ -609,6 +609,7 @@ int ubi_add_volume(struct ubi_device *ubi, struct >>> ubi_volume *vol)> >>> return err; >>> >>> out_cdev: >>> + put_device(&vol->dev); >>> >>> cdev_del(&vol->cdev); >> use-after-free bug here: put_device() has freed the vol obj, and you're >> dereferencing the pointer just after that. Thanks Boris, to point out this error. > eeek, thanks for looking at more context. > Arvind, while you are right that put_device() is missing, please double check > that freeing the devices is also correct. > > Thanks, > //richard Sorry for that. I will take care of this. ~arvind ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] mtd: use put_device() if device_register fail 2018-03-09 10:50 [PATCH 0/2] mtd: use put_device() if device_register fail Arvind Yadav 2018-03-09 10:50 ` [PATCH 1/2] " Arvind Yadav 2018-03-09 10:50 ` [PATCH 2/2] mtd: ubi: " Arvind Yadav @ 2018-03-11 19:35 ` Richard Weinberger 2018-03-12 5:51 ` Arvind Yadav 2 siblings, 1 reply; 13+ messages in thread From: Richard Weinberger @ 2018-03-11 19:35 UTC (permalink / raw) To: Arvind Yadav Cc: dwmw2, computersforpeace, boris.brezillon, marek.vasut, cyrille.pitchen, dedekind1, linux-kernel, linux-mtd Am Freitag, 9. März 2018, 11:50:47 CET schrieb Arvind Yadav: > if device_register() returned an error! Always use put_device() > to give up the reference initialized. > > Arvind Yadav (2): > [PATCH 1/2] mtd: use put_device() if device_register fail > [PATCH 2/2] mtd: ubi: use put_device() if device_register fail Uhh, this is not obvious. Does device_register() really always return with a reference held in all (error) cases? Thanks, //richard ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] mtd: use put_device() if device_register fail 2018-03-11 19:35 ` [PATCH 0/2] mtd: " Richard Weinberger @ 2018-03-12 5:51 ` Arvind Yadav 2018-03-12 14:32 ` Richard Weinberger 0 siblings, 1 reply; 13+ messages in thread From: Arvind Yadav @ 2018-03-12 5:51 UTC (permalink / raw) To: Richard Weinberger Cc: dwmw2, computersforpeace, boris.brezillon, marek.vasut, cyrille.pitchen, dedekind1, linux-kernel, linux-mtd On Monday 12 March 2018 01:05 AM, Richard Weinberger wrote: > Am Freitag, 9. März 2018, 11:50:47 CET schrieb Arvind Yadav: >> if device_register() returned an error! Always use put_device() >> to give up the reference initialized. >> >> Arvind Yadav (2): >> [PATCH 1/2] mtd: use put_device() if device_register fail >> [PATCH 2/2] mtd: ubi: use put_device() if device_register fail > Uhh, this is not obvious. Does device_register() really always return with a > reference held in all (error) cases? > > Thanks, > //richard if device_register() returned an error! Always use put_device() to give up the reference initialized.(-- Please see the comment for device_register() ). put_device() is able to handle those case where it'll not return a reference. ~arvind ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] mtd: use put_device() if device_register fail 2018-03-12 5:51 ` Arvind Yadav @ 2018-03-12 14:32 ` Richard Weinberger 0 siblings, 0 replies; 13+ messages in thread From: Richard Weinberger @ 2018-03-12 14:32 UTC (permalink / raw) To: Arvind Yadav Cc: dwmw2, computersforpeace, boris.brezillon, marek.vasut, cyrille.pitchen, dedekind1, linux-kernel, linux-mtd Arvind, Am Montag, 12. März 2018, 06:51:24 CET schrieb Arvind Yadav: > On Monday 12 March 2018 01:05 AM, Richard Weinberger wrote: > > Am Freitag, 9. März 2018, 11:50:47 CET schrieb Arvind Yadav: > >> if device_register() returned an error! Always use put_device() > >> to give up the reference initialized. > >> > >> Arvind Yadav (2): > >> [PATCH 1/2] mtd: use put_device() if device_register fail > >> [PATCH 2/2] mtd: ubi: use put_device() if device_register fail > > > > Uhh, this is not obvious. Does device_register() really always return with > > a reference held in all (error) cases? > > > > Thanks, > > //richard > > if device_register() returned an error! Always use put_device() > to give up the reference initialized.(-- Please see the comment > for device_register() ). put_device() is able to handle those case > where it'll not return a reference. You are right. For both patches: Acked-by: Richard Weinberger <richard@nod.at> Thanks, //richard ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-03-21 10:08 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-09 10:50 [PATCH 0/2] mtd: use put_device() if device_register fail Arvind Yadav
2018-03-09 10:50 ` [PATCH 1/2] " Arvind Yadav
2018-03-14 14:36 ` Boris Brezillon
2018-03-17 9:45 ` arvindY
2018-03-19 10:43 ` Martin Habets
[not found] ` <5AAFF9C6.2010800@gmail.com>
2018-03-21 10:08 ` Martin Habets
2018-03-09 10:50 ` [PATCH 2/2] mtd: ubi: " Arvind Yadav
2018-03-14 18:56 ` Boris Brezillon
2018-03-14 19:25 ` Richard Weinberger
2018-03-15 6:41 ` Arvind Yadav
2018-03-11 19:35 ` [PATCH 0/2] mtd: " Richard Weinberger
2018-03-12 5:51 ` Arvind Yadav
2018-03-12 14:32 ` Richard Weinberger
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).