Atomics are not needed to read the variables after NET_EPOCH_ENTER due
to the embedded acquire barrier in epoch_enter.
Atomics are also not needed to write the variables prior to
NET_EPOCH_WAIT due to the embedded memory fence in epoch_wait.
Differential D35989
wg: Use plain loads and stores for so_so4 and so_so6. jhb on Jul 29 2022, 4:57 PM. Authored by Tags None Referenced Files
Details
Atomics are not needed to read the variables after NET_EPOCH_ENTER due Atomics are also not needed to write the variables prior to
Diff Detail
Event TimelineComment Actions Hi @jhb , Comment Actions This is a diff I've primarily uploaded for markj@ to review. I have a tree that imports the wg(4) driver from wireguard-freebsd into sys/dev/wg and this change (along with others in the stack) are further changes to the current wireguard-freebsd driver. Comment Actions I don't see how the fences have anything to do with it. The point of atomic load/stores is so that the compiler does not get ideas of splitting them into multiple instructions and does not get ideas about re-reading them, both of which are no longer guaranteed with the patch at hand. Comment Actions That assumption is not valid, although it almost always works out. What is a serious problem is allowing the compiler to decide to re-read these values, meaning multiple uses of 'so4' in the same func can end up with different values. While the sample above is so short I don't believe any compiler would do it, I see 0 reason to make the proposed change. If anything, it is better to add these loads if in any doubt than to skip them. Comment Actions It's valid for the platforms FreeBSD runs on, this assumption is pervasive in the kernel. (Try enabling KCSAN.) Above you say that the purpose of atomic_load/store is to provide atomicity of operations, while below you say it's to ensure that values are read or written exactly once so as to satisfy assumptions of some lockfree code. A third reason is to provide sanitizers with a list of "known" unserialized memory accesses.
Looking at the code again, I agree. The accesses in wg_send() should use atomic_load to ensure that the compiler does not reload a socket pointer after it's compared with NULL. The accesses in wg_socket_set() are serialized by the softc mutex, so there we are merely assuming that the stores are atomic and atomic_store does not accomplish anything except educating KCSAN. Clearly I didn't look carefully enough when I reviewed the first time, sorry about that. We do not have a consistent policy on whether to use atomics when performing serialized stores to memory which is accessed in unserialized contexts, such as here. KCSAN (as it is implemented in FreeBSD, though it's easy to change) and Linux require both sides to use atomics (READ_ONCE/WRITE_ONCE). So, what should we do in general? Comment Actions I see part of the point was lost, so I'm going to quote myself:
(point one of two) So it is not about architectures getting torn loads/stores when the compiler emitted a load/store to a properly aligned var. It is about a hypothetical possibility of the compiler splitting the op -- for example generating 8 one-byte loads instead of 1 eight-byte load. While I never ran into such insanity myself, I do know it happened in the Linux land.
What we should do is play it safe and make it a policy that unlocked loads and stores always use atomic_* primitives. To that end type checking needs to be added with atomic vars becoming opaque so that accidental direct access fails to compile. This is easy for all types except for pointers, which will perhaps require some hackery to retain checking for the intended pointed-to-type apart from it being accessed with atomic. Comment Actions Actually, this is not hard if we just use C11 atomics and use _Atomic directly, even for pointers.
|