[PATCH 0/2] mtd: use put_device() if device_register fail

[PATCH 0/2] mtd: use put_device() if device_register fail

[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

 drivers/mtd/mtdcore.c | 1 +
 drivers/mtd/ubi/vmt.c | 1 +
 2 files changed, 2 insertions(+)

-- 
1.9.1

[PATCH 1/2] mtd: use put_device() if device_register fail

[Arvind Yadav]

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

[PATCH 2/2] mtd: ubi: use put_device() if device_register fail

[Arvind Yadav]

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

Re: [PATCH 0/2] mtd: use put_device() if device_register fail

[Richard Weinberger]

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

Re: [PATCH 0/2] mtd: use put_device() if device_register fail

[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.

~arvind

Re: [PATCH 0/2] mtd: use put_device() if device_register fail

[Richard Weinberger]

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

Re: [PATCH 1/2] mtd: use put_device() if device_register fail

[Boris Brezillon]

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

Re: [PATCH 2/2] mtd: ubi: use put_device() if device_register fail

[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.

>  	return err;
>  }



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/2] mtd: ubi: use put_device() if device_register fail

[Richard Weinberger]

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

Re: [PATCH 2/2] mtd: ubi: use put_device() if device_register fail

[Arvind Yadav]

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

Re: [PATCH 1/2] mtd: use put_device() if device_register fail

[arvindY]

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

Re: [PATCH 1/2] mtd: use put_device() if device_register fail

[Martin Habets]

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/
> 

Re: [PATCH 1/2] mtd: use put_device() if device_register fail

[Martin Habets]

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