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) };
Regards,
Boqun
> [1]: https://doc.rust-lang.org/core/ptr/fn.read_volatile.html
>
> --
> Cheers,
> Benno
>
