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

[Vikash Garodia <vikash.garodia@oss.qualcomm.com>]

On 7/9/2026 7:24 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Unregistering context bank devices in `remove` while V4L2 FDs are open causes DMA API mismatch and IOMMU faults.
> - [Medium] Hardcoded platform device ID causes name collision if multiple Iris instances exist.
> 
> Pre-existing issues:
> - [High] Missing cancellation of sys_error_handler delayed work leads to Use-After-Free.
> - [High] Use-After-Free on `iris_core` during deferred V4L2 close due to `devm_kzalloc`.
> --
> 
> --- Patch [6]: [RFC PATCH 06/11] media: iris: Add hooks for pixel and non-pixel context banks ---
> commit ac38538cd2eb8a1aa776f7262b1bd953d7ac3a78
> Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
> 
> media: iris: Add hooks for pixel and non-pixel context banks
>      
> Iris platforms use separate context-bank devices for the pixel and
> non-pixel domains. Add platform hooks to create and destroy those
> subdevices, and wire them up for the affected platforms.
> 
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> new file mode 100644
>> index 0000000000000..bea904a9249ba
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> @@ -0,0 +1,71 @@
> 
> [ ... ]
> 
>> +static int sm8550_init_cb_devs(struct iris_core *core)
>> +{
>> +	struct device *dev;
>> +
>> +	dev = iris_create_cb_dev(core, "non-pixel");
> 
> [Severity: Medium]
> When creating the child context banks here, could this cause a name
> collision if there are multiple Iris instances?
> 
> Looking at iris_create_cb_dev() in iris_resources.c, it zero-initializes
> struct platform_device_info, which forces the device id to 0:
> 
> drivers/media/platform/qcom/iris/iris_resources.c:iris_create_cb_dev() {
> 	struct platform_device_info plat_dev_info = {};
> 	...
> }
> 
> This causes platform_device_register_full() to assign exact names like
> "pixel.0" and "non-pixel.0". If a second Iris instance probes, wouldn't
> kobject_add() return -EEXIST? Should PLATFORM_DEVID_AUTO be used instead?
> 
>> +	if (IS_ERR(dev))
>> +		return PTR_ERR(dev);
>> +
>> +	core->np_dev = dev;
>> +
>> +	dev = iris_create_cb_dev(core, "pixel");
> 
> [ ... ]
> 
>> +static void sm8550_deinit_cb_devs(struct iris_core *core)
>> +{
>> +	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));
>> +
>> +	core->p_dev = NULL;
>> +	core->np_dev = NULL;
>> +}
> 
> [Severity: High]
> Does unregistering the context bank devices here cause a DMA API mismatch if
> a user-space application still holds a V4L2 file descriptor open?
> 
> V4L2 file handles can outlive the driver's remove callback. If a user closes
> the device after remove completes, iris_close() calls
> iris_destroy_all_internal_buffers() which calls iris_get_cb_dev().
> 
> Since p_dev and np_dev are NULL, iris_get_cb_dev() falls back to core->dev:
> 
> drivers/media/platform/qcom/iris/iris_resources.c:iris_get_cb_dev() {
> 	...
> 	return dev ? dev : core->dev;
> }
> 
> This means dma_free_attrs() is executed using core->dev, but the memory
> was originally allocated with the context bank devices. Could this mismatch
> violate the DMA API and cause IOMMU unmap faults?
> 
> [Severity: High]
> This is a pre-existing issue, but does tying the iris_core struct to the
> device lifecycle result in a Use-After-Free during a deferred V4L2 close?
> 
> In iris_probe(), core is allocated with devm_kzalloc():
> 
> drivers/media/platform/qcom/iris/iris_probe.c:iris_probe() {
> 	...
> 	core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL);
> 	...
> }
> 
> When the driver is unbound, iris_remove() returns and devres frees core.
> If an application holds a file descriptor open, iris_close() is eventually
> called, which dereferences inst->core to clean up sessions:
> 
> drivers/media/platform/qcom/iris/iris_vidc.c:iris_close() {
> 	...
> 	iris_remove_session(inst);
> 	...
> }
> 
> Since core is already freed, won't this cause memory corruption?
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but is it possible for the
> sys_error_handler delayed work to trigger a Use-After-Free if it runs
> during or after driver unbind?

Yes, this is addressed with

https://lore.kernel.org/all/20260604054708.42426-1-dennylin0707@gmail.com/

> 
> The sys_error_handler is dynamically scheduled on hardware errors, but
> iris_remove() does not appear to cancel it:
> 
> drivers/media/platform/qcom/iris/iris_probe.c:iris_remove() {
> 	struct iris_core *core;
> 
> 	core = platform_get_drvdata(pdev);
> 	if (!core)
> 		return;
> 
> 	iris_core_deinit(core);
> }
> 
> If the unbind occurs while the work is pending or running, core will be
> freed by devres after iris_remove() returns. When the workqueue executes
> iris_sys_error_handler(), won't it dereference the freed core?
>