genirq: procfs: Make smp_affinity read-only for interrupts marked with IRQD_AFFINITY_MANAGED flag

[PATCH] genirq: procfs: Make smp_affinity read-only for interrupts marked with IRQD_AFFINITY_MANAGED flag

Posted by Jeff Xie 1 year, 5 months ago

Currently, due to the interrupt subsystem introduced this commit 9c2555835bb3
("genirq: Introduce IRQD_AFFINITY_MANAGED flag"), an error is reported when a
system administrator modifies the smp_affinity for the virtio_blk driver.
For example:

jeff-labs:/proc/irq/26 # echo 2 > ./smp_affinity
-bash: echo: write error: Input/output error

However, checking the permissions of smp_affinity/smp_affinity_list shows that
they are set to rw. System administrators are strongly complaining about this issue.

jeff-labs:/proc/irq/26 # ls -l
total 0
-r--r--r-- 1 root root 0 Aug 20 01:32 affinity_hint
-r--r--r-- 1 root root 0 Aug 20 01:32 effective_affinity
-r--r--r-- 1 root root 0 Aug 20 01:32 effective_affinity_list
-r--r--r-- 1 root root 0 Aug 20 01:32 node
-rw-r--r-- 1 root root 0 Aug 20 01:32 smp_affinity
-rw-r--r-- 1 root root 0 Aug 20 01:32 smp_affinity_list
-r--r--r-- 1 root root 0 Aug 20 01:32 spurious
dr-xr-xr-x 2 root root 0 Aug 20 01:32 virtio3-req.0

Therefore, the permissions of smp_affinity/smp_affinity_list should be changed to read-only.

Signed-off-by: Jeff Xie <jeff.xie@linux.dev>
---
 kernel/irq/proc.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 8cccdf40725a..4a7d572b7a8c 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -363,7 +363,11 @@ void register_irq_proc(unsigned int irq, struct irq_desc *desc)
 
 #ifdef CONFIG_SMP
 	/* create /proc/irq/<irq>/smp_affinity */
-	proc_create_data("smp_affinity", 0644, desc->dir,
+	if (unlikely(irqd_affinity_is_managed(&desc->irq_data)))
+		proc_create_data("smp_affinity", 0444, desc->dir,
+			 &irq_affinity_proc_ops, irqp);
+	else
+		proc_create_data("smp_affinity", 0644, desc->dir,
 			 &irq_affinity_proc_ops, irqp);
 
 	/* create /proc/irq/<irq>/affinity_hint */
@@ -371,7 +375,11 @@ void register_irq_proc(unsigned int irq, struct irq_desc *desc)
 			irq_affinity_hint_proc_show, irqp);
 
 	/* create /proc/irq/<irq>/smp_affinity_list */
