bpf: Replace map types to use &self, remove HashMap::get_mut #290
Conversation
✅ Deploy Preview for aya-rs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
|
This is a breaking change as it removes I'll follow up with a change to aya-book examples as well. |
ishitatsuyuki
force-pushed
the
immut-self
branch
2 times, most recently
from
6abdfc1
to
7c924dc
Compare
|
Reformatted. |
bpf/aya-bpf/src/maps/hash_map.rs
Outdated
| @@ -229,28 +232,19 @@ const fn build_def<K, V>(ty: u32, max_entries: u32, flags: u32, pin: PinningType | |||
| } | |||
|
|
|||
| #[inline] | |||
| fn get<'a, K, V>(def: &mut bpf_map_def, key: &K) -> Option<&'a V> { | |||
| fn get<'a, K, V>(def: *mut bpf_map_def, key: &K) -> Option<&'a V> { | |||
There was a problem hiding this comment.
This is still unsound btw, since some other program might mutate the key while there's an outstanding &V.
I think we should probably have a get() -> Option<V> and then unsafe fn get_ptr() -> Option<*const V> and unsafe fn get_ptr_mut() -> Option<*mut V> for cases where soundness is guaranteed externally (eg a non-preemptable program working on a per-cpu map)
There was a problem hiding this comment.
Is it? For BPF_MAP_TYPE_HASH, "map_update_elem() replaces existing elements atomically" (I think it uses a RCU inside).
There was a problem hiding this comment.
Summarizing discord discussion: the problem is that in rust having a &V guarantees that the value referenced isn't mutated.
The BPF map API does not guarantee this in general. Maps are protected by RCU, but only BPF_F_NO_PREALLOC allocated maps would guarantee no mutation, as updates are new allocations, and old allocations are kept around until RCU synchronization. Pre-allocated maps update elements in place, so RCU doesn't help.
There was a problem hiding this comment.
I think the "tearing" unsoundness is unavoidable at this point (not sure why the kernel people decided that reading garbage value is a good idea under any circumstance), but in that case copying values doesn't help either (you can get "teared" while reading that value too). So I propose the following API instead:
unsafe fn get(&self) -> Option<&V>; // Only safe if the map is "tear-free". Let's put a large warning here
fn get_ptr(&self) -> Option<*const V>; // Unsafe is only required while dereferencing, where invariants could be violated (but there is no use-after-free)As for get_ptr_mut, I still think an UnsafeCell should be used instead (raw_get will get you decent pointer arithmetic). There are synchronization and reentrancy issues here, both should be taken care of separately. Finally, nothing stops you from looking up the same element twice.
There was a problem hiding this comment.
Maybe we should just have get that returns a Option<Cell<V>> (which could be our own Cell impl if it needs to be Sync).
Then API-wise, users would have get() for a copy of T , set() and as_ptr() 🤔
There was a problem hiding this comment.
Is get_ptr_mut any more sound than using an UnsafeCell?
There was a problem hiding this comment.
Yes? get_ptr_mut gives you a pointer, the pointer is always valid. Doesn't violate aliasing, doesn't violate the memory model. It is still a huge foot gun, but it's not unsound.
If we tell you to put UnsafeCell in a map and the cell gets written my multiple programs without using bpf_lock, that's unsound. Ok we could say in the docs this is only sound if you do this and that, but then, is it worth potentially creating unsoundness to make it slightly harder to get a *mut V? This is not a rhetorical question, this is part of what we need to decide here.
There was a problem hiding this comment.
UnsafeCell isn't compatible with any threading model, the point is that the user is intentionally specifying that they would do it unchecked anyway.
From the UnsafeCell docs: "At all times, you must avoid data races. If multiple threads have access to the same UnsafeCell, then any writes must have a proper happens-before relation to all other accesses (or use atomics)."
Which is precisely not the case in our case.
There was a problem hiding this comment.
Btw while I think we should continue this conversation, since regardless of what we do get_mut is going away, I say we should add get_ptr and merge this for now. Then we can add get_ptr_mut later if we decide we want it. This is blocking the ringbuf PR which I'd like to merge soon.
There was a problem hiding this comment.
If the docs say so, then it must be right. I'll put a get_ptr_mut function in then.
|
|
The bpf_map_defs are now wrapped with UnsafeCell, which also happens to provide a cozy way to get a *mut pointer. An UnsafeCell isn't strictly required as the struct fields are practically opaque to us, but using an UnsafeCell follows the general best practices against miscompilation and also prevents some obvious errors. HashMap::get_mut was removed because the idea is completely unsound. Previous users should wrap their data with UnsafeCell instead or use atomics. Closes: aya-rs#233
|
Thanks so much for doing this, great work! 😊 |
dave-tucker
added
feature
The bpf_map_defs are now wrapped with UnsafeCell, which also happens to
provide a cozy way to get a *mut pointer. An UnsafeCell isn't strictly
required as the struct fields are practically opaque to us, but using an
UnsafeCell follows the general best practices against miscompilation and
also prevents some obvious errors.
HashMap::get_mut was removed because the idea is completely unsound.
Previous users should wrap their data with UnsafeCell instead or use
atomics.
Closes: #233