[v2] genirq/irqdesc: fix WARNING in irq_sysfs_del()

  I got the lots of WARNING report when doing fault injection test:

kernfs: can not remove 'chip_name', no directory
WARNING: CPU: 0 PID: 253 at fs/kernfs/dir.c:1616 kernfs_remove_by_name_ns+0xce/0xe0
RIP: 0010:kernfs_remove_by_name_ns+0xce/0xe0
Call Trace:
 <TASK>
 remove_files.isra.1+0x3f/0xb0
 sysfs_remove_group+0x68/0xe0
 sysfs_remove_groups+0x41/0x70
 __kobject_del+0x45/0xc0
 kobject_del+0x29/0x40
 free_desc+0x42/0x70
 irq_free_descs+0x5e/0x90

kernfs: can not remove 'hwirq', no directory
WARNING: CPU: 0 PID: 253 at fs/kernfs/dir.c:1616 kernfs_remove_by_name_ns+0xce/0xe0
RIP: 0010:kernfs_remove_by_name_ns+0xce/0xe0
Call Trace:
 <TASK>
 remove_files.isra.1+0x3f/0xb0
 sysfs_remove_group+0x68/0xe0
 sysfs_remove_groups+0x41/0x70
 __kobject_del+0x45/0xc0
 kobject_del+0x29/0x40
 free_desc+0x42/0x70
 irq_free_descs+0x5e/0x90

If irq_sysfs_add() fails in alloc_descs(), the directory of interrupt
informations is not added to sysfs, it causes the WARNINGs when removing
the information files. Add 'sysfs_added' field in struct irq_desc to
indicate if it is added, and check it before calling kobject_del() to
avoid these WARNINGs.

Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
v1 -> v2:
  Don't use state_in_sysfs, introduce 'sysfs_added' to indicate if it is added.
---
 include/linux/irqdesc.h | 1 +
 kernel/irq/irqdesc.c    | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)
  

On Mon, Nov 28, 2022 at 11:16:12PM +0800, Yang Yingliang wrote:
> I got the lots of WARNING report when doing fault injection test:
> 
> kernfs: can not remove 'chip_name', no directory
> WARNING: CPU: 0 PID: 253 at fs/kernfs/dir.c:1616 kernfs_remove_by_name_ns+0xce/0xe0
> RIP: 0010:kernfs_remove_by_name_ns+0xce/0xe0
> Call Trace:
>  <TASK>
>  remove_files.isra.1+0x3f/0xb0
>  sysfs_remove_group+0x68/0xe0
>  sysfs_remove_groups+0x41/0x70
>  __kobject_del+0x45/0xc0
>  kobject_del+0x29/0x40
>  free_desc+0x42/0x70
>  irq_free_descs+0x5e/0x90
> 
> kernfs: can not remove 'hwirq', no directory
> WARNING: CPU: 0 PID: 253 at fs/kernfs/dir.c:1616 kernfs_remove_by_name_ns+0xce/0xe0
> RIP: 0010:kernfs_remove_by_name_ns+0xce/0xe0
> Call Trace:
>  <TASK>
>  remove_files.isra.1+0x3f/0xb0
>  sysfs_remove_group+0x68/0xe0
>  sysfs_remove_groups+0x41/0x70
>  __kobject_del+0x45/0xc0
>  kobject_del+0x29/0x40
>  free_desc+0x42/0x70
>  irq_free_descs+0x5e/0x90
> 
> If irq_sysfs_add() fails in alloc_descs(), the directory of interrupt
> informations is not added to sysfs, it causes the WARNINGs when removing
> the information files. Add 'sysfs_added' field in struct irq_desc to
> indicate if it is added, and check it before calling kobject_del() to
> avoid these WARNINGs.
> 
> Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> v1 -> v2:
>   Don't use state_in_sysfs, introduce 'sysfs_added' to indicate if it is added.
> ---
>  include/linux/irqdesc.h | 1 +
>  kernel/irq/irqdesc.c    | 7 +++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
> index 844a8e30e6de..fec0f3946a34 100644
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -97,6 +97,7 @@ struct irq_desc {
>  #ifdef CONFIG_SPARSE_IRQ
>  	struct rcu_head		rcu;
>  	struct kobject		kobj;
> +	bool			sysfs_added;
>  #endif
>  	struct mutex		request_mutex;
>  	int			parent_irq;
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index a91f9001103c..9bf74d11bad5 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -292,6 +292,8 @@ static void irq_sysfs_add(int irq, struct irq_desc *desc)
>  		 */
>  		if (kobject_add(&desc->kobj, irq_kobj_base, "%d", irq))
>  			pr_warn("Failed to add kobject for irq %d\n", irq);
> +		else
> +			desc->sysfs_added = true;

Wait, no.  Why are you just not properly failing and unwinding here?
Why do you need a special flag just to say "sysfs worked" or not unlike
all other users of kobjects.

Fix this up properly please.

thanks,

greg k-h
  

On Mon, Nov 28 2022 at 18:20, Greg KH wrote:
> On Mon, Nov 28, 2022 at 11:16:12PM +0800, Yang Yingliang wrote:
>> @@ -292,6 +292,8 @@ static void irq_sysfs_add(int irq, struct irq_desc *desc)
>>  		 */
>>  		if (kobject_add(&desc->kobj, irq_kobj_base, "%d", irq))
>>  			pr_warn("Failed to add kobject for irq %d\n", irq);
>> +		else
>> +			desc->sysfs_added = true;
>
> Wait, no.  Why are you just not properly failing and unwinding here?

There is an issue here.

sysfs is not yet available when the first interrupts are allocated. So
we add the sysfs files late in the boot.

So what can we do if that fails? Unwind the boot process? :)

