linux-kernel - Re: [PATCH] vfio/pci: add msi interrupt affinity support

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] [day] [month] [year] [list]

Message-ID: <8b08098a38fc6c6351d7f978791273d0e14b831d.camel@infradead.org>
Date: Mon, 03 Jun 2024 10:32:47 +0100
From: David Woodhouse <dwmw2@...radead.org>
To: Fred Griffoul <fgriffo@...zon.co.uk>
Cc: Alex Williamson <alex.williamson@...hat.com>, Kevin Tian
 <kevin.tian@...el.com>, Reinette Chatre <reinette.chatre@...el.com>, Eric
 Auger <eric.auger@...hat.com>, Christian Brauner <brauner@...nel.org>, Ye
 Bin <yebin10@...wei.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vfio/pci: add msi interrupt affinity support

On Thu, 2024-05-30 at 16:12 +0000, Fred Griffoul wrote:
> The usual way to configure a device interrupt from userland is to write
> the /proc/irq/<irq>/smp_affinity or smp_affinity_list files. When using
> vfio to implement a device driver or a virtual machine monitor, this may
> not be ideal: the process managing the vfio device interrupts may not be
> granted root privilege, for security reasons. Thus it cannot directly
> control the interrupt affinity and has to rely on an external command.
> 
> This patch extends the VFIO_DEVICE_SET_IRQS ioctl() with a new data flag
> to specify the affinity of a vfio pci device interrupt.
> 
> The affinity argument must be a subset of the process cpuset, otherwise
> an error -EPERM is returned.
> 
> The vfio_irq_set argument shall be set-up in the following way:
> 
> - the 'flags' field have the new flag VFIO_IRQ_SET_DATA_AFFINITY set
> as well as VFIO_IRQ_SET_ACTION_TRIGGER.
> 
> - the 'start' field is the device interrupt index. Only one interrupt
> can be configured per ioctl().
> 
> - the variable-length array consists of one or more CPU index
> encoded as __u32, the number of entries in the array is specified in the
> 'count' field.
> 
> Signed-off-by: Fred Griffoul <fgriffo@...zon.co.uk>

LGTM in general. This is important to avoid a noisy neighbour effect
when guests are supposed to be pinned to their *own* set of pCPUs.

I still think I'd prefer the cpumask to be a bitmask like in the
sys_sched_setaffinity() syscall. Even if we end up making the
get_user_cpu_mask() function non-static (isn't there already a second
open-coded version of that somewhere?). But I don't particularly mind. 

I *do* mind abusing 'count' though, and the various other changes that
follow from doing so. Could we put the CPU count into the 'data' field
itself? Or infer it from 'argsz'? We already explicitly calculate
data_size in order to memdup_user(). Let's just pass it down.


Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)