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
--
Cheers,
Benno