Sure we can fail after sysfs has been initialized, but that's
inconsistent at best and we need some special treatment for the late add
anyway.

I agree that this is not pretty, but the resulting choices are all but
pretty.

Thanks,

        tglx
  

On 2022/11/29 1:20, Greg KH wrote:
> On Mon, Nov 28, 2022 at 11:16:12PM +0800, Yang Yingliang wrote:
>> I got the lots of WARNING report when doing fault injection test:
>>
>> kernfs: can not remove 'chip_name', no directory
>> WARNING: CPU: 0 PID: 253 at fs/kernfs/dir.c:1616 kernfs_remove_by_name_ns+0xce/0xe0
>> RIP: 0010:kernfs_remove_by_name_ns+0xce/0xe0
>> Call Trace:
>>   <TASK>
>>   remove_files.isra.1+0x3f/0xb0
>>   sysfs_remove_group+0x68/0xe0
>>   sysfs_remove_groups+0x41/0x70
>>   __kobject_del+0x45/0xc0
>>   kobject_del+0x29/0x40
>>   free_desc+0x42/0x70
>>   irq_free_descs+0x5e/0x90
>>
>> kernfs: can not remove 'hwirq', no directory
>> WARNING: CPU: 0 PID: 253 at fs/kernfs/dir.c:1616 kernfs_remove_by_name_ns+0xce/0xe0
>> RIP: 0010:kernfs_remove_by_name_ns+0xce/0xe0
>> Call Trace:
>>   <TASK>
>>   remove_files.isra.1+0x3f/0xb0
>>   sysfs_remove_group+0x68/0xe0
>>   sysfs_remove_groups+0x41/0x70
>>   __kobject_del+0x45/0xc0
>>   kobject_del+0x29/0x40
>>   free_desc+0x42/0x70
>>   irq_free_descs+0x5e/0x90
>>
>> If irq_sysfs_add() fails in alloc_descs(), the directory of interrupt
>> informations is not added to sysfs, it causes the WARNINGs when removing
>> the information files. Add 'sysfs_added' field in struct irq_desc to
>> indicate if it is added, and check it before calling kobject_del() to
>> avoid these WARNINGs.
>>
>> Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>> v1 -> v2:
>>    Don't use state_in_sysfs, introduce 'sysfs_added' to indicate if it is added.
>> ---
>>   include/linux/irqdesc.h | 1 +
>>   kernel/irq/irqdesc.c    | 7 +++++--
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
>> index 844a8e30e6de..fec0f3946a34 100644
>> --- a/include/linux/irqdesc.h
>> +++ b/include/linux/irqdesc.h
>> @@ -97,6 +97,7 @@ struct irq_desc {
>>   #ifdef CONFIG_SPARSE_IRQ
>>   	struct rcu_head		rcu;
>>   	struct kobject		kobj;
>> +	bool			sysfs_added;
>>   #endif
>>   	struct mutex		request_mutex;
>>   	int			parent_irq;
>> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
>> index a91f9001103c..9bf74d11bad5 100644
>> --- a/kernel/irq/irqdesc.c
>> +++ b/kernel/irq/irqdesc.c
>> @@ -292,6 +292,8 @@ static void irq_sysfs_add(int irq, struct irq_desc *desc)
>>   		 */
>>   		if (kobject_add(&desc->kobj, irq_kobj_base, "%d", irq))
>>   			pr_warn("Failed to add kobject for irq %d\n", irq);
>> +		else
>> +			desc->sysfs_added = true;
> Wait, no.  Why are you just not properly failing and unwinding here?
We can not call kobject_put() here, it will free the 'desc' in 
irq_kobj_release(),
the irq is still in use and the 'desc' should be freed in free_desc(), 
so the failure
handled in irq_sysfs_del().

If irq_sysfs_add() fails, it does nothing except print message,
we don't know if it's added successfully while calling irq_sysfs_del(),
so I introduced a filed to store the return status that can be used in
irq_sysfs_add().

alloc_descs()
   irq_sysfs_add(desc) <-- it's failed and does nothing except print message

irq_free_descs()
   free_desc()
     irq_sysfs_del(desc) <-- it doesn't know irq_sysfs_add() is failed.
     delayed_free_desc()
       kfree(desc)

I this case, If dont' use a flag, I can not figure out a better way to
let irq_sysfs_del() know it's added failed.

Thanks,
Yang
> Why do you need a special flag just to say "sysfs worked" or not unlike
> all other users of kobjects.
>
> Fix this up properly please.
>
> thanks,
>
> greg k-h
> .
  

On Mon, Nov 28, 2022 at 07:55:17PM +0100, Thomas Gleixner wrote:
> On Mon, Nov 28 2022 at 18:20, Greg KH wrote:
> > On Mon, Nov 28, 2022 at 11:16:12PM +0800, Yang Yingliang wrote:
> >> @@ -292,6 +292,8 @@ static void irq_sysfs_add(int irq, struct irq_desc *desc)
> >>  		 */
> >>  		if (kobject_add(&desc->kobj, irq_kobj_base, "%d", irq))
> >>  			pr_warn("Failed to add kobject for irq %d\n", irq);
> >> +		else
> >> +			desc->sysfs_added = true;
> >
> > Wait, no.  Why are you just not properly failing and unwinding here?
> 
> There is an issue here.
> 
> sysfs is not yet available when the first interrupts are allocated. So
> we add the sysfs files late in the boot.
> 
> So what can we do if that fails? Unwind the boot process? :)
> 
> Sure we can fail after sysfs has been initialized, but that's
> inconsistent at best and we need some special treatment for the late add
> anyway.
> 
> I agree that this is not pretty, but the resulting choices are all but
> pretty.

Ah, ok, that makes more sense.  In this case, yes, the flag should be
fine to have.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>