linux-kernel - Re: [PATCH v2] fs: use acquire ordering in __fget

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y2APCmYNjYOYLf8G@ZenIV>
Date:   Mon, 31 Oct 2022 18:08:10 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Jann Horn <jannh@...gle.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Miklos Szeredi <mszeredi@...hat.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Will Deacon <will@...nel.org>
Subject: Re: [PATCH v2] fs: use acquire ordering in __fget_light()

On Mon, Oct 31, 2022 at 06:52:56PM +0100, Jann Horn wrote:
> We must prevent the CPU from reordering the files->count read with the
> FD table access like this, on architectures where read-read reordering is
> possible:
> 
>     files_lookup_fd_raw()
>                                   close_fd()
>                                   put_files_struct()
>     atomic_read(&files->count)
> 
> I would like to mark this for stable, but the stable rules explicitly say
> "no theoretical races", and given that the FD table pointer and
> files->count are explicitly stored in the same cacheline, this sort of
> reordering seems quite unlikely in practice...

Looks sane, but looking at the definition of atomic_read_acquire...  ouch.

static __always_inline int
atomic_read_acquire(const atomic_t *v)
{
        instrument_atomic_read(v, sizeof(*v));
	return arch_atomic_read_acquire(v);
}

OK...

; git grep -n -w arch_atomic_read_acquire
include/linux/atomic/atomic-arch-fallback.h:220:#ifndef arch_atomic_read_acquire
include/linux/atomic/atomic-arch-fallback.h:222:arch_atomic_read_acquire(const atomic_t *v)
include/linux/atomic/atomic-arch-fallback.h:235:#define arch_atomic_read_acquire arch_atomic_read_acquire
include/linux/atomic/atomic-instrumented.h:35:  return arch_atomic_read_acquire(v);
include/linux/atomic/atomic-long.h:529: return arch_atomic_read_acquire(v);

No arch-specific instances, so...
static __always_inline int
arch_atomic_read_acquire(const atomic_t *v)
{
	int ret;

	if (__native_word(atomic_t)) {
		ret = smp_load_acquire(&(v)->counter);
	} else {
		ret = arch_atomic_read(v);
		__atomic_acquire_fence();
	}

	return ret;
}

OK, but when would that test not be true?  We have unconditional
typedef struct {
        int counter;
} atomic_t;
and
#define __native_word(t) \
        (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
         sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))

Do we really have any architectures where a structure with one
int field does *not* have a size that would satisfy that check?

Is it future-proofing for masturbation sake, or am I missing something
real here?