[v1] staging: most: video: fix potential race in list iteration

[PATCH] staging: most: video: fix potential race in list iteration

Posted by Minu Jin 4 days, 16 hours ago

There is a pattern in the loops where the lock is dropped
to call a specific function and then re-acquired.

The list can be exposed during this short gap, creating a potential
race condition. This patch fixes the problem by moving nodes to a
local list while holding the lock, instead of using the
unlock/lock pattern in the loop.

Signed-off-by: Minu Jin <s9430939@naver.com>
---
 drivers/staging/most/video/video.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index 32f71d9a9cf7..e7db58e5746a 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -121,6 +121,7 @@ static int comp_vdev_close(struct file *filp)
 	struct comp_fh *fh = to_comp_fh(filp);
 	struct most_video_dev *mdev = fh->mdev;
 	struct mbo *mbo, *tmp;
+	LIST_HEAD(free_list);
 
 	/*
 	 * We need to put MBOs back before we call most_stop_channel()
@@ -134,12 +135,15 @@ static int comp_vdev_close(struct file *filp)
 	spin_lock_irq(&mdev->list_lock);
 	mdev->mute = true;
 	list_for_each_entry_safe(mbo, tmp, &mdev->pending_mbos, list) {
-		list_del(&mbo->list);
-		spin_unlock_irq(&mdev->list_lock);
-		most_put_mbo(mbo);
-		spin_lock_irq(&mdev->list_lock);
+		list_move(&mbo->list, &free_list);
 	}
 	spin_unlock_irq(&mdev->list_lock);
+
+	list_for_each_entry_safe(mbo, tmp, &free_list, list) {
+		list_del_init(&mbo->list);
+		most_put_mbo(mbo);
+	}
+
 	most_stop_channel(mdev->iface, mdev->ch_idx, &comp);
 	mdev->mute = false;
 
@@ -554,6 +558,7 @@ static int __init comp_init(void)
 static void __exit comp_exit(void)
 {
 	struct most_video_dev *mdev, *tmp;
+	LIST_HEAD(free_list);
 
 	/*
 	 * As the mostcore currently doesn't call disconnect_channel()
@@ -563,15 +568,16 @@ static void __exit comp_exit(void)
 	 */
 	spin_lock_irq(&list_lock);
 	list_for_each_entry_safe(mdev, tmp, &video_devices, list) {
-		list_del(&mdev->list);
-		spin_unlock_irq(&list_lock);
+		list_move(&mdev->list, &free_list);
+	}
+	spin_unlock_irq(&list_lock);
 
+	list_for_each_entry_safe(mdev, tmp, &free_list, list) {
+		list_del_init(&mdev->list);
 		comp_unregister_videodev(mdev);
 		v4l2_device_disconnect(&mdev->v4l2_dev);
 		v4l2_device_put(&mdev->v4l2_dev);
-		spin_lock_irq(&list_lock);
 	}
-	spin_unlock_irq(&list_lock);
 
 	most_deregister_configfs_subsys(&comp);
 	most_deregister_component(&comp);
-- 
2.43.0

Re: [PATCH] staging: most: video: fix potential race in list iteration

Posted by Dan Carpenter 3 days, 18 hours ago

