[PATCH] genirq/debugfs: add new config option for trigger interrupt from userspace

[PATCH] genirq/debugfs: add new config option for trigger interrupt from userspace

[Joe Jin]

commit 536e2e34bd00 ("genirq/debugfs: Triggering of interrupts from
userspace") is allowed developer inject interrupts via irq debugfs, which
is very useful during development phase, but on a production system, this
is very dangerous, add new config option, developers can enable it as
needed instead of enabling it by default when irq debugfs is enabled.

Signed-off-by: Joe Jin <joe.jin@oracle.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
---
 kernel/irq/Kconfig   | 15 +++++++++++++++
 kernel/irq/debugfs.c |  9 +++++++++
 2 files changed, 24 insertions(+)

diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index f92d9a687372..00521e896e6c 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -135,6 +135,21 @@ config GENERIC_IRQ_DEBUGFS
 
 	  If you don't know what to do here, say N.
 
+config GENERIC_IRQ_DEBUGFS_INJECT_INT
+	bool "Allow trigger interrupts from userspace"
+	depends on DEBUG_FS && GENERIC_IRQ_DEBUGFS
+	default n
+	---help---
+
+	  Allow developers retrigger a (edge-only) interrupt from userspace.
+	  To use this feature:
+
+	  echo -n trigger > /sys/kernel/debug/irq/irqs/IRQNUM
+
+	  WARNING: This is DANGEROUS, and strictly a debug feature.
+
+	  If you don't know what to do here, say N.
+
 endmenu
 
 config GENERIC_IRQ_MULTI_HANDLER
diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index a949bd39e343..56db29bc1acf 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -178,6 +178,7 @@ static int irq_debug_open(struct inode *inode, struct file *file)
 	return single_open(file, irq_debug_show, inode->i_private);
 }
 
+#ifdef CONFIG_GENERIC_IRQ_DEBUGFS_INJECT_INT
 static ssize_t irq_debug_write(struct file *file, const char __user *user_buf,
 			       size_t count, loff_t *ppos)
 {
@@ -223,10 +224,13 @@ static ssize_t irq_debug_write(struct file *file, const char __user *user_buf,
 
 	return count;
 }
+#endif
 
 static const struct file_operations dfs_irq_ops = {
 	.open		= irq_debug_open,
+#ifdef CONFIG_GENERIC_IRQ_DEBUGFS_INJECT_INT
 	.write		= irq_debug_write,
+#endif
 	.read		= seq_read,
 	.llseek		= seq_lseek,
 	.release	= single_release,
@@ -249,8 +253,13 @@ void irq_add_debugfs_entry(unsigned int irq, struct irq_desc *desc)
 		return;
 
 	sprintf(name, "%d", irq);
+#ifdef CONFIG_GENERIC_IRQ_DEBUGFS_INJECT_INT
 	desc->debugfs_file = debugfs_create_file(name, 0644, irq_dir, desc,
 						 &dfs_irq_ops);
+#else
+	desc->debugfs_file = debugfs_create_file(name, 0444, irq_dir, desc,
+						 &dfs_irq_ops);
+#endif
 }
 
 static int __init irq_debugfs_init(void)
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH] genirq/debugfs: add new config option for trigger interrupt from userspace

[Marc Zyngier]

Hi Joe,

On 2020-02-28 05:42, Joe Jin wrote:
> commit 536e2e34bd00 ("genirq/debugfs: Triggering of interrupts from
> userspace") is allowed developer inject interrupts via irq debugfs, 
> which
> is very useful during development phase, but on a production system, 
> this
> is very dangerous, add new config option, developers can enable it as
> needed instead of enabling it by default when irq debugfs is enabled.

I don't really mind the patch (although it could be more elegant), but 
in
general I object to most debugfs options being set on a production 
kernel.
There is way too much information in most debugfs backends to be 
comfortable
with it, and you can find things like page table dumps, for example...

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] genirq/debugfs: add new config option for trigger interrupt from userspace

[Joe Jin]

Hi Marc,

Thanks for your reply.

