* [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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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 0 siblings, 0 replies; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread
end of thread, other threads:[~2018-03-17 9:45 UTC | newest] Thread overview: 11+ 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-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).