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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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