linux-kernel - Re: [PATCH v3] vfio: fix potential deadlock on vfio group lock

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]

Date:   Fri, 13 Jan 2023 15:09:01 -0500
From:   Matthew Rosato <mjrosato@...ux.ibm.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     alex.williamson@...hat.com, pbonzini@...hat.com, cohuck@...hat.com,
        farman@...ux.ibm.com, pmorel@...ux.ibm.com,
        borntraeger@...ux.ibm.com, frankja@...ux.ibm.com,
        imbrenda@...ux.ibm.com, david@...hat.com, akrowiak@...ux.ibm.com,
        jjherne@...ux.ibm.com, pasic@...ux.ibm.com,
        zhenyuw@...ux.intel.com, zhi.a.wang@...el.com, seanjc@...gle.com,
        linux-s390@...r.kernel.org, kvm@...r.kernel.org,
        intel-gvt-dev@...ts.freedesktop.org,
        intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] vfio: fix potential deadlock on vfio group lock

On 1/13/23 1:52 PM, Jason Gunthorpe wrote:
> On Fri, Jan 13, 2023 at 12:11:32PM -0500, Matthew Rosato wrote:
>> @@ -462,9 +520,19 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
>>  static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>>  {
>>  	struct vfio_device *device = filep->private_data;
>> +	struct kvm *kvm = NULL;
>>  
>>  	vfio_device_group_close(device);
>>  
>> +	mutex_lock(&device->dev_set->lock);
>> +	if (device->open_count == 0 && device->kvm) {
>> +		kvm = device->kvm;
>> +		device->kvm = NULL;
>> +	}
>> +	mutex_unlock(&device->dev_set->lock);
> 
> This still doesn't seem right, another thread could have incr'd the
> open_count already 
> 
> This has to be done at the moment open_count is decremented to zero,
> while still under the original lock.

Hmm..  Fair.  Well, we can go back to clearing of device->kvm in vfio_device_last_close but the group lock is held then so we can't immediately do the kvm_put at that time -- unless we go back to the notion of stacking the kvm_put on a workqueue, but now from vfio.  If we do that, I think we also have to scrap the idea of putting the kvm_put_kvm function pointer into device->put_kvm too (or otherwise stash it along with the kvm value to be picked up by the scheduled work).

Another thought would be passing the device->open_count that was read while holding the dev_set->lock back on vfio_close_device() / vfio_device_group_close() as an indicator of whether vfio_device_last_close() was called - then you could use the stashed kvm value because it doesn't matter what's currently in device->kvm or what the current device->open_count is, you know that kvm reference needs to be put.

e.g.:
struct *kvm = device->kvm;
void (*put)(struct kvm *kvm) = device->put_kvm;
opened = vfio_device_group_close(device);
if (opened == 0 && kvm)
	put(kvm);