[PATCH v2] chardev: fix error handling in cdev_device_add()

[PATCH v2] chardev: fix error handling in cdev_device_add()

[Yang Yingliang]

While doing fault injection test, I got the following report:

------------[ cut here ]------------
kobject: '(null)' (0000000039956980): is not initialized, yet kobject_put() is being called.
WARNING: CPU: 3 PID: 6306 at kobject_put+0x23d/0x4e0
CPU: 3 PID: 6306 Comm: 283 Tainted: G        W          6.1.0-rc2-00005-g307c1086d7c9 #1253
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
RIP: 0010:kobject_put+0x23d/0x4e0
Call Trace:
 <TASK>
 cdev_device_add+0x15e/0x1b0
 __iio_device_register+0x13b4/0x1af0 [industrialio]
 __devm_iio_device_register+0x22/0x90 [industrialio]
 max517_probe+0x3d8/0x6b4 [max517]
 i2c_device_probe+0xa81/0xc00

When device_add() is injected fault and returns error, if dev->devt is not set,
cdev_add() is not called, cdev_del() is not needed. Fix this by checking dev->devt
in error path.

Fixes: 233ed09d7fda ("chardev: add helper function to register char devs with a struct device")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
v1 -> v2:
  Add information to update commit message.
  v1 link: https://lore.kernel.org/lkml/1959fa74-b06c-b8bc-d14f-b71e5c4290ee@huawei.com/T/
---
 fs/char_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index ba0ded7842a7..3f667292608c 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -547,7 +547,7 @@ int cdev_device_add(struct cdev *cdev, struct device *dev)
 	}
 
 	rc = device_add(dev);
-	if (rc)
+	if (rc && dev->devt)
 		cdev_del(cdev);
 
 	return rc;
-- 
2.25.1

Re: [PATCH v2] chardev: fix error handling in cdev_device_add()

[Greg KH]

On Tue, Oct 25, 2022 at 07:39:57PM +0800, Yang Yingliang wrote:
> While doing fault injection test, I got the following report:
> 
> ------------[ cut here ]------------
> kobject: '(null)' (0000000039956980): is not initialized, yet kobject_put() is being called.
> WARNING: CPU: 3 PID: 6306 at kobject_put+0x23d/0x4e0
> CPU: 3 PID: 6306 Comm: 283 Tainted: G        W          6.1.0-rc2-00005-g307c1086d7c9 #1253
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> RIP: 0010:kobject_put+0x23d/0x4e0
> Call Trace:
>  <TASK>
>  cdev_device_add+0x15e/0x1b0
>  __iio_device_register+0x13b4/0x1af0 [industrialio]
>  __devm_iio_device_register+0x22/0x90 [industrialio]
>  max517_probe+0x3d8/0x6b4 [max517]
>  i2c_device_probe+0xa81/0xc00
> 
> When device_add() is injected fault and returns error, if dev->devt is not set,
> cdev_add() is not called, cdev_del() is not needed. Fix this by checking dev->devt
> in error path.

Nit, please wrap your changelog text at 72 columns.

> 
> Fixes: 233ed09d7fda ("chardev: add helper function to register char devs with a struct device")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> v1 -> v2:
>   Add information to update commit message.
>   v1 link: https://lore.kernel.org/lkml/1959fa74-b06c-b8bc-d14f-b71e5c4290ee@huawei.com/T/
> ---
>  fs/char_dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/char_dev.c b/fs/char_dev.c
> index ba0ded7842a7..3f667292608c 100644
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -547,7 +547,7 @@ int cdev_device_add(struct cdev *cdev, struct device *dev)
>  	}
>  
>  	rc = device_add(dev);
> -	if (rc)
> +	if (rc && dev->devt)

No, this is a layering violation and one that you do not know is really
going to be true or not.  the devt being present, or not, should not be
an issue of if the device_add failed or not.  This isn't correct, sorry.

thanks,

greg k-h

Re: [PATCH v2] chardev: fix error handling in cdev_device_add()

[Yang Yingliang]

Hi, Greg

