linux-kernel - Re: [PATCH v2] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z9no3dnDt2mkd2MJ@codewreck.org>
Date: Wed, 19 Mar 2025 06:42:53 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Ignacio Encinas Rubio <ignacio@...cinas.com>
Cc: Ignacio Encinas Rubio <ignacio.encinas@...idynamics.com>,
	linux-kernel-mentees@...ts.linux.dev, v9fs@...ts.linux.dev,
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org
Subject: Re: [PATCH v2] 9p/trans_fd: mark concurrent read and writes to
 p9_conn->err

Ignacio Encinas Rubio wrote on Tue, Mar 18, 2025 at 10:21:52PM +0100:
> Trimming CC to avoid spamming people (I hope that's ok)

Probably fine :)

> > This is weird... I'll double check because it shouldn't generate the
> > same code as far as I know.
> 
> I had a bit of time to check this. I understood you said that (A)
> 
> 	err = READ_ONCE(m->err);
> 	if (err < 0) {
> 		spin_unlock(&m->req_lock);
> 		return READ_ONCE(m->err);
> 	}
> 
> compiles to the same thing as (B)
> 
> 	err = READ_ONCE(m->err);
> 	if (err < 0) {
> 		spin_unlock(&m->req_lock);
> 		return err;
> 	}
> 
> if you didn't say this, just ignore this email :).

Right, I checked by looking at `objdump -D` output after rebuilding the
.o.
I've just double-checked and these two are identical:
```
err = READ_ONCE(m->err);
if (err < 0) {
    spin_unlock(&m->req_lock);
    return m->err;
}
```
```
err = READ_ONCE(m->err);
if (err < 0) {
    spin_unlock(&m->req_lock);
    return READ_ONCE(m->err);
}
```
but now I'm double-checking returning the local variable
```
```
err = READ_ONCE(m->err);
if (err < 0) {
    spin_unlock(&m->req_lock);
    return err;
}
```
is properly different (and is the right thing to do), so I must have
checked the wrong .o, sorry for the confusion.
(that or the minor gcc upgrade I did this morning change this, but I'd
rather inclined to think I screwed up...)

> With gcc (GCC) 14.2.1 20250110 (Red Hat 14.2.1-7) I'm seeing a difference:
> (A) performs another memory read after the spinlock has been unlocked
> while (B) reuses the value from the register. If you're using an old GCC
> it might have bugs. I can't recall where exactly but I have seen links
> to GCC bugs regarding this issues somewhere (LWN posts or kernel docs?)

Right, I'm seeing one less mov as well

So, yeah, v3 with this would be great :)
I don't have a strong opinion on the READ_ONCE within the lock, so if
you think it's clearer without it then I'm fine with that.

Thanks for taking the time to check!
-- 
Dominique Martinet | Asmadeus