media: ipu6: Stop calling ISR hooks of unloaded drivers

isys_probe() points adev->auxdrv and adev->auxdrv_data at data in the isys module, and the buttress interrupt handler calls the isys ISR through them. Neither isys_remove() nor the probe error path clears them. Once intel_ipu6_isys is unloaded, any buttress interrupt dereferences memory of the unloaded module.

One way to hit this: the IRQ is shared, so with CONFIG_DEBUG_SHIRQ free_irq() runs the handler once more. If a buttress interrupt status bit is pending at that point, "rmmod intel_ipu6" after "rmmod intel_ipu6_isys" oopses:

  BUG: unable to handle page fault for address: ffffffffc8a00560
  RIP: 0010:ipu6_buttress_isr+0x19b/0x370 [intel_ipu6]
  Call Trace:
   free_irq+0x16b/0x360
   devres_release+0x37/0x80
   devm_free_irq+0x42/0x70
   ipu6_pci_remove+0x52/0xd0 [intel_ipu6]

This happened on a Samsung Galaxy Book3 Ultra. A module notifier added for testing confirmed that after "rmmod intel_ipu6_isys" the hook still points into the unloaded module, and that it is NULL with this change.

Set the hooks only after the last early return of isys_probe(). Clear them on the probe error path and at the end of isys_remove(), then synchronize_irq(). In the buttress handlers, read auxdrv_data once, so that a hook cleared concurrently is seen as NULL rather than dereferenced.

Fixes f50c4ca0a820 ("media: intel/ipu6: add the main input system driver")

Cc [email protected]

Signed-off-by Felipe Calliari <[email protected]>

---
 drivers/media/pci/intel/ipu6/ipu6-buttress.c | 15 ++++++++-----
 drivers/media/pci/intel/ipu6/ipu6-isys.c     | 23 +++++++++++++++++---
 2 files changed, 30 insertions(+), 8 deletions(-)

drivers/media/pci/intel/ipu6/ipu6-buttress.c +1 -0

