On Sat, Oct 21, 2023 at 01:48:28PM +0000, Benno Lossin wrote:
> On 20.10.23 02:52, Boqun Feng wrote:
> > On Thu, Oct 19, 2023 at 02:30:56PM +0000, Benno Lossin wrote:
> > [...]
> >>> + let inode =
> >>> + ptr::NonNull::new(unsafe { bindings::iget_locked(self.0.get(), ino) }).ok_or(ENOMEM)?;
> >>> +
> >>> + // SAFETY: `inode` is valid for read, but there could be concurrent writers (e.g., if it's
> >>> + // an already-initialised inode), so we use `read_volatile` to read its current state.
> >>> + let state = unsafe { ptr::read_volatile(ptr::addr_of!((*inode.as_ptr()).i_state)) };
> >>
> >> Are you sure that `read_volatile` is sufficient for this use case? The
> >> documentation [1] clearly states that concurrent write operations are still
> >> UB:
> >>
> >> Just like in C, whether an operation is volatile has no bearing
> >> whatsoever on questions involving concurrent access from multiple
> >> threads. Volatile accesses behave exactly like non-atomic accesses in
> >> that regard. In particular, a race between a read_volatile and any
> >> write operation to the same location is undefined behavior.
> >>
> >
> > Right, `read_volatile` can have data race. I think what we can do here
> > is:
> >
> > // SAFETY: `i_state` in `inode` is `unsigned long`, therefore
> > // it's safe to treat it as `AtomicUsize` and do a relaxed read.
> > let state = unsafe { *(ptr::addr_of!((*inode.as_ptr()).i_state).cast::<AtomicUsize>()).load(Relaxed) };
>
> I am not sure if that is enough. What kind of writes happen
> concurrently on the C side? If they are atomic, then this should
> be fine, if they are not synchronized at all, then it could be
> problematic, as miri says that it is still UB:
> https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=aa75fb6805c8d67ade8837531a2096d0
>
You're not wrong, my suggestion here had the assumption that write part
of ->i_state is atomic (I hadn't look into that). Now a quick look tells
it isn't, for example in fs/f2fs/namei.c, there is:
inode->i_state |= I_LINKABLE;
so I think we need to take the inode->i_lock here for a data-race free
solution. Or if we have something like:
https://github.com/rust-lang/unsafe-code-guidelines/issues/321
in Rust.
Benno, notice my reasoning about whether a write is atomic is less
strict, since in C side, in the current rule of the kernel, plain
writes to machine words can be treated as atomic, in case you're
interested CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC is the pointer ;-)
While we are at it, adding Marco, could kcsan work for Rust code? If I
understand correctly, as long as Rust compilers could generate these
__tsan_* instrument functions, it should work, right?
Regards,
Boqun
> --
> Cheers,
> Benno
