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

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

[Pavan Kumar Yalagada]

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_probe_channel() checks comp_exiting before modifying video_devices.

	if (comp_exiting) {
		spin_unlock_irq(&list_lock);
		ret = -BUSY;
		goto err_unreg;
	}

This ensures that all partially created resources are properly freed
and no new channels are allowed while the driver is being unloaded.

Signed-off-by: Pavan Kumar Yalagada <pavankumaryalagada@gmail.com>

---

v5:
 - Removed unlikely() macro since this is not fast path.
 - Added comp_unregister_videodev() to err_unreg cleanup patch.
 - Removed blank line in comp_exit().

v4:
 - Used unlikely() for the comp_exiting check to optimize branch
   prediction.
 - Removed err_cleanup and use err_unreg for proper cleanup
   of partially initialized mdev/v4l2 structures.
 - Removed redundant spinlock usage around 'comp_exiting = true;'
   comp_exit().
 - Removed explicit kfree(mdev) in comp_exit() loop.

v3:
 - comp_exiting flag update and memory cleanup for early exits.
 - Commit message clarified for reviewers.
 - Removed WARN/BUG as it becomes unnecessary.
---
 drivers/staging/most/video/video.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index 32f71d9a9cf7..11e63f5d5b84 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,11 +500,17 @@ 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);
+		ret = -EBUSY;
+		goto err_unreg;
+	}
 	list_add(&mdev->list, &video_devices);
 	spin_unlock_irq(&list_lock);
 	return 0;

 err_unreg:
+	comp_unregister_videodev(mdev);
 	v4l2_device_disconnect(&mdev->v4l2_dev);
 	v4l2_device_put(&mdev->v4l2_dev);
 	return ret;
@@ -555,6 +563,8 @@ static void __exit comp_exit(void)
 {
 	struct most_video_dev *mdev, *tmp;

+	comp_exiting = true;
+
 	/*
 	 * As the mostcore currently doesn't call disconnect_channel()
 	 * for linked channels while we call most_deregister_component()
@@ -575,7 +585,6 @@ static void __exit comp_exit(void)

 	most_deregister_configfs_subsys(&comp);
 	most_deregister_component(&comp);
-	BUG_ON(!list_empty(&video_devices));
 }

 module_init(comp_init);
--
2.47.3

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

[Greg KH]

On Wed, Dec 03, 2025 at 03:34:11AM -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.

Wait, please step back.  How can this happen?  I would argue that if it
could happen, then this must be solved at the higher level.  The most
core should not be allowing this, as you do not want to be forced to
solve this for each individual driver (hint, the driver core does not
allow this to happen for normal driver operations for this very reason.)

You are only solving this for one most driver, not all of them (well,
there are only two with this codepath...)  Please move back up the stack
a bit and prevent this from even being an issue at all.  Especially as I
do not think this really solves the problem, see below:

> This patch prevents new devices from being added while exiting by:
> 
> - comp_probe_channel() checks comp_exiting before modifying video_devices.
> 
> 	if (comp_exiting) {
> 		spin_unlock_irq(&list_lock);
> 		ret = -BUSY;
> 		goto err_unreg;
> 	}

That's great, but what prevents comp_exiting from changing right after
you check it?

thanks,

greg k-h

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

[Dan Carpenter]

On Wed, Dec 03, 2025 at 03:34:11AM -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_probe_channel() checks comp_exiting before modifying video_devices.
> 
> 	if (comp_exiting) {
> 		spin_unlock_irq(&list_lock);
> 		ret = -BUSY;
> 		goto err_unreg;
> 	}
> 
> This ensures that all partially created resources are properly freed
> and no new channels are allowed while the driver is being unloaded.
> 
> Signed-off-by: Pavan Kumar Yalagada <pavankumaryalagada@gmail.com>

There we go.  Well done.  :)

Reviewed-by:  Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

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

[Dan Carpenter]

On Wed, Dec 03, 2025 at 10:05:17AM +0100, Greg KH wrote:
> On Wed, Dec 03, 2025 at 03:34:11AM -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.
> 
> Wait, please step back.  How can this happen?  I would argue that if it
> could happen, then this must be solved at the higher level.  The most
> core should not be allowing this, as you do not want to be forced to
> solve this for each individual driver (hint, the driver core does not
> allow this to happen for normal driver operations for this very reason.)
> 
> You are only solving this for one most driver, not all of them (well,
> there are only two with this codepath...)  Please move back up the stack
> a bit and prevent this from even being an issue at all.  Especially as I
> do not think this really solves the problem, see below:
> 

I had looked at this before and had assumed it's more complicated than
it actually is.  All this stuff comes from configfs so all we need to do
is move the most_deregister_configfs_subsys(&comp) earlier in the
function.

> > This patch prevents new devices from being added while exiting by:
> > 
> > - comp_probe_channel() checks comp_exiting before modifying video_devices.
> > 
> > 	if (comp_exiting) {
> > 		spin_unlock_irq(&list_lock);
> > 		ret = -BUSY;
> > 		goto err_unreg;
> > 	}
> 
> That's great, but what prevents comp_exiting from changing right after
> you check it?

That would have been okay.  In that case it would have been added and then
removed from the list.

But, you're  right.  It's not the right fix.

regards,
dan carpenter