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

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

[Felipe Calliari]

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

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

[Sakari Ailus]

Hi Felipe,

On Wed, Sep 23, 2026 at 08:42:23PM -0300, Felipe Calliari wrote:
> 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;

This patch looks very much LLM-generated. Are the tags in
Documentation/process/coding-assistants.rst relevant for this?

I believe you need something more elaborate to guard against unbinding the
driver. Do note that we currently can't safely remove the ISYS driver if
the userspace isn't guaranteed to have no file handles open to the device
nodes related to the Media device and the related sub-device and video
device nodes.

> +
> +	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 = {

-- 
Regards,

Sakari Ailus

[PATCH 2/2] media: ipu6: Only call the isys and psys ISRs for their own interrupts

[Felipe Calliari]

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 <tmorolias@gmail.com>
Closes: https://lore.kernel.org/linux-media/20260922063507.690-1-tmorolias@gmail.com/
Fixes: ab29a2478e70 ("media: intel/ipu6: add IPU6 buttress interface driver")
Cc: stable@vger.kernel.org
Signed-off-by: Felipe Calliari <calliarifelipe@gmail.com>
---
 drivers/media/pci/intel/ipu6/ipu6-buttress.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
index 63197f746..74c191d72 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
@@ -369,11 +369,13 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
 		writel(irq_status, isp->base + regs->irq_clear);
 
 		for (i = 0; i < ARRAY_SIZE(adev_irq_mask); i++) {
-			irqreturn_t r = ipu6_buttress_call_isr(adev[i]);
+			irqreturn_t r;
 
 			if (!(irq_status & adev_irq_mask[i]))
 				continue;
 
+			r = ipu6_buttress_call_isr(adev[i]);
+
 			if (r == IRQ_WAKE_THREAD) {
 				ret = IRQ_WAKE_THREAD;
 				disable_irqs |= adev_irq_mask[i];
-- 
2.55.0

Re: [PATCH 2/2] media: ipu6: Only call the isys and psys ISRs for their own interrupts

[Sakari Ailus]

Hi Felipe,

Thanks for the patch.

On Wed, Sep 23, 2026 at 08:42:24PM -0300, Felipe Calliari wrote:
> 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.

The problem here is a dangling pointer. Can you also post a patch to set
that drvdata to NULL?

The above paragraph isn't relevant in the context of this patch.

> 
> Check the interrupt status bit before calling the ISR.
> 
> Reported-by: Mars-Wave <tmorolias@gmail.com>
> Closes: https://lore.kernel.org/linux-media/20260922063507.690-1-tmorolias@gmail.com/
> Fixes: ab29a2478e70 ("media: intel/ipu6: add IPU6 buttress interface driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Felipe Calliari <calliarifelipe@gmail.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-buttress.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> index 63197f746..74c191d72 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
> @@ -369,11 +369,13 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
>  		writel(irq_status, isp->base + regs->irq_clear);
>  
>  		for (i = 0; i < ARRAY_SIZE(adev_irq_mask); i++) {
> -			irqreturn_t r = ipu6_buttress_call_isr(adev[i]);
> +			irqreturn_t r;
>  
>  			if (!(irq_status & adev_irq_mask[i]))
>  				continue;
>  
> +			r = ipu6_buttress_call_isr(adev[i]);
> +

No need for this newline.

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

-- 
Regards,

Sakari Ailus