On 2022/10/25 19:50, Greg KH wrote:
> On Tue, Oct 25, 2022 at 07:39:57PM +0800, Yang Yingliang wrote:
>> While doing fault injection test, I got the following report:
>>
>> ------------[ cut here ]------------
>> kobject: '(null)' (0000000039956980): is not initialized, yet kobject_put() is being called.
>> WARNING: CPU: 3 PID: 6306 at kobject_put+0x23d/0x4e0
>> CPU: 3 PID: 6306 Comm: 283 Tainted: G        W          6.1.0-rc2-00005-g307c1086d7c9 #1253
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
>> RIP: 0010:kobject_put+0x23d/0x4e0
>> Call Trace:
>>   <TASK>
>>   cdev_device_add+0x15e/0x1b0
>>   __iio_device_register+0x13b4/0x1af0 [industrialio]
>>   __devm_iio_device_register+0x22/0x90 [industrialio]
>>   max517_probe+0x3d8/0x6b4 [max517]
>>   i2c_device_probe+0xa81/0xc00
>>
>> When device_add() is injected fault and returns error, if dev->devt is not set,
>> cdev_add() is not called, cdev_del() is not needed. Fix this by checking dev->devt
>> in error path.
> Nit, please wrap your changelog text at 72 columns.
>
>> Fixes: 233ed09d7fda ("chardev: add helper function to register char devs with a struct device")
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>> v1 -> v2:
>>    Add information to update commit message.
>>    v1 link: https://lore.kernel.org/lkml/1959fa74-b06c-b8bc-d14f-b71e5c4290ee@huawei.com/T/
>> ---
>>   fs/char_dev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/char_dev.c b/fs/char_dev.c
>> index ba0ded7842a7..3f667292608c 100644
>> --- a/fs/char_dev.c
>> +++ b/fs/char_dev.c
>> @@ -547,7 +547,7 @@ int cdev_device_add(struct cdev *cdev, struct device *dev)
>>   	}
>>   
>>   	rc = device_add(dev);
>> -	if (rc)
>> +	if (rc && dev->devt)
> No, this is a layering violation and one that you do not know is really
> going to be true or not.  the devt being present, or not, should not be
> an issue of if the device_add failed or not.  This isn't correct, sorry.
Do you mean it's not a bug or the warn can be ignored or it's bug in 
driver ?
I see devt is checked before calling cdev_del() in cdev_device_del().

Thanks,
Yang
>
> thanks,
>
> greg k-h
> .

Re: [PATCH v2] chardev: fix error handling in cdev_device_add()

[Greg KH]

On Tue, Oct 25, 2022 at 09:20:12PM +0800, Yang Yingliang wrote:
> Hi, Greg
> 
> On 2022/10/25 19:50, Greg KH wrote:
> > On Tue, Oct 25, 2022 at 07:39:57PM +0800, Yang Yingliang wrote:
> > > While doing fault injection test, I got the following report:
> > > 
> > > ------------[ cut here ]------------
> > > kobject: '(null)' (0000000039956980): is not initialized, yet kobject_put() is being called.
> > > WARNING: CPU: 3 PID: 6306 at kobject_put+0x23d/0x4e0
> > > CPU: 3 PID: 6306 Comm: 283 Tainted: G        W          6.1.0-rc2-00005-g307c1086d7c9 #1253
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> > > RIP: 0010:kobject_put+0x23d/0x4e0
> > > Call Trace:
> > >   <TASK>
> > >   cdev_device_add+0x15e/0x1b0
> > >   __iio_device_register+0x13b4/0x1af0 [industrialio]
> > >   __devm_iio_device_register+0x22/0x90 [industrialio]
> > >   max517_probe+0x3d8/0x6b4 [max517]
> > >   i2c_device_probe+0xa81/0xc00
> > > 
> > > When device_add() is injected fault and returns error, if dev->devt is not set,
> > > cdev_add() is not called, cdev_del() is not needed. Fix this by checking dev->devt
> > > in error path.
> > Nit, please wrap your changelog text at 72 columns.
> > 
> > > Fixes: 233ed09d7fda ("chardev: add helper function to register char devs with a struct device")
> > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> > > ---
> > > v1 -> v2:
> > >    Add information to update commit message.
> > >    v1 link: https://lore.kernel.org/lkml/1959fa74-b06c-b8bc-d14f-b71e5c4290ee@huawei.com/T/
> > > ---
> > >   fs/char_dev.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/char_dev.c b/fs/char_dev.c
> > > index ba0ded7842a7..3f667292608c 100644
> > > --- a/fs/char_dev.c
> > > +++ b/fs/char_dev.c
> > > @@ -547,7 +547,7 @@ int cdev_device_add(struct cdev *cdev, struct device *dev)
> > >   	}
> > >   	rc = device_add(dev);
> > > -	if (rc)
> > > +	if (rc && dev->devt)
> > No, this is a layering violation and one that you do not know is really
> > going to be true or not.  the devt being present, or not, should not be
> > an issue of if the device_add failed or not.  This isn't correct, sorry.
> Do you mean it's not a bug or the warn can be ignored or it's bug in driver
> ?
> I see devt is checked before calling cdev_del() in cdev_device_del().

