Re: [PATCH v10 11/14] media: iris: Add hooks for pixel and non-pixel context banks

On 7/26/2026 11:33 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Dangling pointer in `vb2_queue->dev` due to synchronous unregistration of context-bank platform devices.
>
> Pre-existing issues:
> - [High] Missing cancel_delayed_work_sync() for sys_error_handler in iris_remove() leads to use-after-free.
> --
>
> --- Patch [11]: [PATCH v10 11/14] media: iris: Add hooks for pixel and non-pixel context banks ---
> commit 1f1fdc377aea1a90f259a986e4bc14be351b3bc0
> Author: Vikash Garodia <[email protected]>
>
> media: iris: Add hooks for pixel and non-pixel context banks
>     
> Iris platforms use separate context-bank devices for the pixel, non-pixel
> firmware domains. Add platform hooks to create and destroy those
> subdevices, providing a interface for their lifecycle management across
> platforms.
>     
>> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>> index ec35ad4f86102..bc95be22ed96d 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>> @@ -397,6 +397,52 @@ static u64 iris_vpu36_get_required_freq(struct iris_inst *inst)
>>  	return max(vcodec0_freq, vcodec1_freq);
>>  }
>>  
>> +static int iris_vpu3x_init_cb_devs(struct iris_core *core)
>> +{
>> +	struct device *dev;
>> +
>> +	dev = iris_create_cb_dev(core, "non-pixel");
>> +	if (IS_ERR(dev))
>> +		return PTR_ERR(dev);
>> +
>> +	core->np_dev = dev;
>> +
>> +	dev = iris_create_cb_dev(core, "pixel");
>> +	if (IS_ERR(dev))
>> +		goto unreg_np_dev;
>> +
>> +	core->p_dev = dev;
>> +
>> +	dev = iris_create_cb_dev(core, "video-firmware");
>> +	if (IS_ERR(dev))
>> +		goto unreg_p_dev;
>> +
>> +	core->fw_dev = dev;
>> +
>> +	return 0;
>> +
>> +unreg_p_dev:
>> +	if (core->p_dev)
>> +		platform_device_unregister(to_platform_device(core->p_dev));
>> +	core->p_dev = NULL;
>> +unreg_np_dev:
>> +	if (core->np_dev)
>> +		platform_device_unregister(to_platform_device(core->np_dev));
>> +	core->np_dev = NULL;
>> +
>> +	return PTR_ERR(dev);
>> +}
>> +
>> +static void iris_vpu3x_deinit_cb_devs(struct iris_core *core)
>> +{
>> +	if (core->fw_dev)
>> +		platform_device_unregister(to_platform_device(core->fw_dev));
>> +	if (core->p_dev)
>> +		platform_device_unregister(to_platform_device(core->p_dev));
>> +	if (core->np_dev)
>> +		platform_device_unregister(to_platform_device(core->np_dev));
>> +}
> [Severity: High]
> Does this code synchronously free the context-bank platform devices without
> waiting for userspace V4L2 file descriptors to close?
>
> If a userspace application holds a video device file descriptor open during
> driver unbind, this function will unregister and free the context-bank
> platform devices.

Remove callback cannot execute if driver module->refcnt, ref-counted
during open, and not decremented with corresponding close call.

> However, in iris_m2m_queue_init(), the context-bank devices are assigned to
> vb2_queue->dev:
>
> drivers/media/platform/qcom/iris/iris_vidc.c:iris_m2m_queue_init() {
>     ...
> 	src_vq->dev = iris_get_cb_dev(inst, BUF_INPUT);
>     ...
> 	dst_vq->dev = iris_get_cb_dev(inst, BUF_OUTPUT);
>     ...
> }
>
> Because the V4L2 core does not hold a reference to q->dev during videobuf2
> queue initialization, won't this result in a use-after-free when userspace
> finally closes the file descriptor and v4l2_m2m_ctx_release() triggers
> vb2_queue_release() to unmap DMA buffers?
>
> [Severity: High]
> This is a pre-existing issue, but does iris_remove() fail to cancel the
> sys_error_handler delayed work before returning?

This is addressed here

https://lore.kernel.org/all/[email protected]

> drivers/media/platform/qcom/iris/iris_probe.c:iris_remove() {
>     ...
> 	iris_core_deinit(core);
>
> 	video_unregister_device(core->vdev_dec);
> 	video_unregister_device(core->vdev_enc);
>
> 	v4l2_device_unregister(&core->v4l2_dev);
>
> 	iris_deinit_cb_devs(core);
>
> 	mutex_destroy(&core->lock);
> }
>
> If a system fatal error triggers an interrupt just before or during module
> removal, the sys_error_handler delayed work could be scheduled.
>
> Since iris_remove() doesn't explicitly cancel this work before returning,
> could the handler execute after the devm_kzalloc-managed core structure is
> freed?
>
> drivers/media/platform/qcom/iris/iris_probe.c:iris_sys_error_handler() {
> 	struct iris_core *core =
> 			container_of(work, struct iris_core, sys_error_handler.work);
>
> 	iris_core_deinit(core);
> 	iris_core_init(core);
> }
>