Re: [PATCH v2] vfio: Use file-based reference counting for KVM

[Steffen Eiden <seiden@linux.ibm.com>]

On Thu, Sep 03, 2026 at 10:07:36AM -0700, Sean Christopherson wrote:
> On Thu, Sep 03, 2026, sashiko-bot@kernel.org wrote:
> > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> > > index 940c0ff668bee..f160e1945fbce 100644
> > > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > 
> > [ ... ]
> > 
> > > @@ -1822,17 +1822,27 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
> > >  
> > >  /**
> > >   * vfio_ap_mdev_set_kvm - sets all data for @matrix_mdev that are needed
> > > - * to manage AP resources for the guest whose state is represented by @kvm
> > > + * to manage AP resources for the guest whose state is represented by
> > > + * @kvm_file
> > >   *
> > >   * @matrix_mdev: a mediated matrix device
> > > - * @kvm: reference to KVM instance
> > > + * @kvm_file: the KVM VM file this vfio device is associated with
> > >   *
> > > - * Return: 0 if no other mediated matrix device has a reference to @kvm;
> > > + * Return: 0 if no other mediated matrix device has a reference to the VM;
> > >   * otherwise, returns an -EPERM.
> > >   */
> > >  static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
> > > -				struct kvm *kvm)
> > > +				struct file *kvm_file)
> > >  {
> > > +	struct kvm *kvm;
> > > +
> > > +	if (!kvm_file)
> > > +		return -ENOENT;
> > > +
> > > +	kvm = kvm_file->private_data;
> > > +	if (!kvm)
> > > +		return -ENOENT;
> > > +
> > >  	if (kvm->arch.crypto.crycbd) {
> > 
> > [Severity: Critical]
> > Does this code adequately verify that the file is an s390 KVM file before
> > casting its private_data in vfio_ap_mdev_set_kvm() inside
> > drivers/s390/crypto/vfio_ap_ops.c?
> > 
> > If a user instantiates a VM using a concurrent KVM-arm64 module and attaches
> > an s390-specific VFIO device to its KVM device, could this driver extract the
> > untyped private_data and mistakenly treat an arm64 struct kvm as an s390 one?
> > 
> > Because the memory layouts of struct kvm_arch differ across architectures,
> > would accessing s390-specific fields in the arch member (like
> > kvm->arch.crypto.pqap_hook) result in arbitrary memory corruption?
> 
> This is effectively the same concern I raised[1] in the s390+arm64 series:
> 
>  : Side topic #2, this entire approach seems extremely brittle unless you make it
>  : all but impossible for non-KVM code to get at KVM structure definitions.  Outside
>  : of KVM, all compilation units will see the s390 version of KVM structures.  Which
>  : is "fine", but obviously dangerous and IMO asking for maintenance issues down the
>  : road.
> 
> I don't think we need to go to the super extreme lengths I proposed[2] back when
> we were exploring multi-KVM on x86, but the direct dereference of ->private_data
> is a huge red flag.
> 
> Given that external usage of "struct kvm" should be *super* rare, and IMO is
> something we should actively discourage, I think we should make it opt-in.  Then
> at the same time, define the API so that it's arch-specific, e.g. to yield
> file_to_kvm_s390() so that drivers/s390/crypto/vfio_ap_ops.c can get exactly
> what it wants.
> 

yes, I was not happy with the ->private_data either. Thanks for finding
a solution. Simple is good. I only came up overly complex solutions I
did not like :) I will incoporate this. 

> Diff below, though it needs to be split into multiple patches (I'll respond with
> more to the full patch).

Amazing, thanks!

	Steffen