Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 13 Jan 2023 02:05:14 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Alex Williamson <alex.williamson@redhat.com>
Cc:     Matthew Rosato <mjrosato@linux.ibm.com>, pbonzini@redhat.com,
        jgg@nvidia.com, cohuck@redhat.com, farman@linux.ibm.com,
        pmorel@linux.ibm.com, borntraeger@linux.ibm.com,
        frankja@linux.ibm.com, imbrenda@linux.ibm.com, david@redhat.com,
        akrowiak@linux.ibm.com, jjherne@linux.ibm.com, pasic@linux.ibm.com,
        zhenyuw@linux.intel.com, zhi.a.wang@intel.com,
        linux-s390@vger.kernel.org, kvm@vger.kernel.org,
        intel-gvt-dev@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] vfio: fix potential deadlock on vfio group lock
Message-ID: <Y8C8WpkjgFelldTQ@google.com>
References: <20230112203844.41179-1-mjrosato@linux.ibm.com>
 <20230112140517.6db5b346.alex.williamson@redhat.com>
 <bce7912a-f904-b5a3-d234-c3e2c42d9e54@linux.ibm.com>
 <Y8CX8YwT/T9v4U/D@google.com>
 <20230112175648.158dca5f.alex.williamson@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230112175648.158dca5f.alex.williamson@redhat.com>
Precedence: bulk

On Thu, Jan 12, 2023, Alex Williamson wrote:
> On Thu, 12 Jan 2023 23:29:53 +0000
> Sean Christopherson <seanjc@google.com> wrote:
> 
> > On Thu, Jan 12, 2023, Matthew Rosato wrote:
> > > On 1/12/23 4:05 PM, Alex Williamson wrote:  
> > > > On Thu, 12 Jan 2023 15:38:44 -0500
> > > > Matthew Rosato <mjrosato@linux.ibm.com> wrote:  
> > > >> @@ -344,6 +345,35 @@ static bool vfio_assert_device_open(struct vfio_device *device)
> > > >>  	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
> > > >>  }
> > > >>  
> > > >> +static bool vfio_kvm_get_kvm_safe(struct kvm *kvm)
> > > >> +{
> > > >> +	bool (*fn)(struct kvm *kvm);
> > > >> +	bool ret;
> > > >> +
> > > >> +	fn = symbol_get(kvm_get_kvm_safe);
> > > >> +	if (WARN_ON(!fn))  
> > 
> > In a related vein to Alex's comments about error handling, this should not WARN.
> > WARNing during vfio_kvm_put_kvm() makes sense, but the "get" is somewhat blind.
> 
> It's not exactly blind though, we wouldn't have a kvm pointer if the
> kvm-vfio device hadn't stuffed one into the group.  We only call into
> here if we have a non-NULL pointer, so it wouldn't simply be that the
> kvm module isn't available for this to fire, but more that we have an
> API change to make the symbol no longer exist.  A WARN for that doesn't
> seem unreasonable.  Thanks,

Hmm, I was thinking that it might be possible for kvm.ko to be on its way out,
but barring use of force module unload, which breaks things left and right, kvm.ko
can only be going if all VMs have been destroyed.

Agreed, WARN is fine.