Re: [PATCH v3] staging: most: video: prevent probes during component exit

[Dan Carpenter <dan.carpenter@linaro.org>]

On Fri, Nov 28, 2025 at 11:50:33AM -0500, Pavan Kumar Yalagada wrote:
> When comp_exit() runs, comp_probe_channel() could still add new devices
> to video_devices, creating a race and potentially leaving the list in
> an inconsistent state.
> 
> This patch prevents new devices from being added while exiting by:
> 
>  - comp_exiting is set under lock to prevent new devices being added.
>  - Early exit paths free allocated memory (kfree) to avoid leaks.
>  - comp_probe_channel() checks comp_exiting before modifying video_devices.
>  - Removing WARN/BUG as it becomes unnecessary.
> 
> Signed-off-by: Pavan Kumar Yalagada <pavankumaryalagada@gmail.com>
> 
> ---
> 
> v3:
>  - comp_exiting flag update and memory cleanup for early exits.
>  - Commit message clarified for reviewers.
> ---
>  drivers/staging/most/video/video.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> index 32f71d9a9cf7..f257fb179813 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -60,6 +60,8 @@ static inline struct comp_fh *to_comp_fh(struct file *filp)
>  static LIST_HEAD(video_devices);
>  static DEFINE_SPINLOCK(list_lock);
> 
> +static bool comp_exiting;
> +
>  static inline bool data_ready(struct most_video_dev *mdev)
>  {
>  	return !list_empty(&mdev->pending_mbos);
> @@ -498,13 +500,22 @@ static int comp_probe_channel(struct most_interface *iface, int channel_idx,
>  		goto err_unreg;
> 
>  	spin_lock_irq(&list_lock);
> +	if (comp_exiting) {
> +		spin_unlock_irq(&list_lock);
> +		goto err_cleanup;

Why do you not need to unregister?  I don't think this error handling
is correct.

> +	}
>  	list_add(&mdev->list, &video_devices);
>  	spin_unlock_irq(&list_lock);
>  	return 0;
> 
> +err_cleanup:
> +	kfree(mdev);
> +	return -EBUSY;
> +
>  err_unreg:
>  	v4l2_device_disconnect(&mdev->v4l2_dev);
>  	v4l2_device_put(&mdev->v4l2_dev);
> +	kfree(mdev);
>  	return ret;
>  }
> 
> @@ -553,8 +564,13 @@ static int __init comp_init(void)
> 
>  static void __exit comp_exit(void)
>  {
> +	unsigned long flags;
>  	struct most_video_dev *mdev, *tmp;
> 
> +	spin_lock_irqsave(&list_lock, flags);
> +	comp_exiting = true;
> +	spin_unlock_irqrestore(&list_lock, flags);
> +

It doesn't make sense to call spin_unlock_irqrestore()
and the spin_lock_irq() on the next line.

So the issue is, we want a write barrier so that the write in comp_exit()
happens before the read in comp_probe_channel(), which the spinlocks
do accomplish, I suppose.

>  	/*
>  	 * As the mostcore currently doesn't call disconnect_channel()
>  	 * for linked channels while we call most_deregister_component()
> @@ -569,13 +585,14 @@ static void __exit comp_exit(void)
>  		comp_unregister_videodev(mdev);
>  		v4l2_device_disconnect(&mdev->v4l2_dev);
>  		v4l2_device_put(&mdev->v4l2_dev);
> +		kfree(mdev);

No no.  This isn't related to race conditions and it's probably
wrong as well.

regards,
dan carpenter