[RFC PATCH 08/19] vfio/pci: Serialize BAR and ROM access with recovery

[Shameer Kolothum]

Hold recovery_lock for reading around trapped BAR reads and writes, and
around the ROM mapping, so they do not run while host recovery has access
blocked.

The lock is taken inside the width-specific I/O helpers, one access at a
time, rather than across the whole transfer. copy_to_user() and
copy_from_user() run in the callers of those helpers and so stay outside
it. A user buffer can fault, and with userfaultfd the fault is serviced
by userspace, so holding recovery_lock across the copy would let a user
stall error_detected() for as long as it likes.

VFIO_DEVICE_GET_REGION_INFO probes the ROM the same way, enabling memory
decode and mapping it to see whether the contents are valid, so guard that
too.

Mapping and unmapping the ROM both write config space: pci_map_rom()
enables decode, and assigns the resource first if it has none, and
pci_unmap_rom() disables it again. If recovery has blocked access, do the
iounmap and record the disable in pci_recovery_rom_disable instead.
recovery_lock is held across the decision and the record so recovery
cannot complete in between.

Do the recorded disable from vfio_pci_try_reset_function() once the reset
has finished, and from the resume() handler a later patch adds. Both are
points where whatever blocked access has ended. A closed device is
skipped, since close puts the device back through reset and config
restore without holding recovery_lock.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@xxxxxxxxxx>
---
 drivers/vfio/pci/vfio_pci_core.c | 32 +++++++++++++++++++-
 drivers/vfio/pci/vfio_pci_rdwr.c | 51 +++++++++++++++++++++++++++++++-
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index d46448662e84..0b1b2398dc88 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1244,6 +1244,9 @@ int vfio_pci_ioctl_get_region_info(struct vfio_device *core_vdev,
 			 * Check ROM content is valid. Need to enable memory
 			 * decode for ROM access in pci_map_rom().
 			 */
+			ret = vfio_pci_core_access_begin(vdev);
+			if (ret)
+				return ret;
 			cmd = vfio_pci_memory_lock_and_enable(vdev);
 			io = pci_map_rom(pdev, &size);
 			if (io) {
@@ -1254,6 +1257,7 @@ int vfio_pci_ioctl_get_region_info(struct vfio_device *core_vdev,
 				pci_unmap_rom(pdev, io);
 			}
 			vfio_pci_memory_unlock_and_restore(vdev, cmd);
+			vfio_pci_core_access_end(vdev);
 		} else if (pdev->rom && pdev->romlen) {
 			info->flags = VFIO_REGION_INFO_FLAG_READ;
 			/* Report BAR size as power of two. */
@@ -1379,6 +1383,30 @@ static int vfio_pci_ioctl_set_irqs(struct vfio_pci_core_device *vdev,
 	return ret;
 }
 
+/*
+ * Complete a ROM unmap which could not disable decode through config space.
+ * Call once whatever blocked access has finished. A closed device is skipped.
+ * It runs without recovery_lock, and close puts the device back through reset
+ * and config restore.
+ *
+ * The IORESOURCE_ROM_ENABLE test is what pci_unmap_rom() would have done.
+ * A ROM which firmware left enabled is not ours to turn off.
+ */
+static void vfio_pci_recovery_rom_disable(struct vfio_pci_core_device *vdev)
+{
+	struct pci_dev *pdev = vdev->pdev;
+
+	lockdep_assert_held_write(&vdev->recovery_lock);
+
+	if (!vdev->pci_recovery_device_open ||
+	    !READ_ONCE(vdev->pci_recovery_rom_disable))
+		return;
+
+	if (!(pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_ENABLE))
+		pci_disable_rom(pdev);
+	WRITE_ONCE(vdev->pci_recovery_rom_disable, false);
+}
+
 int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
 				bool reset_power_state)
 {
@@ -1455,8 +1483,10 @@ int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
 		 */
 		if (vdev->pci_recovery_device_open &&
 		    !(vdev->pci_recovery_flags & (VFIO_PCI_RECOVERY_IN_PROGRESS |
-						  VFIO_PCI_RECOVERY_FAILED)))
+						  VFIO_PCI_RECOVERY_FAILED))) {
+			vfio_pci_recovery_rom_disable(vdev);
 			WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
+		}
 		up_write(&vdev->recovery_lock);
 		/*
 		 * Access is blocked for the length of the reset, so anything
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 20362e2f0166..86fadc999962 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -42,10 +42,17 @@
 int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
 			bool test_mem, u##size val, void __iomem *io)	\
 {									\
+	int ret;							\
+									\
+	ret = vfio_pci_core_access_begin(vdev);				\
+	if (ret)							\
+		return ret;						\
+									\
 	if (test_mem) {							\
 		down_read(&vdev->memory_lock);				\
 		if (!__vfio_pci_memory_enabled(vdev)) {			\
 			up_read(&vdev->memory_lock);			\
+			vfio_pci_core_access_end(vdev);			\
 			return -EIO;					\
 		}							\
 	}								\
@@ -54,6 +61,7 @@ int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
 									\
 	if (test_mem)							\
 		up_read(&vdev->memory_lock);				\
+	vfio_pci_core_access_end(vdev);					\
 									\
 	return 0;							\
 }									\
@@ -68,10 +76,17 @@ VFIO_IOWRITE(64)
 int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
 			bool test_mem, u##size *val, void __iomem *io)	\
 {									\
+	int ret;							\
+									\
+	ret = vfio_pci_core_access_begin(vdev);				\
+	if (ret)							\
+		return ret;						\
+									\
 	if (test_mem) {							\
 		down_read(&vdev->memory_lock);				\
 		if (!__vfio_pci_memory_enabled(vdev)) {			\
 			up_read(&vdev->memory_lock);			\
+			vfio_pci_core_access_end(vdev);			\
 			return -EIO;					\
 		}							\
 	}								\
@@ -80,6 +95,7 @@ int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
 									\
 	if (test_mem)							\
 		up_read(&vdev->memory_lock);				\
+	vfio_pci_core_access_end(vdev);					\
 									\
 	return 0;							\
 }									\
@@ -198,12 +214,41 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
 
+/*
+ * Undo pci_map_rom(). The iounmap is always safe, but pci_disable_rom() is a
+ * config space write. If recovery has blocked access, do the iounmap now and
+ * record the disable, for whichever of resume() or the reset tail unblocks
+ * access again. recovery_lock spans the decision and the record so recovery
+ * cannot complete in between.
+ */
+static void vfio_pci_unmap_rom(struct vfio_pci_core_device *vdev,
+			       void __iomem *io)
+{
+	struct pci_dev *pdev = vdev->pdev;
+
+	if (!vdev->pci_recovery_supported) {
+		pci_unmap_rom(pdev, io);
+		return;
+	}
+
+	down_read(&vdev->recovery_lock);
+	if (vdev->pci_recovery_device_open &&
+	    !vdev->pci_recovery_access_blocked) {
+		pci_unmap_rom(pdev, io);
+	} else {
+		iounmap(io);
+		WRITE_ONCE(vdev->pci_recovery_rom_disable, true);
+	}
+	up_read(&vdev->recovery_lock);
+}
+
 ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 			size_t count, loff_t *ppos, bool iswrite)
 {
 	struct pci_dev *pdev = vdev->pdev;
 	loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
 	int bar = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
+	int ret;
 	size_t x_start = 0, x_end = 0;
 	resource_size_t end;
 	void __iomem *io;
@@ -230,7 +275,11 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 		 * filling large ROM BARs much faster.
 		 */
 		if (pci_resource_start(pdev, bar)) {
+			ret = vfio_pci_core_access_begin(vdev);
+			if (ret)
+				return ret;
 			io = pci_map_rom(pdev, &x_start);
+			vfio_pci_core_access_end(vdev);
 		} else {
 			io = ioremap(pdev->rom, pdev->romlen);
 			x_start = pdev->romlen;
@@ -269,7 +318,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 
 	if (bar == PCI_ROM_RESOURCE) {
 		if (pci_resource_start(pdev, bar))
-			pci_unmap_rom(pdev, io);
+			vfio_pci_unmap_rom(vdev, io);
 		else
 			iounmap(io);
 	}
-- 
2.43.0