Ah!  The core doesn't set devt, the caller has that set.  That makes
more sense now, sorry for the confusion on my side.

Yes, this looks correct, the diff didn't have the full context and I was
confused.

I'll go queue this up, very nice work.

greg k-h

Re: [PATCH v2] chardev: fix error handling in cdev_device_add()

[Yang Yingliang]

Hi Greg,

On 2022/10/25 21:37, Greg KH wrote:
> On Tue, Oct 25, 2022 at 09:20:12PM +0800, Yang Yingliang wrote:
>> Hi, Greg
>>
>> On 2022/10/25 19:50, Greg KH wrote:
>>> On Tue, Oct 25, 2022 at 07:39:57PM +0800, Yang Yingliang wrote:
>>>> While doing fault injection test, I got the following report:
>>>>
>>>> ------------[ cut here ]------------
>>>> kobject: '(null)' (0000000039956980): is not initialized, yet kobject_put() is being called.
>>>> WARNING: CPU: 3 PID: 6306 at kobject_put+0x23d/0x4e0
>>>> CPU: 3 PID: 6306 Comm: 283 Tainted: G        W          6.1.0-rc2-00005-g307c1086d7c9 #1253
>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
>>>> RIP: 0010:kobject_put+0x23d/0x4e0
>>>> Call Trace:
>>>>    <TASK>
>>>>    cdev_device_add+0x15e/0x1b0
>>>>    __iio_device_register+0x13b4/0x1af0 [industrialio]
>>>>    __devm_iio_device_register+0x22/0x90 [industrialio]
>>>>    max517_probe+0x3d8/0x6b4 [max517]
>>>>    i2c_device_probe+0xa81/0xc00
>>>>
>>>> When device_add() is injected fault and returns error, if dev->devt is not set,
>>>> cdev_add() is not called, cdev_del() is not needed. Fix this by checking dev->devt
>>>> in error path.
>>> Nit, please wrap your changelog text at 72 columns.
>>>
>>>> Fixes: 233ed09d7fda ("chardev: add helper function to register char devs with a struct device")
>>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>>>> ---
>>>> v1 -> v2:
>>>>     Add information to update commit message.
>>>>     v1 link: https://lore.kernel.org/lkml/1959fa74-b06c-b8bc-d14f-b71e5c4290ee@huawei.com/T/
>>>> ---
>>>>    fs/char_dev.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/char_dev.c b/fs/char_dev.c
>>>> index ba0ded7842a7..3f667292608c 100644
>>>> --- a/fs/char_dev.c
>>>> +++ b/fs/char_dev.c
>>>> @@ -547,7 +547,7 @@ int cdev_device_add(struct cdev *cdev, struct device *dev)
>>>>    	}
>>>>    	rc = device_add(dev);
>>>> -	if (rc)
>>>> +	if (rc && dev->devt)
>>> No, this is a layering violation and one that you do not know is really
>>> going to be true or not.  the devt being present, or not, should not be
>>> an issue of if the device_add failed or not.  This isn't correct, sorry.
>> Do you mean it's not a bug or the warn can be ignored or it's bug in driver
>> ?
>> I see devt is checked before calling cdev_del() in cdev_device_del().
> Ah!  The core doesn't set devt, the caller has that set.  That makes
> more sense now, sorry for the confusion on my side.
>
> Yes, this looks correct, the diff didn't have the full context and I was
> confused.
>
> I'll go queue this up, very nice work.
>
> greg k-h
I didn't find this patch in your trees, does it been merged?

