Re: [PATCH v2] fs: use acquire ordering in __fget_light()

[Al Viro]

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?