Re: [PATCH v13 03/17] media: ti: j721e-csi2rx: separate out device and context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Incorrect teardown order in `ti_csi2rx_cleanup_ctx` releases the DMA channel and vb2 queue before unregistering the video device.
- [Low] Comma operator used instead of semicolons for struct field assignments.

Pre-existing issues:
- [High] Unsafe teardown order in `ti_csi2rx_remove` destroys contexts before unregistering the async notifier.
- [High] The `video_device` struct is freed via devres before userspace file descriptors are closed.
--

commit 2c69e8bb02469f4292c85082c0033fc459b17d78
Author: Jai Luthra <jai.luthra@linux.dev>
media: ti: j721e-csi2rx: separate out device and context

This commit separates out the device and DMA context in the TI CSI2RX driver
to support multiplexed stream capture. It creates a new ti_csi2rx_ctx
struct to hold the context-specific data.

> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index 710d05a053539..23717a3b6c4cf 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c

[ ... ]

> +static void ti_csi2rx_cleanup_ctx(struct ti_csi2rx_ctx *ctx)
> +{
> +	dma_release_channel(ctx->dma.chan);
> +	vb2_queue_release(&ctx->vidq);
> +
> +	video_unregister_device(&ctx->vdev);
> +
> +	mutex_destroy(&ctx->mutex);
> +}

[Severity: High]
Can this cause a use-after-free if the driver is unbound while streaming
is active?

If vb2_queue_release() is called before video_unregister_device(), it
stops the queue, which invokes ti_csi2rx_stop_streaming() and subsequently
ti_csi2rx_stop_dma(). ti_csi2rx_stop_dma() will then attempt to access the
already freed ctx->dma.chan pointer to terminate the DMA.

Should the video device be unregistered first to prevent concurrent ioctls,
followed by releasing the vb2 queue, and finally releasing the DMA channel?

[ ... ]

> +static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
> +{

[ ... ]

> -	pix_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> -	pix_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
> -	pix_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
> -
> -	ti_csi2rx_fill_fmt(fmt, &csi->v_fmt);
> +	pix_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601,
> +	pix_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE,
> +	pix_fmt->xfer_func = V4L2_XFER_FUNC_SRGB,
>  
> -	mdev->dev = csi->dev;
> -	mdev->hw_revision = 1;
> -	strscpy(mdev->model, "TI-CSI2RX", sizeof(mdev->model));
> +	ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt);

[Severity: Low]
This isn't a bug, but is it intentional to use commas instead of semicolons
for these assignments? It appears to be a typo introduced during the
refactoring.

[ ... ]

>  	vdev->vfl_dir = VFL_DIR_RX;
>  	vdev->fops = &csi_fops;
>  	vdev->ioctl_ops = &csi_ioctl_ops;
>  	vdev->release = video_device_release_empty;

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a
use-after-free if a userspace process holds an open file descriptor when the
device unbinds?

The video_device is embedded inside ti_csi2rx_ctx, which is part of the
ti_csi2rx_dev allocated via devm_kzalloc(). Since devres frees the memory
immediately upon returning from the remove function, any later V4L2 operations
or file descriptor closures by userspace might access freed memory.

[ ... ]

>  static void ti_csi2rx_remove(struct platform_device *pdev)
>  {
>  	struct ti_csi2rx_dev *csi = platform_get_drvdata(pdev);
> +	unsigned int i;
>  
> -	video_unregister_device(&csi->vdev);
> +	for (i = 0; i < TI_CSI2RX_NUM_CTX; i++)
> +		ti_csi2rx_cleanup_ctx(&csi->ctx[i]);
>  
> -	ti_csi2rx_cleanup_vb2q(csi);
> -	ti_csi2rx_cleanup_subdev(csi);
> +	ti_csi2rx_cleanup_notifier(csi);

[Severity: High]
This is a pre-existing issue, but does cleaning up the contexts before
unregistering the async notifier pose a risk?

If the async subdev binding completes concurrently while remove is executing,
the notifier's complete callback (csi_async_notifier_complete) might attempt
to register video devices using the already destroyed contexts.

Should the async notifier be unregistered first to guarantee no async
callbacks fire during teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520120022.539913-1-r-donadkar@ti.com?part=3