Thanks,
Yang
> .

Re: [PATCH v2] chardev: fix error handling in cdev_device_add()

[Greg KH]

On Thu, Dec 01, 2022 at 08:06:44PM +0800, Yang Yingliang wrote:
> Hi Greg,
> 
> On 2022/10/25 21:37, Greg KH wrote:
> > On Tue, Oct 25, 2022 at 09:20:12PM +0800, Yang Yingliang wrote:
> > > Hi, Greg
> > > 
> > > On 2022/10/25 19:50, Greg KH wrote:
> > > > On Tue, Oct 25, 2022 at 07:39:57PM +0800, Yang Yingliang wrote:
> > > > > While doing fault injection test, I got the following report:
> > > > > 
> > > > > ------------[ cut here ]------------
> > > > > kobject: '(null)' (0000000039956980): is not initialized, yet kobject_put() is being called.
> > > > > WARNING: CPU: 3 PID: 6306 at kobject_put+0x23d/0x4e0
> > > > > CPU: 3 PID: 6306 Comm: 283 Tainted: G        W          6.1.0-rc2-00005-g307c1086d7c9 #1253
> > > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> > > > > RIP: 0010:kobject_put+0x23d/0x4e0
> > > > > Call Trace:
> > > > >    <TASK>
> > > > >    cdev_device_add+0x15e/0x1b0
> > > > >    __iio_device_register+0x13b4/0x1af0 [industrialio]
> > > > >    __devm_iio_device_register+0x22/0x90 [industrialio]
> > > > >    max517_probe+0x3d8/0x6b4 [max517]
> > > > >    i2c_device_probe+0xa81/0xc00
> > > > > 
> > > > > When device_add() is injected fault and returns error, if dev->devt is not set,
> > > > > cdev_add() is not called, cdev_del() is not needed. Fix this by checking dev->devt
> > > > > in error path.
> > > > Nit, please wrap your changelog text at 72 columns.
> > > > 
> > > > > Fixes: 233ed09d7fda ("chardev: add helper function to register char devs with a struct device")
> > > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> > > > > ---
> > > > > v1 -> v2:
> > > > >     Add information to update commit message.
> > > > >     v1 link: https://lore.kernel.org/lkml/1959fa74-b06c-b8bc-d14f-b71e5c4290ee@huawei.com/T/
> > > > > ---
> > > > >    fs/char_dev.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/char_dev.c b/fs/char_dev.c
> > > > > index ba0ded7842a7..3f667292608c 100644
> > > > > --- a/fs/char_dev.c
> > > > > +++ b/fs/char_dev.c
> > > > > @@ -547,7 +547,7 @@ int cdev_device_add(struct cdev *cdev, struct device *dev)
> > > > >    	}
> > > > >    	rc = device_add(dev);
> > > > > -	if (rc)
> > > > > +	if (rc && dev->devt)
> > > > No, this is a layering violation and one that you do not know is really
> > > > going to be true or not.  the devt being present, or not, should not be
> > > > an issue of if the device_add failed or not.  This isn't correct, sorry.
> > > Do you mean it's not a bug or the warn can be ignored or it's bug in driver
> > > ?
> > > I see devt is checked before calling cdev_del() in cdev_device_del().
> > Ah!  The core doesn't set devt, the caller has that set.  That makes
> > more sense now, sorry for the confusion on my side.
> > 
> > Yes, this looks correct, the diff didn't have the full context and I was
> > confused.
> > 
> > I'll go queue this up, very nice work.
> > 
> > greg k-h
> I didn't find this patch in your trees, does it been merged?

Hm, is this:
4634c973096a ("chardev: Fix potential memory leak when cdev_add() failed")
or is this a different patch?  If different, it's not in my review queue
anymore, sorry, can you resend it?

