[PATCH v3] staging: most: dim2: fix race condition in network info delivery

[PATCH v3] staging: most: dim2: fix race condition in network info delivery

[Minu Jin]

The previous implementation could lead to race conditions between
deliver_netinfo_thread() and retrieve_netinfo().
Additionally, multiple interrupts could lead to event loss.

1. Using local variables to store a snapshot of the network
   information under a spinlock. This prevents race conditions between
   deliver_netinfo_thread() and retrieve_netinfo().

2. Invoking the on_netinfo() callback outside the spinlock. Since
   callbacks may sleep or take other locks, it must be called after
   releasing the spinlock, using the previously captured local variables.

3. Introducing an 'is_netinfo_pending' flag to track the delivery
   status. This avoids redundant wake-ups of the delivery
   thread when multiple interrupts occur.

Fixes: ba3d7ddfb5c6 ("Staging: most: add MOST driver's hdm-dim2 module")
Signed-off-by: Minu Jin <s9430939@naver.com>
---
Changes in v3:
- No code chages: resend to include missing mailing lists.

Changes in v2:
- Add 'is_netinfo_pending' flag to prevent redundant wake-ups.
- Add Fixes tag.

Test Note:
- Compiled success, with no error.
- Verified with Smatch, no new warnings.

 drivers/staging/most/dim2/dim2.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index 66617e89e028..b75e1db671dc 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -97,6 +97,7 @@ struct dim2_hdm {
 	void (*on_netinfo)(struct most_interface *most_iface,
 			   unsigned char link_state, unsigned char *addrs);
 	void (*disable_platform)(struct platform_device *pdev);
+	bool is_netinfo_pending;
 };
 
 struct dim2_platform_data {
@@ -212,20 +213,29 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
 static int deliver_netinfo_thread(void *data)
 {
 	struct dim2_hdm *dev = data;
+	unsigned long flags;
+	unsigned char local_mac_addrs[6];
+	unsigned char local_link_state;
 
 	while (!kthread_should_stop()) {
 		wait_event_interruptible(dev->netinfo_waitq,
 					 dev->deliver_netinfo ||
 					 kthread_should_stop());
 
+		spin_lock_irqsave(&dim_lock, flags);
 		if (dev->deliver_netinfo) {
 			dev->deliver_netinfo--;
+			memcpy(local_mac_addrs, dev->mac_addrs, 6);
+			local_link_state = dev->link_state;
+			dev->is_netinfo_pending = false;
+			spin_unlock_irqrestore(&dim_lock, flags);
 			if (dev->on_netinfo) {
 				dev->on_netinfo(&dev->most_iface,
-						dev->link_state,
-						dev->mac_addrs);
+						local_link_state,
+						local_mac_addrs);
 			}
-		}
+		} else
+			spin_unlock_irqrestore(&dim_lock, flags);
 	}
 
 	return 0;
@@ -242,12 +252,24 @@ static int deliver_netinfo_thread(void *data)
 static void retrieve_netinfo(struct dim2_hdm *dev, struct mbo *mbo)
 {
 	u8 *data = mbo->virt_address;
+	unsigned long flags;
 
 	dev_dbg(&dev->dev, "Node Address: 0x%03x\n", (u16)data[16] << 8 | data[17]);
+
+	spin_lock_irqsave(&dim_lock, flags);
+
 	dev->link_state = data[18];
-	dev_dbg(&dev->dev, "NIState: %d\n", dev->link_state);
 	memcpy(dev->mac_addrs, data + 19, 6);
+	if (dev->is_netinfo_pending) {
+		spin_unlock_irqrestore(&dim_lock, flags);
+		return;
+	}
+
+	dev->is_netinfo_pending = true;
 	dev->deliver_netinfo++;
+	spin_unlock_irqrestore(&dim_lock, flags);
+
+	dev_dbg(&dev->dev, "NIState: %d\n", dev->link_state);
 	wake_up_interruptible(&dev->netinfo_waitq);
 }
 
-- 
2.43.0

Re: [PATCH v3] staging: most: dim2: fix race condition in network info delivery

[Dan Carpenter]

On Thu, Mar 19, 2026 at 01:49:06AM +0900, Minu Jin wrote:
> The previous implementation could lead to race conditions between
> deliver_netinfo_thread() and retrieve_netinfo().
> Additionally, multiple interrupts could lead to event loss.
> 
> 1. Using local variables to store a snapshot of the network
>    information under a spinlock. This prevents race conditions between
>    deliver_netinfo_thread() and retrieve_netinfo().
> 
> 2. Invoking the on_netinfo() callback outside the spinlock. Since
>    callbacks may sleep or take other locks, it must be called after
>    releasing the spinlock, using the previously captured local variables.
> 
> 3. Introducing an 'is_netinfo_pending' flag to track the delivery
>    status. This avoids redundant wake-ups of the delivery
>    thread when multiple interrupts occur.
> 
> Fixes: ba3d7ddfb5c6 ("Staging: most: add MOST driver's hdm-dim2 module")
> Signed-off-by: Minu Jin <s9430939@naver.com>
> ---
> Changes in v3:
> - No code chages: resend to include missing mailing lists.
> 
> Changes in v2:
> - Add 'is_netinfo_pending' flag to prevent redundant wake-ups.
> - Add Fixes tag.
> 
> Test Note:
> - Compiled success, with no error.
> - Verified with Smatch, no new warnings.
> 

I had really hoped this was tested...  It's difficult to know just
from looking at it if it's correct.  We could add a FIXME in the
code:

	/* FIXME: This is racy.  See:
         * https://lore.kernel.org/all/20260318164906.262225-1-s9430939@naver.com/
	 */

We wouldn't need is_netinfo_pending because we already have
dev->deliver_netinfo.  But I'm not sure that this patch is correct.
Probably it's less correct than the current behavior which will
eventually set it to the right thing even if it goes through a corrupted
step first.

regards,
dan carpenter