Messages in this thread | | | Date | Tue, 12 Dec 2023 17:35:09 -0800 | From | Boqun Feng <> | Subject | Re: [PATCH v2 7/7] rust: file: add abstraction for `poll_table` |
| |
On Tue, Dec 12, 2023 at 05:01:28PM +0000, Benno Lossin wrote: > On 12/12/23 10:59, Alice Ryhl wrote: > > On Fri, Dec 8, 2023 at 6:53 PM Benno Lossin <benno.lossin@proton.me> wrote: > >> On 12/6/23 12:59, Alice Ryhl wrote: > >>> + fn get_qproc(&self) -> bindings::poll_queue_proc { > >>> + let ptr = self.0.get(); > >>> + // SAFETY: The `ptr` is valid because it originates from a reference, and the `_qproc` > >>> + // field is not modified concurrently with this call since we have an immutable reference. > >> > >> This needs an invariant on `PollTable` (i.e. `self.0` is valid). > > > > How would you phrase it? > > - `self.0` contains a valid `bindings::poll_table`. > - `self.0` is only modified via references to `Self`. > > >>> + unsafe { (*ptr)._qproc } > >>> + } > >>> + > >>> + /// Register this [`PollTable`] with the provided [`PollCondVar`], so that it can be notified > >>> + /// using the condition variable. > >>> + pub fn register_wait(&mut self, file: &File, cv: &PollCondVar) { > >>> + if let Some(qproc) = self.get_qproc() { > >>> + // SAFETY: The pointers to `self` and `file` are valid because they are references. > >> > >> What about cv.wait_list... > > > > I can add it to the list of things that are valid due to references. >
Actually, there is an implied safety requirement here, it's about how qproc is implemented. As we can see, PollCondVar::drop() will wait for a RCU grace period, that means the waiter (a file or something) has to use RCU to access the cv.wait_list, otherwise, the synchronize_rcu() in PollCondVar::drop() won't help.
To phrase it, it's more like:
(in the safety requirement of `PollTable::from_ptr` and the type invariant of `PollTable`):
", further, if the qproc function in poll_table publishs the pointer of the wait_queue_head, it must publish it in a way that reads on the published pointer have to be in an RCU read-side critical section."
and here we can said,
"per type invariant, `qproc` cannot publish `cv.wait_list` without proper RCU protection, so it's safe to use `cv.wait_list` here, and with the synchronize_rcu() in PollCondVar::drop(), free of the wait_list will be delayed until all usages are done."
I know, this is quite verbose, but just imagine some one removes the rcu_read_lock() and rcu_read_unlock() in ep_remove_wait_queue(), the poll table from epoll (using ep_ptable_queue_proc()) is still valid one according to the current safety requirement, but now there is a use-after-free in the following case:
CPU 0 CPU1 ep_remove_wait_queue(): struct wait_queue_head *whead; whead = smp_load_acquire(...); if (whead) { // not null PollCondVar::drop(): __wake_pollfree(); synchronize_rcu(); // no current RCU readers, yay. <free the wait_queue_head> remove_wait_queue(whead, ...); // BOOM, use-after-free
Regards, Boqun
| |