From: Al Viro <viro@zeniv.linux.org.uk>
To: Jann Horn <jannh@google.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Miklos Szeredi <mszeredi@redhat.com>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
Will Deacon <will@kernel.org>
Subject: Re: [PATCH v2] fs: use acquire ordering in __fget_light()
Date: Mon, 31 Oct 2022 18:08:10 +0000 [thread overview]
Message-ID: <Y2APCmYNjYOYLf8G@ZenIV> (raw)
In-Reply-To: <20221031175256.2813280-1-jannh@google.com>
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?
next prev parent reply other threads:[~2022-10-31 18:08 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-31 17:52 [PATCH v2] fs: use acquire ordering in __fget_light() Jann Horn
2022-10-31 18:08 ` Al Viro [this message]
2022-10-31 18:13 ` Jann Horn
2022-10-31 18:48 ` Al Viro
2022-10-31 19:07 ` Linus Torvalds
2022-10-31 19:31 ` Al Viro
2022-10-31 18:45 ` Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y2APCmYNjYOYLf8G@ZenIV \
--to=viro@zeniv.linux.org.uk \
--cc=jannh@google.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mszeredi@redhat.com \
--cc=torvalds@linux-foundation.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).