-	proc_create_data("smp_affinity_list", 0644, desc->dir,
+	if (unlikely(irqd_affinity_is_managed(&desc->irq_data)))
+		proc_create_data("smp_affinity_list", 0444, desc->dir,
+			 &irq_affinity_list_proc_ops, irqp);
+	else
+		proc_create_data("smp_affinity_list", 0644, desc->dir,
 			 &irq_affinity_list_proc_ops, irqp);
 
 	proc_create_single_data("node", 0444, desc->dir, irq_node_proc_show,
--
2.34.1

Re: [PATCH] genirq: procfs: Make smp_affinity read-only for interrupts marked with IRQD_AFFINITY_MANAGED flag

Posted by Thomas Gleixner 1 year, 5 months ago

On Tue, Aug 20 2024 at 10:09, Jeff Xie wrote:
> Currently, due to the interrupt subsystem introduced this commit 9c2555835bb3
> ("genirq: Introduce IRQD_AFFINITY_MANAGED flag"),

This is not really a proper sentence.

> an error is reported when a
> system administrator modifies the smp_affinity for the virtio_blk driver.
> For example:
>
> jeff-labs:/proc/irq/26 # echo 2 > ./smp_affinity
> -bash: echo: write error: Input/output error

That should obviously return -EPERM for managed interrupts.

> However, checking the permissions of smp_affinity/smp_affinity_list shows that
> they are set to rw. System administrators are strongly complaining about this issue.

System administrators complain strongly about a lot of things. Such
complaints are not necessarily a technical reason to change the code.

A proper reason is to argue, that the kernel already knows at the time
of interrupt allocation that the affinity cannot be controlled by
userspace and therefore creating the file with write permissions is
wrong.

> jeff-labs:/proc/irq/26 # ls -l
> total 0
> -r--r--r-- 1 root root 0 Aug 20 01:32 affinity_hint
> -r--r--r-- 1 root root 0 Aug 20 01:32 effective_affinity
> -r--r--r-- 1 root root 0 Aug 20 01:32 effective_affinity_list
> -r--r--r-- 1 root root 0 Aug 20 01:32 node
> -rw-r--r-- 1 root root 0 Aug 20 01:32 smp_affinity
> -rw-r--r-- 1 root root 0 Aug 20 01:32 smp_affinity_list
> -r--r--r-- 1 root root 0 Aug 20 01:32 spurious
> dr-xr-xr-x 2 root root 0 Aug 20 01:32 virtio3-req.0

We can see that from the code, no?

> Therefore, the permissions of smp_affinity/smp_affinity_list should be changed to read-only.

Should? Tell what the solution is:

Therefore set the file permissions to read-only for such interrupts.

And please format you change log so that it has linebreaks around 75
characters.

>  
>  #ifdef CONFIG_SMP
>  	/* create /proc/irq/<irq>/smp_affinity */
> -	proc_create_data("smp_affinity", 0644, desc->dir,
> +	if (unlikely(irqd_affinity_is_managed(&desc->irq_data)))

This unlikely is a pointless exercise as this is not a hotpath
operation. Also please switch to S_IRUGO / S_IWUSR and simplify the
whole thing to:

	umode_t umode = S_IRUGO;

	if (!irqd_affinity_is_managed(&desc->irq_data))
		umode |= S_IWUSR;
	proc_create_data("smp_affinity", umode, desc->dir, &irq_affinity_proc_ops, irqp);

Thanks,

        tglx

Re: [PATCH] genirq: procfs: Make smp_affinity read-only for interrupts marked with IRQD_AFFINITY_MANAGED flag

Posted by jeff.xie@linux.dev 1 year, 5 months ago

August 24, 2024 at 3:16 AM, "Thomas Gleixner" <tglx@linutronix.de> wrote:

Hi tglx,

Thank you for your very patient review, I’ve learned a lot from it.

> 
> On Tue, Aug 20 2024 at 10:09, Jeff Xie wrote:
> 
> > 
> > Currently, due to the interrupt subsystem introduced this commit 9c2555835bb3
> > 
> >  ("genirq: Introduce IRQD_AFFINITY_MANAGED flag"),
> > 
> 
> This is not really a proper sentence.

Thanks for pointing this out, I see. 

The introduced IRQD_AFFINITY_MANAGED is not the reason, I will delete the description.

> 
> > 
> > an error is reported when a
> > 
> >  system administrator modifies the smp_affinity for the virtio_blk driver.
> > 
> >  For example:
> > 
> >  jeff-labs:/proc/irq/26 # echo 2 > ./smp_affinity
> > 
> >  -bash: echo: write error: Input/output error
> > 
> 
> That should obviously return -EPERM for managed interrupts.'

Got it, I will fix it.

> 
> > 
> > However, checking the permissions of smp_affinity/smp_affinity_list shows that
> > 
> >  they are set to rw. System administrators are strongly complaining about this issue.
> > 
> 
> System administrators complain strongly about a lot of things. Such
> 
> complaints are not necessarily a technical reason to change the code.
> 
> A proper reason is to argue, that the kernel already knows at the time
> 
> of interrupt allocation that the affinity cannot be controlled by
> 
> userspace and therefore creating the file with write permissions is
> 
> wrong.

Thanks, I will use the description.

> 
> > 
> > jeff-labs:/proc/irq/26 # ls -l
> > 
> >  total 0
> > 
> >  -r--r--r-- 1 root root 0 Aug 20 01:32 affinity_hint
> > 
> >  -r--r--r-- 1 root root 0 Aug 20 01:32 effective_affinity
> > 
> >  -r--r--r-- 1 root root 0 Aug 20 01:32 effective_affinity_list
> > 
> >  -r--r--r-- 1 root root 0 Aug 20 01:32 node
> > 
> >  -rw-r--r-- 1 root root 0 Aug 20 01:32 smp_affinity
> > 
> >  -rw-r--r-- 1 root root 0 Aug 20 01:32 smp_affinity_list
> > 
> >  -r--r--r-- 1 root root 0 Aug 20 01:32 spurious
> > 
> >  dr-xr-xr-x 2 root root 0 Aug 20 01:32 virtio3-req.0
> > 
> 
> We can see that from the code, no?

Yes, we can see that from the code, I will delete it.

> 
> > 
> > Therefore, the permissions of smp_affinity/smp_affinity_list should be changed to read-only.
> > 
> 
> Should? Tell what the solution is:
> 
> Therefore set the file permissions to read-only for such interrupts.

Thanks, That's indeed a better description.

> 
> And please format you change log so that it has linebreaks around 75
> 
> characters.

Thanks for you reminder, I will change it.

> 
> > 
> > #ifdef CONFIG_SMP
> > 
> >  /* create /proc/irq/<irq>/smp_affinity */
> > 
> >  - proc_create_data("smp_affinity", 0644, desc->dir,
> > 
> >  + if (unlikely(irqd_affinity_is_managed(&desc->irq_data)))
> > 
> 
> This unlikely is a pointless exercise as this is not a hotpath
> 
> operation. Also please switch to S_IRUGO / S_IWUSR and simplify the
> 
> whole thing to:
> 
>  umode_t umode = S_IRUGO;
> 
>  if (!irqd_affinity_is_managed(&desc->irq_data))

Okay, I will delete the unlikely.

After thoroughly analyzing the code, I think it would be better to replace irqd_affinity_is_managed() with irq_can_set_affinity_usr() like below.  What do you think? 
 
      if (irq_can_set_affinity_usr(desc->irq_data.irq))
             umode |= S_IWUSR;

> 
>  umode |= S_IWUSR;
> 
>  proc_create_data("smp_affinity", umode, desc->dir, &irq_affinity_proc_ops, irqp);
> 
> Thanks,
> 
>  tglx
>

Re: [PATCH] genirq: procfs: Make smp_affinity read-only for interrupts marked with IRQD_AFFINITY_MANAGED flag

Posted by Thomas Gleixner 1 year, 5 months ago

On Sat, Aug 24 2024 at 14:54, jeff xie wrote:
>> This unlikely is a pointless exercise as this is not a hotpath
>> operation. Also please switch to S_IRUGO / S_IWUSR and simplify the
>> whole thing to:
>> 
>>  umode_t umode = S_IRUGO;
>> 
>>  if (!irqd_affinity_is_managed(&desc->irq_data))
>
> Okay, I will delete the unlikely.
>
> After thoroughly analyzing the code, I think it would be better to
> replace irqd_affinity_is_managed() with irq_can_set_affinity_usr()
> like below.  What do you think?
>  
>       if (irq_can_set_affinity_usr(desc->irq_data.irq))
>              umode |= S_IWUSR;

Makes sense