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

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

[Yang Yingliang]

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;
 	}
 }
 
@@ -299,11 +301,12 @@ static void irq_sysfs_del(struct irq_desc *desc)
 {
 	/*
 	 * If irq_sysfs_init() has not yet been invoked (early boot), then
-	 * irq_kobj_base is NULL and the descriptor was never added.
+	 * irq_kobj_base is NULL or kobject_add() fails, the descriptor was
+	 * never added.
 	 * kobject_del() complains about a object with no parent, so make
 	 * it conditional.
 	 */
-	if (irq_kobj_base)
+	if (desc->sysfs_added)
 		kobject_del(&desc->kobj);
 }
 
-- 
2.25.1

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

[Greg KH]

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

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

[Thomas Gleixner]

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

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

[Yang Yingliang]

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

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

[Greg KH]

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>

[tip: irq/core] genirq/irqdesc: Don't try to remove non-existing sysfs files

[tip-bot2 for Yang Yingliang]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     9049e1ca41983ab773d7ea244bee86d7835ec9f5
Gitweb:        https://git.kernel.org/tip/9049e1ca41983ab773d7ea244bee86d7835ec9f5
Author:        Yang Yingliang <yangyingliang@huawei.com>
AuthorDate:    Mon, 28 Nov 2022 23:16:12 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 30 Nov 2022 14:52:11 +01:00

genirq/irqdesc: Don't try to remove non-existing sysfs files

Fault injection tests trigger warnings like this:

  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

The reason is that the interrupt descriptor sysfs handling does not roll
back on a failing kobject_add() during allocation. If the descriptor is
freed later on, kobject_del() is invoked with a not added kobject resulting
in the above warnings.

A proper rollback in case of a kobject_add() failure would be the straight
forward solution. But this is not possible due to the way how interrupt
descriptor sysfs handling works.

Interrupt descriptors are allocated before sysfs becomes available. So the
sysfs files for the early allocated descriptors are added later in the boot
process. At this point there can be nothing useful done about a failing
kobject_add(). For consistency the interrupt descriptor allocation always
treats kobject_add() failures as non-critical and just emits a warning.

To solve this problem, keep track in the interrupt descriptor whether
kobject_add() was successful or not and make the invocation of
kobject_del() conditional on that.

[ tglx: Massage changelog, comments and use a state bit. ]

Fixes: ecb3f394c5db ("genirq: Expose interrupt information through sysfs")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Link: https://lore.kernel.org/r/20221128151612.1786122-1-yangyingliang@huawei.com
---
 kernel/irq/internals.h |  2 ++
 kernel/irq/irqdesc.c   | 15 +++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index f09c603..5fdc0b5 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -52,6 +52,7 @@ enum {
  * IRQS_PENDING			- irq is pending and replayed later
  * IRQS_SUSPENDED		- irq is suspended
  * IRQS_NMI			- irq line is used to deliver NMIs
+ * IRQS_SYSFS			- descriptor has been added to sysfs
  */
 enum {
 	IRQS_AUTODETECT		= 0x00000001,
@@ -64,6 +65,7 @@ enum {
 	IRQS_SUSPENDED		= 0x00000800,
 	IRQS_TIMINGS		= 0x00001000,
 	IRQS_NMI		= 0x00002000,
+	IRQS_SYSFS		= 0x00004000,
 };
 
 #include "debug.h"
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index a91f900..fd09962 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -288,22 +288,25 @@ static void irq_sysfs_add(int irq, struct irq_desc *desc)
 	if (irq_kobj_base) {
 		/*
 		 * Continue even in case of failure as this is nothing
-		 * crucial.
+		 * crucial and failures in the late irq_sysfs_init()
+		 * cannot be rolled back.
 		 */
 		if (kobject_add(&desc->kobj, irq_kobj_base, "%d", irq))
 			pr_warn("Failed to add kobject for irq %d\n", irq);
+		else
+			desc->istate |= IRQS_SYSFS;
 	}
 }
 
 static void irq_sysfs_del(struct irq_desc *desc)
 {
 	/*
-	 * If irq_sysfs_init() has not yet been invoked (early boot), then
-	 * irq_kobj_base is NULL and the descriptor was never added.
-	 * kobject_del() complains about a object with no parent, so make
-	 * it conditional.
+	 * Only invoke kobject_del() when kobject_add() was successfully
+	 * invoked for the descriptor. This covers both early boot, where
+	 * sysfs is not initialized yet, and the case of a failed
+	 * kobject_add() invocation.
 	 */
-	if (irq_kobj_base)
+	if (desc->istate & IRQS_SYSFS)
 		kobject_del(&desc->kobj);
 }