[PATCH 1/2] media: ipu6: Clear the isys ISR hooks when the isys driver goes away

[Felipe Calliari <calliarifelipe@gmail.com>]

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: stable@vger.kernel.org
Signed-off-by: Felipe Calliari <calliarifelipe@gmail.com>
---
 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(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
index 105de1744..63197f746 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
@@ -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;
 
-	if (!adev || !adev->auxdrv || !adev->auxdrv_data)
+	if (!adev || !READ_ONCE(adev->auxdrv))
 		return IRQ_NONE;
 
-	if (adev->auxdrv_data->isr)
-		ret = adev->auxdrv_data->isr(adev);
+	drv_data = READ_ONCE(adev->auxdrv_data);
+	if (!drv_data)
+		return IRQ_NONE;
+
+	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;
 
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
index 08f29b678..15254e3e3 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
@@ -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 = {
-- 
2.55.0