Re: [PATCH v5 06/13] media: iris: Route buffers to the matching context bank device

[Vishnu Reddy <busanna.reddy@oss.qualcomm.com>]

On 10/8/2026 3:48 PM, Dmitry Baryshkov wrote:
> On Sat, Sep 26, 2026 at 02:01:21PM +0530, Vishnu Reddy wrote:
>>
>> On 9/26/2026 12:19 PM, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>> - [High] media: use-after-free of context bank devices during deferred V4L2 cleanup
>>> --
>>>
>>> commit 720de65140e46882c5d0651161ef771fcb1189a2
>>> Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
>>>
>>> media: iris: Route buffers to the matching context bank device
>>>
>>> The VPU issues DMA through several SMMU streams, and the hardware does
>>> not give every stream the same addressable range. The non-pixel stream
>>> cannot address the low 600MB of IOVA space, while the pixel stream can
>>> address the full range.
>>> Add iris_get_cb_dev(), which maps a buffer type to the owning context
>>> bank device. Bitstream and internal buffers (BIN, ARP, COMV, LINE,
>>> NON_COMV, PERSIST) belong to the non-pixel device, and uncompressed
>>> buffers (DPB, PARTIAL, SCRATCH_1, SCRATCH_2, VPSS) to the pixel device.
>>> BUF_INPUT and BUF_OUTPUT depend on direction and are resolved from
>>> inst->domain: for a decoder the input is non-pixel and the output pixel,
>>> and the other way round for an encoder.
>>> Fall back to core->dev whenever the relevant context bank device is
>>> absent, so platforms still describing "iommus" on the parent iris node
>>> behave exactly as before to maintain backward compatibility.
>>>
>>>> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c b/drivers/media/platform/qcom/iris/iris_buffer.c
>>>> index eb8de60c1177f..6e03d13ac1a79 100644
>>>> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
>>>> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
>>>> @@ -531,7 +531,7 @@ static int iris_create_internal_buffer(struct iris_inst *inst,
>>>>  				       enum iris_buffer_type buffer_type, u32 index)
>>>>  {
>>>>  	struct iris_buffers *buffers = &inst->buffers[buffer_type];
>>>> -	struct iris_core *core = inst->core;
>>>> +	struct device *dev = iris_get_cb_dev(inst, buffer_type);
>>> [Severity: High]
>>> Does this acquire the context bank device for DMA mapping without taking a
>>> reference to guarantee its lifetime matches the buffer?
>>>
>>> If iris_remove() explicitly calls platform_device_unregister() on these
>>> devices upon sysfs driver unbind, and no standard buffers are active, their
>>> refcount can drop to 0 and they will be freed.
>> This is not true. Remove callback cannot complete if driver module->refcnt,
>> refcounted during open, and not decremented with corresponding close call.
>> iris_close() would call iris_destroy_internal_buffers() to free up the buffers
>> before un-registering the device.
> I don't follow this comment. There is nothing about module removal.
> Sashiko here (and in several other comments) tells about using the
> `unbind` file in sysfs to unbind the driver from the device. The module
> is still present in the memory, the module refcount is not decremented.

Thanks for clarifying the comment. Detaching the driver from device at runtime
might leave the interaction with hardware unclosed.
Would prefer adding something like this
    .suppress_bind_attrs = true;

Again, this is not just applicable to sub-node devices, but to parent device
too. Sashiko should classify this as a pre-existing issue.

>>>>  	struct iris_buffer *buffer;
>>>>  
>>>>  	if (!buffers->size)
>>> [ ... ]
>>>
>>>> @@ -650,10 +650,10 @@ int iris_queue_internal_buffers(struct iris_inst *inst, u32 plane)
>>>>  
>>>>  void iris_destroy_internal_buffer(struct iris_inst *inst, struct iris_buffer *buffer)
>>>>  {
>>>> -	struct iris_core *core = inst->core;
>>>> +	struct device *dev = iris_get_cb_dev(inst, buffer->type);
>>>>  
>>>>  	list_del(&buffer->list);
>>>> -	dma_free_attrs(core->dev, buffer->buffer_size, buffer->kvaddr,
>>>> +	dma_free_attrs(dev, buffer->buffer_size, buffer->kvaddr,
>>> [Severity: High]
>>> Can this result in a use-after-free of the context bank device during
>>> deferred V4L2 cleanup?
>>>
>>> If standard buffers are active, videobuf2 holds a reference. However, during
>>> deferred cleanup in iris_close(), v4l2_m2m_ctx_release() frees standard
>>> buffers before iris_destroy_all_internal_buffers() runs. This drops the
>>> final refcount, freeing the devices.
>> iris_destroy_all_internal_buffers() frees the internal buffers which is managed
>> within the driver outside the scope of v4l2_m2m_ctx_release().
>>
>>> Will this fetch and dereference the freed device pointer, passing it to
>>> dma_free_attrs()? Additionally, does the devm-allocated core also get freed
>>> on unbind, making inst->core a dangling pointer here?
>> Unbind will not happen until iris_close() completes(explained above), which
>> avoids this scenario.
>>