[PATCH] xen/privcmd: Only claim ioreqs matching an ioeventfd

[PATCH] xen/privcmd: Only claim ioreqs matching an ioeventfd

[Koichiro Den]

The ioeventfd handler shares its event channel with the userspace device
model. It must only claim requests that match a registered ioeventfd.
Other requests are left for userspace.

ioeventfd_interrupt() currently marks every MMIO write INPROCESS before
looking for a match. For an unmatched request it drops the lock and
restores READY. Userspace may see INPROCESS and skip it. Restoring READY
does not send another notification. If userspace handles the request at
the same time, the READY store may overwrite its state.

Find a matching ioeventfd before changing the request state.

The lock now protects only the ioeventfd list. Use explicit virt_*mb()
barriers for the ioreq shared with Xen.

Fixes: f0d7db7b3324 ("xen: privcmd: Add support for ioeventfd")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/xen/privcmd.c | 47 +++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 29 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 7cfc28f1bb86..c67ec6d282cf 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -1169,51 +1169,40 @@ static irqreturn_t ioeventfd_interrupt(int irq, void *dev_id)
 	struct privcmd_kernel_ioreq *kioreq = port->kioreq;
 	struct ioreq *ioreq = &kioreq->ioreq[port->vcpu];
 	struct privcmd_kernel_ioeventfd *kioeventfd;
-	unsigned int state = STATE_IOREQ_READY;
+	bool matched = false;
 
-	if (ioreq->state != STATE_IOREQ_READY ||
-	    ioreq->type != IOREQ_TYPE_COPY || ioreq->dir != IOREQ_WRITE)
+	if (ioreq->state != STATE_IOREQ_READY)
 		return IRQ_NONE;
 
-	/*
-	 * We need a barrier, smp_mb(), here to ensure reads are finished before
-	 * `state` is updated. Since the lock implementation ensures that
-	 * appropriate barrier will be added anyway, we can avoid adding
-	 * explicit barrier here.
-	 *
-	 * Ideally we don't need to update `state` within the locks, but we do
-	 * that here to avoid adding explicit barrier.
-	 */
+	/* Xen publishes the request before changing its state to READY. */
+	virt_rmb();
 
-	spin_lock(&kioreq->lock);
-	ioreq->state = STATE_IOREQ_INPROCESS;
+	if (ioreq->type != IOREQ_TYPE_COPY || ioreq->dir != IOREQ_WRITE)
+		return IRQ_NONE;
 
+	spin_lock(&kioreq->lock);
 	list_for_each_entry(kioeventfd, &kioreq->ioeventfds, list) {
 		if (ioreq->addr == kioeventfd->addr + VIRTIO_MMIO_QUEUE_NOTIFY &&
 		    ioreq->size == kioeventfd->addr_len &&
 		    (ioreq->data & QUEUE_NOTIFY_VQ_MASK) == kioeventfd->vq) {
+			/* Finish reading the request before claiming it. */
+			virt_mb();
+			ioreq->state = STATE_IOREQ_INPROCESS;
 			eventfd_signal(kioeventfd->eventfd);
-			state = STATE_IORESP_READY;
+			matched = true;
 			break;
 		}
 	}
 	spin_unlock(&kioreq->lock);
 
-	/*
-	 * We need a barrier, smp_mb(), here to ensure writes are finished
-	 * before `state` is updated. Since the lock implementation ensures that
-	 * appropriate barrier will be added anyway, we can avoid adding
-	 * explicit barrier here.
-	 */
-
-	ioreq->state = state;
-
-	if (state == STATE_IORESP_READY) {
-		notify_remote_via_evtchn(port->port);
-		return IRQ_HANDLED;
-	}
+	if (!matched)
+		return IRQ_NONE;
 
-	return IRQ_NONE;
+	/* Publish the response only after signaling the eventfd. */
+	virt_wmb();
+	ioreq->state = STATE_IORESP_READY;
+	notify_remote_via_evtchn(port->port);
+	return IRQ_HANDLED;
 }
 
 static void ioreq_free(struct privcmd_kernel_ioreq *kioreq)

base-commit: d330fb86a7170f845123ae82d95df440fad9b707
-- 
2.51.0

Re: [PATCH] xen/privcmd: Only claim ioreqs matching an ioeventfd

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 993 bytes --]

On 21.08.26 10:41, Koichiro Den wrote:
> The ioeventfd handler shares its event channel with the userspace device
> model. It must only claim requests that match a registered ioeventfd.
> Other requests are left for userspace.
> 
> ioeventfd_interrupt() currently marks every MMIO write INPROCESS before
> looking for a match. For an unmatched request it drops the lock and
> restores READY. Userspace may see INPROCESS and skip it. Restoring READY
> does not send another notification. If userspace handles the request at
> the same time, the READY store may overwrite its state.
> 
> Find a matching ioeventfd before changing the request state.
> 
> The lock now protects only the ioeventfd list. Use explicit virt_*mb()
> barriers for the ioreq shared with Xen.
> 
> Fixes: f0d7db7b3324 ("xen: privcmd: Add support for ioeventfd")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]