Re: [V9fs-developer] KCSAN BUG report on p9_client_cb / p9_client_rpc

[Dominique Martinet <asmadeus@codewreck.org>]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

jim.cromie@gmail.com wrote on Tue, Jun 22, 2021 at 10:42:58AM -0600:
> I got this on rc7 + my hacks ( not near p9 )
> ISTM someone here will know what it means.
> If theres anything else i can do to help,
> (configs, drop my patches and retry)
>  please let me know

Thanks for the report!

> [   14.904783] ==================================================================
> [   14.905848] BUG: KCSAN: data-race in p9_client_cb / p9_client_rpc

hm, this code hasn't changed in ages (unless someone merged code behind
my back :D)

I had assumed the p9_req_put() in p9_client_cb would protect the tag,
but that doesn't appear to be true -- could you try this patch if this
is reproductible to you?

The tag is actually reclaimed in the woken up p9_client_rpc thread so
that would be a good match (reset in the other thread vs. read here),
caching the value is good enough but that is definitely not obvious...

-- 
Dominique

[-- Attachment #2: 0001-9p-net-cache-tag-in-p9_client_cb.patch --]
[-- Type: text/plain, Size: 1396 bytes --]

From 1135d60baa5d743e8a123812428a342b101e290e Mon Sep 17 00:00:00 2001
From: Dominique Martinet <asmadeus@codewreck.org>
Date: Wed, 23 Jun 2021 02:12:20 +0900
Subject: [PATCH] 9p net: cache tag in p9_client_cb

req->tc.tag is not safe to access after status has been set,
because tag is reclaimed by p9_client_rpc and not by the p9_req_put
below as one might think.

Reported-by: jim.cromie@gmail.com
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
 net/9p/client.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index b7b958f61faf..3e95a56ead80 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -419,7 +419,8 @@ static void p9_tag_cleanup(struct p9_client *c)
  */
 void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
 {
-	p9_debug(P9_DEBUG_MUX, " tag %d\n", req->tc.tag);
+	u16 tag = req->tc.tag;
+	p9_debug(P9_DEBUG_MUX, " tag %d\n", tag);
 
 	/*
 	 * This barrier is needed to make sure any change made to req before
@@ -429,7 +430,8 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
 	req->status = status;
 
 	wake_up(&req->wq);
-	p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc.tag);
+	/* req->tc.tag is not safe to access after status has been set */
+	p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", tag);
 	p9_req_put(req);
 }
 EXPORT_SYMBOL(p9_client_cb);
-- 
2.31.1