linux-kernel - Re: [PATCH] genirq: procfs: Make smp_affinity read-only for interrupts marked with IRQD_AFFINITY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f2e282ae3c502561d8ae302f535d969250dd967e@linux.dev>
Date: Sat, 24 Aug 2024 14:54:46 +0000
From: jeff.xie@...ux.dev
To: "Thomas Gleixner" <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org, xiehuan09@...il.com
Subject: Re: [PATCH] genirq: procfs: Make smp_affinity read-only for
 interrupts marked with IRQD_AFFINITY_MANAGED flag

August 24, 2024 at 3:16 AM, "Thomas Gleixner" <tglx@...utronix.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
>