KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	cohuck@redhat.com, jgg@ziepe.ca, cai@lca.pw
Subject: Re: [PATCH v3 3/3] vfio-pci: Invalidate mmaps and block MMIO access on disabled memory
Date: Sat, 23 May 2020 17:06:02 -0600
Message-ID: <20200523170602.5eb09a66@x1.home> (raw)
In-Reply-To: <20200523193417.GI766834@xz-x1>

On Sat, 23 May 2020 15:34:17 -0400
Peter Xu <peterx@redhat.com> wrote:

> Hi, Alex,
> 
> On Fri, May 22, 2020 at 01:17:43PM -0600, Alex Williamson wrote:
> > @@ -1346,15 +1526,32 @@ static vm_fault_t vfio_pci_mmap_fault(struct vm_fault *vmf)
> >  {
> >  	struct vm_area_struct *vma = vmf->vma;
> >  	struct vfio_pci_device *vdev = vma->vm_private_data;
> > +	vm_fault_t ret = VM_FAULT_NOPAGE;
> > +
> > +	mutex_lock(&vdev->vma_lock);
> > +	down_read(&vdev->memory_lock);  
> 
> I remembered to have seen the fault() handling FAULT_FLAG_RETRY_NOWAIT at least
> in the very first version, but it's not here any more...  Could I ask what's
> the reason behind?  I probably have missed something along with the versions,
> I'm just not sure whether e.g. this would potentially block a GUP caller even
> if it's with FOLL_NOWAIT.

This is largely what v2 was about, from the cover letter:

    Locking in 3/ is substantially changed to avoid the retry scenario
    within the fault handler, therefore a caller who does not allow
    retry will no longer receive a SIGBUS on contention.

The discussion thread starts here:

https://lore.kernel.org/kvm/20200501234849.GQ26002@ziepe.ca/

Feel free to interject if there's something that doesn't make sense,
the idea is that since we've fixed the lock ordering we never need to
release one lock to wait for another, therefore we can wait for the
lock.  I'm under the impression that we can wait for the lock
regardless of the flags under these conditions.

> Side note: Another thing I thought about when reading this patch - there seems
> to have quite some possibility that the VFIO_DEVICE_PCI_HOT_RESET ioctl will
> start to return -EBUSY now.  Not a problem for this series, but maybe we should
> rememeber to let the userspace handle -EBUSY properly as follow up too, since I
> saw QEMU seemed to not handle -EBUSY for host reset path right now.

I think this has always been the case, whether it's the device lock or
this lock, the only way I know to avoid potential deadlock is to use
the 'try' locking semantics.  In normal scenarios I expect access to
sibling devices is quiesced at this point, so a user driver actually
wanting to achieve a reset shouldn't be affected by this.  Thanks,

Alex


  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-22 19:17 [PATCH v3 0/3] vfio-pci: Block user access to disabled device MMIO Alex Williamson
2020-05-22 19:17 ` [PATCH v3 1/3] vfio/type1: Support faulting PFNMAP vmas Alex Williamson
2020-05-22 19:17 ` [PATCH v3 2/3] vfio-pci: Fault mmaps to enable vma tracking Alex Williamson
2020-05-22 19:17 ` [PATCH v3 3/3] vfio-pci: Invalidate mmaps and block MMIO access on disabled memory Alex Williamson
2020-05-23 19:34   ` Peter Xu
2020-05-23 23:06     ` Alex Williamson [this message]
2020-05-23 23:52       ` Peter Xu
2020-05-24  0:02         ` Peter Xu
2020-05-25 12:26         ` Jason Gunthorpe
2020-05-25 14:28           ` Peter Xu
2020-05-25 14:46             ` Jason Gunthorpe
2020-05-25 15:11               ` Peter Xu
2020-05-25 16:56                 ` Jason Gunthorpe
2020-05-25 20:56                   ` John Hubbard
2020-05-26  0:37                     ` Jason Gunthorpe
2020-05-26  0:46                       ` John Hubbard
2020-05-26 13:49                       ` Peter Xu
2020-05-26 14:32                         ` Alex Williamson
2020-05-26 14:46                           ` Peter Xu
2020-05-26 15:53                           ` Jason Gunthorpe
2020-05-26 15:57                             ` Alex Williamson
2020-05-22 22:08 ` [PATCH v3 0/3] vfio-pci: Block user access to disabled device MMIO Qian Cai
2020-05-22 22:25   ` Alex Williamson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200523170602.5eb09a66@x1.home \
    --to=alex.williamson@redhat.com \
    --cc=cai@lca.pw \
    --cc=cohuck@redhat.com \
    --cc=jgg@ziepe.ca \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterx@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git