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

[Thomas Gleixner <tglx@linutronix.de>]

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