On Tue, Feb 03, 2026 at 08:28:00PM +0900, Minu Jin wrote:
> There is a pattern in the loops where the lock is dropped
> to call a specific function and then re-acquired.
> 
> The list can be exposed during this short gap, creating a potential
> race condition. This patch fixes the problem by moving nodes to a
> local list while holding the lock, instead of using the
> unlock/lock pattern in the loop.
> 
> Signed-off-by: Minu Jin <s9430939@naver.com>
> ---
>  drivers/staging/most/video/video.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> index 32f71d9a9cf7..e7db58e5746a 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -121,6 +121,7 @@ static int comp_vdev_close(struct file *filp)
>  	struct comp_fh *fh = to_comp_fh(filp);
>  	struct most_video_dev *mdev = fh->mdev;
>  	struct mbo *mbo, *tmp;
> +	LIST_HEAD(free_list);
>  
>  	/*
>  	 * We need to put MBOs back before we call most_stop_channel()
> @@ -134,12 +135,15 @@ static int comp_vdev_close(struct file *filp)
>  	spin_lock_irq(&mdev->list_lock);
>  	mdev->mute = true;
>  	list_for_each_entry_safe(mbo, tmp, &mdev->pending_mbos, list) {
> -		list_del(&mbo->list);
> -		spin_unlock_irq(&mdev->list_lock);
> -		most_put_mbo(mbo);
> -		spin_lock_irq(&mdev->list_lock);
> +		list_move(&mbo->list, &free_list);
>  	}

This loop moves ever single item in the list one by one.  You could
do it in one step instead by copying the list_head and setting
pending_mbos to point to itself.

Same in other loop.

regards,
dan carpenter

[PATCH v2] staging: most: video: fix potential race in list iteration

Posted by Minu Jin 3 days, 1 hour ago

There is a pattern in the loops where the lock is dropped
to call a specific function and then re-acquired.

The list can be exposed during this short gap, creating a potential
race condition. This patch fixes the problem by replacing the list
head with a local free list using list_replace_init(), instead of
using the lock/unlock pattern in the loop.

Signed-off-by: Minu Jin <s9430939@naver.com>
---
Changes in v2:
- Use list_replace_init() instead of moving one-by-one.
  (suggested by Dan Carpenter)

 drivers/staging/most/video/video.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index 32f71d9a9cf7..8eeae209ff1c 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -121,6 +121,7 @@ static int comp_vdev_close(struct file *filp)
 	struct comp_fh *fh = to_comp_fh(filp);
 	struct most_video_dev *mdev = fh->mdev;
 	struct mbo *mbo, *tmp;
+	LIST_HEAD(free_list);
 
 	/*
 	 * We need to put MBOs back before we call most_stop_channel()
@@ -133,13 +134,14 @@ static int comp_vdev_close(struct file *filp)
 
 	spin_lock_irq(&mdev->list_lock);
 	mdev->mute = true;
-	list_for_each_entry_safe(mbo, tmp, &mdev->pending_mbos, list) {
-		list_del(&mbo->list);
-		spin_unlock_irq(&mdev->list_lock);
+	list_replace_init(&mdev->pending_mbos, &free_list);
+	spin_unlock_irq(&mdev->list_lock);
+
+	list_for_each_entry_safe(mbo, tmp, &free_list, list) {
+		list_del_init(&mbo->list);
 		most_put_mbo(mbo);
-		spin_lock_irq(&mdev->list_lock);
 	}
-	spin_unlock_irq(&mdev->list_lock);
+
 	most_stop_channel(mdev->iface, mdev->ch_idx, &comp);
 	mdev->mute = false;
 
@@ -554,6 +556,7 @@ static int __init comp_init(void)
 static void __exit comp_exit(void)
 {
 	struct most_video_dev *mdev, *tmp;
+	LIST_HEAD(free_list);
 
 	/*
 	 * As the mostcore currently doesn't call disconnect_channel()
@@ -562,16 +565,15 @@ static void __exit comp_exit(void)
 	 * This must be fixed in core.
 	 */
 	spin_lock_irq(&list_lock);
-	list_for_each_entry_safe(mdev, tmp, &video_devices, list) {
-		list_del(&mdev->list);
-		spin_unlock_irq(&list_lock);
+	list_replace_init(&video_devices, &free_list);
+	spin_unlock_irq(&list_lock);
 
+	list_for_each_entry_safe(mdev, tmp, &free_list, list) {
+		list_del_init(&mdev->list);
 		comp_unregister_videodev(mdev);
 		v4l2_device_disconnect(&mdev->v4l2_dev);
 		v4l2_device_put(&mdev->v4l2_dev);
-		spin_lock_irq(&list_lock);
 	}
-	spin_unlock_irq(&list_lock);
 
 	most_deregister_configfs_subsys(&comp);
 	most_deregister_component(&comp);
-- 
2.43.0

Re: [PATCH v2] staging: most: video: fix potential race in list iteration

Posted by Dan Carpenter 2 days, 22 hours ago

On Thu, Feb 05, 2026 at 11:16:20AM +0900, Minu Jin wrote:
> There is a pattern in the loops where the lock is dropped
> to call a specific function and then re-acquired.
> 
> The list can be exposed during this short gap, creating a potential
> race condition. This patch fixes the problem by replacing the list
> head with a local free list using list_replace_init(), instead of
> using the lock/unlock pattern in the loop.
> 
> Signed-off-by: Minu Jin <s9430939@naver.com>
> ---
> Changes in v2:
> - Use list_replace_init() instead of moving one-by-one.
>   (suggested by Dan Carpenter)

Thanks.

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

regards,
dan carpenter