linux-kernel - Re: [PATCH] 9p/trans_fd: mark concurrent read and writes to p9

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c4517e43-b2e2-485c-b727-7e5d47930ba0@iencinas.com>
Date: Sun, 9 Mar 2025 08:35:22 +0100
From: Ignacio Encinas Rubio <ignacio@...cinas.com>
To: Dominique Martinet <asmadeus@...ewreck.org>
Cc: linux-kernel-mentees@...ts.linux.dev, skhan@...uxfoundation.org,
 Eric Van Hensbergen <ericvh@...nel.org>, Latchesar Ionkov
 <lucho@...kov.net>, Christian Schoenebeck <linux_oss@...debyte.com>,
 Sishuai Gong <sishuai.system@...il.com>, Marco Elver <elver@...gle.com>,
 v9fs@...ts.linux.dev, linux-kernel@...r.kernel.org,
 syzbot+d69a7cc8c683c2cb7506@...kaller.appspotmail.com,
 syzbot+483d6c9b9231ea7e1851@...kaller.appspotmail.com
Subject: Re: [PATCH] 9p/trans_fd: mark concurrent read and writes to
 p9_conn->err

On 8/3/25 23:08, Dominique Martinet wrote:
> Thank for looking into it, I wasn't aware this could be enough to please
> the KCSAN gods.

Thank you for reviewing it!

> I've just gone over read/write work and I think overall the logic
> doesn't look too bad as the checks for m->err are just optimizations
> that could be skipped entierly.

That was my impression too. Thanks for confirming! 
As far as I know, this is as non-problematic as it gets. 

> So, sure, they could recheck but I don't see the point; if syzbot is
> happy with this patch I think that's good enough.

I think KCSAN shouldn't complain anymore. However, let me send a v2:

>> [1] https://lore.kernel.org/all/ZTZtHdqifXlWG8nN@codewreck.org/

I last-minute edited this snippet because it looks like it should be
like the rest of them (just a READ_ONCE, no spinlock) 

@@ -673,7 +674,7 @@ static int p9_fd_request(struct p9_client *client, struct p9_req_t *req)

 	spin_lock(&m->req_lock);

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

but as I left it, it doesn't make any sense. It's either a racy read +
READ_ONCE to make KCSAN happy or a protected read which shouldn't be a
problem. I'll just drop this hunk and leave it as it was.

>> ---
>>  net/9p/trans_fd.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
>> index 196060dc6138af10e99ad04a76ee36a11f770c65..5458e6530084cabeb01d13e9b9a4b1b8f338e494 100644
>> --- a/net/9p/trans_fd.c
>> +++ b/net/9p/trans_fd.c
>> @@ -194,9 +194,10 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
>>        if (m->err) {
> 
> This is under spin lock and I don't see the compiler reordering this
> read and write, but should this also get READ_ONCE?

It wouldn't hurt, but I don't think it would do anything. spin_lock and
spin_unlock both emit compiler barriers so that code can't be moved out
of critical sections (apart from doing actual locking, release-acquire
ordering ...). I guess the only function of a READ_ONCE here would be to
ensure atomicity of the read, but 

  1) There are no concurrent writes when this read is happening due to
  the spinlock being locked

  2) Getting the load teared is almost impossible(?) as it is an aligned
  4 byte read. Even if the load got garbage, we would just return
  without reading the actual value.

I'll wait a couple of days to send the v2 in case there is any more
feedback. 

Thanks again!