[RFC PATCH] smb: client: fix data races in STATS2 per-command timing fields

[RFC PATCH] smb: client: fix data races in STATS2 per-command timing fields

[Frank Sorenson]

KCSAN reports concurrent read/write races on server->time_per_cmd[],
server->slowest_cmd[], and server->fastest_cmd[] inside __release_mid().

These debug counters are updated by every thread completing an smb2
request without holding any lock, which can lead to inaccurate
accumulated totals and min/max values.

Reduce the data races by protecting the statistics updates:
 - Convert time_per_cmd[] to atomic64_t and use atomic64_add().
 - Use try_cmpxchg_relaxed() retry loops to safely update
   slowest_cmd[] and fastest_cmd[]
 - Use WRITE_ONCE() for the initial assignment on first completion,
   accepting the narrow race which might overwrite the very first
   statistic.

Update cifs_stats_proc_show() and cifs_stats_proc_write() to use the
corresponding atomic and READ_ONCE/WRITE_ONCE accessors.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
Hi all,

I'm sending this as an RFC because I'd like guidance on the preferred
approach for fixing these STATS2 data races.

This patch fixes the races by using atomic64_t and cmpxchg loops for
higher accuracy of the timing statistics, with a slight overhead
due to the use of another atomic in a hot path.

Obviously, locking is out of the question here, so the alternatives
I see are:

1. accept this patch to prioritize accuracy; the other two counter
   fields in STATS2 are already atomic, so this makes the third
   counter match.  And the try_cmpxchg_relaxed loops for
   slowest_cmd and fastest_cmd should be exercised infrequently, so
   overhead should be minimal.

2. change nothing about the logic, but wrap the existing bare
   assignments in WRITE_ONCE()/READ_ONCE() and accept that
   concurrent updates will sometimes silently clobber each
   other.

Let me know which approach fits best, and I'll submit a formal
v1.

 fs/smb/client/cifs_debug.c | 12 ++++++------
 fs/smb/client/cifsglob.h   |  2 +-
 fs/smb/client/transport.c  | 24 +++++++++++++++++-------
 3 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index 3761d3ad6088..c354a554853a 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -752,9 +752,9 @@ static ssize_t cifs_stats_proc_write(struct file *file,
 			for (i = 0; i < NUMBER_OF_SMB2_COMMANDS; i++) {
 				atomic_set(&server->num_cmds[i], 0);
 				atomic_set(&server->smb2slowcmd[i], 0);
-				server->time_per_cmd[i] = 0;
-				server->slowest_cmd[i] = 0;
-				server->fastest_cmd[i] = 0;
+				atomic64_set(&server->time_per_cmd[i], 0);
+				WRITE_ONCE(server->slowest_cmd[i], 0);
+				WRITE_ONCE(server->fastest_cmd[i], 0);
 			}
 #endif /* CONFIG_CIFS_STATS2 */
 			list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
@@ -826,9 +826,9 @@ static int cifs_stats_proc_show(struct seq_file *m, void *v)
 		for (j = 0; j < NUMBER_OF_SMB2_COMMANDS; j++)
 			seq_printf(m, "  %d\t\t%d\t%llu\t\t%u\t%u\n", j,
 				atomic_read(&server->num_cmds[j]),
-				server->time_per_cmd[j],
-				server->fastest_cmd[j],
-				server->slowest_cmd[j]);
+				(unsigned long long)atomic64_read(&server->time_per_cmd[j]),
+				READ_ONCE(server->fastest_cmd[j]),
+				READ_ONCE(server->slowest_cmd[j]));
 		for (j = 0; j < NUMBER_OF_SMB2_COMMANDS; j++)
 			if (atomic_read(&server->smb2slowcmd[j])) {
 				spin_lock(&server->srv_lock);
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 5c5b76a9e9fe..96d945627548 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -772,7 +772,7 @@ struct TCP_Server_Info {
 #ifdef CONFIG_CIFS_STATS2
 	atomic_t num_cmds[NUMBER_OF_SMB2_COMMANDS]; /* total requests by cmd */
 	atomic_t smb2slowcmd[NUMBER_OF_SMB2_COMMANDS]; /* count resps > 1 sec */
-	__u64 time_per_cmd[NUMBER_OF_SMB2_COMMANDS]; /* total time per cmd */
+	atomic64_t time_per_cmd[NUMBER_OF_SMB2_COMMANDS]; /* total time per cmd */
 	__u32 slowest_cmd[NUMBER_OF_SMB2_COMMANDS];
 	__u32 fastest_cmd[NUMBER_OF_SMB2_COMMANDS];
 #endif /* STATS2 */
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 6e21b5f8754a..437fc08dbfb5 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -67,16 +67,26 @@ void __release_mid(struct TCP_Server_Info *server, struct mid_q_entry *midEntry)
 
 	if (smb_cmd < NUMBER_OF_SMB2_COMMANDS) {
 		if (atomic_read(&server->num_cmds[smb_cmd]) == 0) {
-			server->slowest_cmd[smb_cmd] = roundtrip_time;
-			server->fastest_cmd[smb_cmd] = roundtrip_time;
+			/* First completion for this command: initialize min/max. */
+			WRITE_ONCE(server->slowest_cmd[smb_cmd], roundtrip_time);
+			WRITE_ONCE(server->fastest_cmd[smb_cmd], roundtrip_time);
 		} else {
-			if (server->slowest_cmd[smb_cmd] < roundtrip_time)
-				server->slowest_cmd[smb_cmd] = roundtrip_time;
-			else if (server->fastest_cmd[smb_cmd] > roundtrip_time)
-				server->fastest_cmd[smb_cmd] = roundtrip_time;
+			u32 old;
+
+			old = READ_ONCE(server->slowest_cmd[smb_cmd]);
+			while (old < roundtrip_time &&
+			       !try_cmpxchg_relaxed(&server->slowest_cmd[smb_cmd],
+			       &old, roundtrip_time))
+				;
+
+			old = READ_ONCE(server->fastest_cmd[smb_cmd]);
+			while (old > roundtrip_time &&
+			       !try_cmpxchg_relaxed(&server->fastest_cmd[smb_cmd],
+			       &old, roundtrip_time))
+				;
 		}
 		cifs_stats_inc(&server->num_cmds[smb_cmd]);
-		server->time_per_cmd[smb_cmd] += roundtrip_time;
+		atomic64_add(roundtrip_time, &server->time_per_cmd[smb_cmd]);
 	}
 	/*
 	 * commands taking longer than one second (default) can be indications
-- 
2.55.0