index 105de1744..63197f746 100644
@@ -315,15 +315,20 @@ ipu6_buttress_ipc_send(struct ipu6_device *isp,
 
 static irqreturn_t ipu6_buttress_call_isr(struct ipu6_bus_device *adev)
 {
+	const struct ipu6_auxdrv_data *drv_data;
 	irqreturn_t ret = IRQ_WAKE_THREAD;

drivers/media/pci/intel/ipu6/ipu6-buttress.c +5 -2

+
+	if (drv_data->isr)
+		ret = drv_data->isr(adev);
 
-	if (ret == IRQ_WAKE_THREAD && !adev->auxdrv_data->isr_threaded)
+	if (ret == IRQ_WAKE_THREAD && !drv_data->isr_threaded)
 		ret = IRQ_NONE;
 
 	return ret;

@@ -436,7 +441,7 @@ irqreturn_t ipu6_buttress_isr_threaded(int irq, void *isp_ptr)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(adev) && adev[i]; i++) {
-		drv_data = adev[i]->auxdrv_data;
+		drv_data = READ_ONCE(adev[i]->auxdrv_data);
 		if (!drv_data)
 			continue;
 

drivers/media/pci/intel/ipu6/ipu6-isys.c +20 -3

index 08f29b678..15254e3e3 100644
@@ -13,6 +13,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/firmware.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irqreturn.h>
 #include <linux/list.h>

@@ -989,6 +990,18 @@ void ipu6_put_fw_msg_buf(struct ipu6_isys *isys, struct isys_fw_msgs *msg)
 static const struct ipu6_auxdrv_data ipu6_isys_auxdrv_data;
 static const struct ipu6_auxdrv_data ipu7_isys_auxdrv_data;
 
+/*
+ * The buttress interrupt handler calls into this driver through
+ * adev->auxdrv_data, which points into this module. Clear it once the
+ * device is torn down, and wait for a running handler to finish.
+ */
+static void isys_unset_auxdrv(struct ipu6_bus_device *adev)
+{
+	WRITE_ONCE(adev->auxdrv, NULL);
+	WRITE_ONCE(adev->auxdrv_data, NULL);
+	synchronize_irq(adev->isp->pdev->irq);
+}
+
 static int isys_probe(struct auxiliary_device *auxdev,
 		      const struct auxiliary_device_id *auxdev_id)
 {

@@ -1006,9 +1019,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
 	if (!isys)
 		return -ENOMEM;
 
-	adev->auxdrv_data = IS_IPU7(isp) ? &ipu7_isys_auxdrv_data :
-					   &ipu6_isys_auxdrv_data;
-	adev->auxdrv = to_auxiliary_drv(auxdev->dev.driver);
 	isys->adev = adev;
 	isys->pdata = adev->pdata;
 	csi2_pdata = &isys->pdata->ipdata->csi2;

@@ -1037,6 +1047,10 @@ static int isys_probe(struct auxiliary_device *auxdev,
 
 	dev_set_drvdata(&auxdev->dev, isys);
 
+	adev->auxdrv_data = IS_IPU7(isp) ? &ipu7_isys_auxdrv_data :
+					   &ipu6_isys_auxdrv_data;
+	adev->auxdrv = to_auxiliary_drv(auxdev->dev.driver);
+
 	isys_stream_init(isys);
 
 	cpu_latency_qos_add_request(&isys->pm_qos, PM_QOS_DEFAULT_VALUE);

@@ -1065,6 +1079,7 @@ static int isys_probe(struct auxiliary_device *auxdev,
 free_fw_msg_bufs:
 	free_fw_msg_bufs(isys);
 out_remove_pkg_dir_shared_buffer:
+	isys_unset_auxdrv(adev);
 	cpu_latency_qos_remove_request(&isys->pm_qos);
 
 	for (i = 0; i < IPU6_ISYS_MAX_STREAMS; i++)

@@ -1094,6 +1109,8 @@ static void isys_remove(struct auxiliary_device *auxdev)
 	isys_iwake_watermark_cleanup(isys);
 	mutex_destroy(&isys->stream_mutex);
 	mutex_destroy(&isys->mutex);
+
+	isys_unset_auxdrv(auxdev_to_adev(auxdev));
 }
 
 static const struct ipu6_auxdrv_data ipu6_isys_auxdrv_data = {

ipu6_buttress_isr() calls the isys and psys ISRs on every buttress interrupt, and only afterwards checks whether the interrupt was theirs. So each isys interrupt also runs the psys ISR, and each psys interrupt runs the isys ISR, only to have the result discarded.

Beyond the wasted work, this makes an interrupt for one device dereference the other device's hooks. After intel_ipu6_psys is unloaded, its adev->auxdrv_data still points into the unloaded module, and every isys interrupt, e.g. on the next stream, calls through it. This matches a hard lockup without a trace reported on the first stream after unloading the psys driver. On a Samsung Galaxy Book3 Ultra, instrumenting the ISR showed that a 60-frame capture after "rmmod intel_ipu6_psys" would have made at least ten calls through the stale psys hooks. The same capture made none with this change, and captured all 60 frames.

Check the interrupt status bit before calling the ISR.

Reported-by Mars-Wave <[email protected]>

Closes https://lore.kernel.org/linux-media/[email protected]/ open here

Fixes ab29a2478e70 ("media: intel/ipu6: add IPU6 buttress interface driver")

Cc [email protected]

Signed-off-by Felipe Calliari <[email protected]>

---
 drivers/media/pci/intel/ipu6/ipu6-buttress.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/media/pci/intel/ipu6/ipu6-buttress.c +0 -0

index 63197f746..74c191d72 100644
@@ -369,11 +369,13 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
 		writel(irq_status, isp->base + regs->irq_clear);

drivers/media/pci/intel/ipu6/ipu6-buttress.c +0 -0

 			if (r == IRQ_WAKE_THREAD) {
 				ret = IRQ_WAKE_THREAD;
 				disable_irqs |= adev_irq_mask[i];