thanks,

greg k-h

Re: [PATCH v2] chardev: fix error handling in cdev_device_add()

[Yang Yingliang]

On 2022/12/1 20:28, Greg KH wrote:
> On Thu, Dec 01, 2022 at 08:06:44PM +0800, Yang Yingliang wrote:
>> Hi Greg,
>>
>> On 2022/10/25 21:37, Greg KH wrote:
>>> On Tue, Oct 25, 2022 at 09:20:12PM +0800, Yang Yingliang wrote:
>>>> Hi, Greg
>>>>
>>>> On 2022/10/25 19:50, Greg KH wrote:
>>>>> On Tue, Oct 25, 2022 at 07:39:57PM +0800, Yang Yingliang wrote:
>>>>>> While doing fault injection test, I got the following report:
>>>>>>
>>>>>> ------------[ cut here ]------------
>>>>>> kobject: '(null)' (0000000039956980): is not initialized, yet kobject_put() is being called.
>>>>>> WARNING: CPU: 3 PID: 6306 at kobject_put+0x23d/0x4e0
>>>>>> CPU: 3 PID: 6306 Comm: 283 Tainted: G        W          6.1.0-rc2-00005-g307c1086d7c9 #1253
>>>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
>>>>>> RIP: 0010:kobject_put+0x23d/0x4e0
>>>>>> Call Trace:
>>>>>>     <TASK>
>>>>>>     cdev_device_add+0x15e/0x1b0
>>>>>>     __iio_device_register+0x13b4/0x1af0 [industrialio]
>>>>>>     __devm_iio_device_register+0x22/0x90 [industrialio]
>>>>>>     max517_probe+0x3d8/0x6b4 [max517]
>>>>>>     i2c_device_probe+0xa81/0xc00
>>>>>>
>>>>>> When device_add() is injected fault and returns error, if dev->devt is not set,
>>>>>> cdev_add() is not called, cdev_del() is not needed. Fix this by checking dev->devt
>>>>>> in error path.
>>>>> Nit, please wrap your changelog text at 72 columns.
>>>>>
>>>>>> Fixes: 233ed09d7fda ("chardev: add helper function to register char devs with a struct device")
>>>>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>>>>>> ---
>>>>>> v1 -> v2:
>>>>>>      Add information to update commit message.
>>>>>>      v1 link: https://lore.kernel.org/lkml/1959fa74-b06c-b8bc-d14f-b71e5c4290ee@huawei.com/T/
>>>>>> ---
>>>>>>     fs/char_dev.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/char_dev.c b/fs/char_dev.c
>>>>>> index ba0ded7842a7..3f667292608c 100644
>>>>>> --- a/fs/char_dev.c
>>>>>> +++ b/fs/char_dev.c
>>>>>> @@ -547,7 +547,7 @@ int cdev_device_add(struct cdev *cdev, struct device *dev)
>>>>>>     	}
>>>>>>     	rc = device_add(dev);
>>>>>> -	if (rc)
>>>>>> +	if (rc && dev->devt)
>>>>> No, this is a layering violation and one that you do not know is really
>>>>> going to be true or not.  the devt being present, or not, should not be
>>>>> an issue of if the device_add failed or not.  This isn't correct, sorry.
>>>> Do you mean it's not a bug or the warn can be ignored or it's bug in driver
>>>> ?
>>>> I see devt is checked before calling cdev_del() in cdev_device_del().
>>> Ah!  The core doesn't set devt, the caller has that set.  That makes
>>> more sense now, sorry for the confusion on my side.
>>>
>>> Yes, this looks correct, the diff didn't have the full context and I was
>>> confused.
>>>
>>> I'll go queue this up, very nice work.
>>>
>>> greg k-h
>> I didn't find this patch in your trees, does it been merged?
> Hm, is this:
> 4634c973096a ("chardev: Fix potential memory leak when cdev_add() failed")
> or is this a different patch?  If different, it's not in my review queue
> anymore, sorry, can you resend it?
It's a different patch, I can resend it.

Thanks,
Yang
>
> thanks,
>
> greg k-h
> .