On 2/28/20 8:37 AM, Marc Zyngier wrote:
> On 2020-02-28 05:42, Joe Jin wrote:
>> commit 536e2e34bd00 ("genirq/debugfs: Triggering of interrupts from
>> userspace") is allowed developer inject interrupts via irq debugfs, which
>> is very useful during development phase, but on a production system, this
>> is very dangerous, add new config option, developers can enable it as
>> needed instead of enabling it by default when irq debugfs is enabled.
> 
> I don't really mind the patch (although it could be more elegant), but in
> general I object to most debugfs options being set on a production kernel.
> There is way too much information in most debugfs backends to be comfortable
> with it, and you can find things like page table dumps, for example...

We should not enable any debug option on production system, actual customer
want to resize their BM or VM, cpu offline may lead system not works properly,
and if we knew more details of IRQ info it will be very help to identify 
if it caused by IRQ/vectors, this is the motivation of we want to enable it
on production kernel.

Thanks,
Joe

Re: [PATCH] genirq/debugfs: add new config option for trigger interrupt from userspace

[Marc Zyngier]

On 2020-02-28 19:13, Joe Jin wrote:
> Hi Marc,
> 
> Thanks for your reply.
> 
> On 2/28/20 8:37 AM, Marc Zyngier wrote:
>> On 2020-02-28 05:42, Joe Jin wrote:
>>> commit 536e2e34bd00 ("genirq/debugfs: Triggering of interrupts from
>>> userspace") is allowed developer inject interrupts via irq debugfs, 
>>> which
>>> is very useful during development phase, but on a production system, 
>>> this
>>> is very dangerous, add new config option, developers can enable it as
>>> needed instead of enabling it by default when irq debugfs is enabled.
>> 
>> I don't really mind the patch (although it could be more elegant), but 
>> in
>> general I object to most debugfs options being set on a production 
>> kernel.
>> There is way too much information in most debugfs backends to be 
>> comfortable
>> with it, and you can find things like page table dumps, for example...
> 
> We should not enable any debug option on production system, actual 
> customer
> want to resize their BM or VM, cpu offline may lead system not works 
> properly,
> and if we knew more details of IRQ info it will be very help to 
> identify
> if it caused by IRQ/vectors, this is the motivation of we want to 
> enable it
> on production kernel.

If something doesn't work properly, then you are still debugging, 
AFAICT.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] genirq/debugfs: add new config option for trigger interrupt from userspace

[Joe Jin]

On 2/28/20 11:54 AM, Marc Zyngier wrote:
> On 2020-02-28 19:13, Joe Jin wrote:
>> Hi Marc,
>>
>> Thanks for your reply.
>>
>> On 2/28/20 8:37 AM, Marc Zyngier wrote:
>>> On 2020-02-28 05:42, Joe Jin wrote:
>>>> commit 536e2e34bd00 ("genirq/debugfs: Triggering of interrupts from
>>>> userspace") is allowed developer inject interrupts via irq debugfs, which
>>>> is very useful during development phase, but on a production system, this
>>>> is very dangerous, add new config option, developers can enable it as
>>>> needed instead of enabling it by default when irq debugfs is enabled.
>>>
>>> I don't really mind the patch (although it could be more elegant), but in
>>> general I object to most debugfs options being set on a production kernel.
>>> There is way too much information in most debugfs backends to be comfortable
>>> with it, and you can find things like page table dumps, for example...
>>
>> We should not enable any debug option on production system, actual customer
>> want to resize their BM or VM, cpu offline may lead system not works properly,
>> and if we knew more details of IRQ info it will be very help to identify
>> if it caused by IRQ/vectors, this is the motivation of we want to enable it
>> on production kernel.
> 
> If something doesn't work properly, then you are still debugging, AFAICT.

Yes you're right, there are various reasons to led the problem such like
bad mapping, interrupts lost, vectors used up .... irq debugfs is help to
identify which part caused the problem, and it help to save troubleshooting
time :-)

Thanks,
Joe

Re: [PATCH] genirq/debugfs: add new config option for trigger interrupt from userspace

[Thomas Gleixner]

Joe,

Joe Jin <joe.jin@oracle.com> writes:
> On 2/28/20 11:54 AM, Marc Zyngier wrote:
>> On 2020-02-28 19:13, Joe Jin wrote:
>>>> On 2020-02-28 05:42, Joe Jin wrote:
>>>>> commit 536e2e34bd00 ("genirq/debugfs: Triggering of interrupts from
>>>>> userspace") is allowed developer inject interrupts via irq debugfs, which
>>>>> is very useful during development phase, but on a production system, this
>>>>> is very dangerous, add new config option, developers can enable it as
>>>>> needed instead of enabling it by default when irq debugfs is enabled.
>>>>
>>>> I don't really mind the patch (although it could be more elegant), but in
>>>> general I object to most debugfs options being set on a production kernel.
>>>> There is way too much information in most debugfs backends to be comfortable
>>>> with it, and you can find things like page table dumps, for example...
>>>
>>> We should not enable any debug option on production system, actual customer
>>> want to resize their BM or VM, cpu offline may lead system not works properly,
>>> and if we knew more details of IRQ info it will be very help to identify
>>> if it caused by IRQ/vectors, this is the motivation of we want to enable it
>>> on production kernel.
>> 
>> If something doesn't work properly, then you are still debugging, AFAICT.
>
> Yes you're right, there are various reasons to led the problem such like
> bad mapping, interrupts lost, vectors used up .... irq debugfs is help to
> identify which part caused the problem, and it help to save troubleshooting
> time :-)

Just picking out individual interfaces is going to create a whack a mole
game and a boat load of config options to disable these. That's really
not the way to go.

The right thing to do is to have ONE config switch which disables all
write interfaces at once by refusing the write right in the debugfs
entry point. 

Of course there might be a few debugfs files which need write access
even in that case, but that's easy enough to fix by marking them as
explicitely allowed.

Thanks,

        tglx