Re: [PATCH v2] 9p/trans_fd: mark concurrent read and writes to p9_conn->err

[Dominique Martinet <asmadeus@codewreck.org>]

Ignacio Encinas wrote on Thu, Mar 13, 2025 at 07:08:19PM +0100:
> Writes for the error value of a connection are spinlock-protected inside
> p9_conn_cancel, but lockless reads are present elsewhere to avoid
> performing unnecessary work after an error has been met.
> 
> Mark the write and lockless reads to make KCSAN happy. Mark the write as
> exclusive following the recommendation in "Lock-Protected Writes with
> Lockless Reads" in tools/memory-model/Documentation/access-marking.txt
> while we are at it.
> 
> Reported-by: syzbot+d69a7cc8c683c2cb7506@syzkaller.appspotmail.com
> Reported-by: syzbot+483d6c9b9231ea7e1851@syzkaller.appspotmail.com
> Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
> ---
> Changes in v2:
> 
> Drop unnecessary READ_ONCE in p9_fd_request (that I added in v1)

Ah, sorry; I think you misread my comment for v1 (or perhaps you
disagreed in the response and I misread that!)

I was thinking that style-wise it's better to access the err field
through READ/WRITE_ONCE everywhere, even if it's locked; so suggested
this diff from v1:
----
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index f163f6fc7354..65270c028f52 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -192,7 +192,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
 
        spin_lock(&m->req_lock);
 
-       if (m->err) {
+       if (READ_ONCE(m->err)) {
                spin_unlock(&m->req_lock);
                return;
        }
----

OTOH, looking at this again:
> --  if (m->err < 0) {
> -+  if (READ_ONCE(m->err) < 0) {
> -           spin_unlock(&m->req_lock);
> -           return m->err;

There's this access out of the lock so perhaps this should look like
this instead (with or without the READ_ONCE)

+       err = READ_ONCE(m->err);
+       if (err < 0) {
                spin_unlock(&m->req_lock);
-               return m->err;
+               return err;


Anyway, m->err is only written exactly once so it doesn't matter the
least in practice, and it looks like gcc generates exactly the same
thing (... even if I make that `return READ_ONCE(m->err)` which
surprises me a bit..), so this is just yak shaving.

I don't care all that much so I'll just pick this v2 as it's more
consistent, but feel free to send a v3 if you have an opinion, or if
someone else chips in.

Thanks!
-- 
Dominique