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

[PATCH] 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: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
---
 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] usbmon: mark concurrent accesses to the monitored flag

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] usbmon: unresolvable Fixes trailer SHA 1da177e4c3f4 in commit
  message
--

commit 68910eb830ad82fcbb29334f2601dba719edc500
Author: Zhu Jun <zhujun2@cmss.chinamobile.com>

> usbmon: mark concurrent accesses to the monitored flag
>
> 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: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>

[Severity: Low]
The commit SHA in the Fixes: trailer cannot be resolved in the repository
because the 12-character prefix 1da177e4c3f4 is ambiguous (the initial git
commit requires at least 13 characters, 1da177e4c3f41 ("Linux-2.6.12-rc2")).

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