[PATCH v2] usbmon: mark concurrent accesses to the monitored flag

[PATCH v2] usbmon: mark concurrent accesses to the monitored flag

[Zhu Jun]

USB submission and completion paths read bus->monitored without taking
mon_lock or the monitor bus lock. Adding and removing readers changes the
flag under those locks, so the plain accesses race, as reported by KCSAN.

Use READ_ONCE() for the fast-path checks and WRITE_ONCE() for the updates.
The monitor callbacks still serialize access to their reader lists using
the existing bus locks.

Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
---
Changes in v2:
- Extend the Fixes commit ID to 13 characters to avoid ambiguity.

 drivers/usb/mon/mon_main.c | 14 +++++++-------
 include/linux/usb/hcd.h    |  6 +++---
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/mon/mon_main.c b/drivers/usb/mon/mon_main.c
index 600c9de84c7e..e6fff0396a18 100644
--- a/drivers/usb/mon/mon_main.c
+++ b/drivers/usb/mon/mon_main.c
@@ -44,10 +44,10 @@ void mon_reader_add(struct mon_bus *mbus, struct mon_reader *r)
 			list_for_each (p, &mon_buses) {
 				struct mon_bus *m1;
 				m1 = list_entry(p, struct mon_bus, bus_link);
-				m1->u_bus->monitored = 1;
+				WRITE_ONCE(m1->u_bus->monitored, 1);
 			}
 		} else {
-			mbus->u_bus->monitored = 1;
+			WRITE_ONCE(mbus->u_bus->monitored, 1);
 		}
 	}
 	mbus->nreaders++;
@@ -163,7 +163,7 @@ static void mon_stop(struct mon_bus *mbus)
 			 * We do not change nreaders here, so rely on mon_lock.
 			 */
 			if (mbus->nreaders == 0 && (ubus = mbus->u_bus) != NULL)
-				ubus->monitored = 0;
+				WRITE_ONCE(ubus->monitored, 0);
 		}
 	} else {
 		/*
@@ -171,7 +171,7 @@ static void mon_stop(struct mon_bus *mbus)
 		 * a reader staying across an rmmod foo_hcd, so test ->u_bus.
 		 */
 		if (mon_bus0.nreaders == 0 && (ubus = mbus->u_bus) != NULL) {
-			ubus->monitored = 0;
+			WRITE_ONCE(ubus->monitored, 0);
 			mb();
 		}
 	}
@@ -188,7 +188,7 @@ static void mon_bus_add(struct usb_bus *ubus)
 	mon_bus_init(ubus);
 	mutex_lock(&mon_lock);
 	if (mon_bus0.nreaders != 0)
-		ubus->monitored = 1;
+		WRITE_ONCE(ubus->monitored, 1);
 	mutex_unlock(&mon_lock);
 }
 
@@ -246,8 +246,8 @@ static const struct usb_mon_operations mon_ops_0 = {
 static void mon_dissolve(struct mon_bus *mbus, struct usb_bus *ubus)
 {
 
-	if (ubus->monitored) {
-		ubus->monitored = 0;
+	if (READ_ONCE(ubus->monitored)) {
+		WRITE_ONCE(ubus->monitored, 0);
 		mb();
 	}
 
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 181db044c4d2..ce4525e4289b 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -720,21 +720,21 @@ extern const struct usb_mon_operations *mon_ops;
 
 static inline void usbmon_urb_submit(struct usb_bus *bus, struct urb *urb)
 {
-	if (bus->monitored)
+	if (READ_ONCE(bus->monitored))
 		(*mon_ops->urb_submit)(bus, urb);
 }
 
 static inline void usbmon_urb_submit_error(struct usb_bus *bus, struct urb *urb,
     int error)
 {
-	if (bus->monitored)
+	if (READ_ONCE(bus->monitored))
 		(*mon_ops->urb_submit_error)(bus, urb, error);
 }
 
 static inline void usbmon_urb_complete(struct usb_bus *bus, struct urb *urb,
 		int status)
 {
-	if (bus->monitored)
+	if (READ_ONCE(bus->monitored))
 		(*mon_ops->urb_complete)(bus, urb, status);
 }
 
-- 
2.51.0

Re: [PATCH v2] usbmon: mark concurrent accesses to the monitored flag

[sashiko-bot]

> USB submission and completion paths read bus->monitored without taking
> mon_lock or the monitor bus lock. Adding and removing readers changes the
> flag under those locks, so the plain accesses race, as reported by KCSAN.
> 
> Use READ_ONCE() for the fast-path checks and WRITE_ONCE() for the updates.
> The monitor callbacks still serialize access to their reader lists using
> the existing bus locks.
> 
> Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
> Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111062752.16802-1-zhujun2@cmss.chinamobile.com?part=1