From: Boqun Feng <boqun.feng@gmail.com>
To: Benno Lossin <benno.lossin@proton.me>
Cc: "Alice Ryhl" <aliceryhl@google.com>,
"Miguel Ojeda" <ojeda@kernel.org>,
"Alex Gaynor" <alex.gaynor@gmail.com>,
"Wedson Almeida Filho" <wedsonaf@gmail.com>,
"Gary Guo" <gary@garyguo.net>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Andreas Hindborg" <a.hindborg@samsung.com>,
"Peter Zijlstra" <peterz@infradead.org>,
"Alexander Viro" <viro@zeniv.linux.org.uk>,
"Christian Brauner" <brauner@kernel.org>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Arve Hjønnevåg" <arve@android.com>,
"Todd Kjos" <tkjos@android.com>,
"Martijn Coenen" <maco@android.com>,
"Joel Fernandes" <joel@joelfernandes.org>,
"Carlos Llamas" <cmllamas@google.com>,
"Suren Baghdasaryan" <surenb@google.com>,
"Dan Williams" <dan.j.williams@intel.com>,
"Kees Cook" <keescook@chromium.org>,
"Matthew Wilcox" <willy@infradead.org>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Daniel Xu" <dxu@dxuuu.xyz>,
linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 7/7] rust: file: add abstraction for `poll_table`
Date: Tue, 12 Dec 2023 17:35:09 -0800 [thread overview]
Message-ID: <ZXkKTSTCuQMt2ge6@boqun-archlinux> (raw)
In-Reply-To: <E-jdYd0FVvs15f_pEC0Fo6k2DByCDEQoh_Ux9P9ldmC-otCvUfQghkJOUkiAi8gDI8J47wAaDe56XYC5NiJhuohyhIklGAWMvv9v1qi6yYM=@proton.me>
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
next prev parent reply other threads:[~2023-12-13 1:35 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-06 11:59 [PATCH v2 0/7] File abstractions needed by Rust Binder Alice Ryhl
2023-12-06 11:59 ` [PATCH v2 1/7] rust: file: add Rust abstraction for `struct file` Alice Ryhl
2023-12-08 9:48 ` Benno Lossin
2023-12-11 15:34 ` Alice Ryhl
2023-12-06 11:59 ` [PATCH v2 2/7] rust: cred: add Rust abstraction for `struct cred` Alice Ryhl
2023-12-08 16:13 ` Benno Lossin
2023-12-11 15:34 ` Alice Ryhl
2023-12-11 1:19 ` Boqun Feng
2023-12-11 15:34 ` Alice Ryhl
2023-12-11 17:35 ` Boqun Feng
2023-12-11 19:30 ` Benno Lossin
2023-12-12 9:40 ` Alice Ryhl
2023-12-06 11:59 ` [PATCH v2 3/7] rust: security: add abstraction for secctx Alice Ryhl
2023-12-08 16:22 ` Benno Lossin
2023-12-11 15:34 ` Alice Ryhl
2023-12-06 11:59 ` [PATCH v2 4/7] rust: file: add `FileDescriptorReservation` Alice Ryhl
2023-12-08 7:37 ` Benno Lossin
2023-12-08 7:43 ` Alice Ryhl
2023-12-11 15:34 ` Alice Ryhl
2023-12-06 11:59 ` [PATCH v2 5/7] rust: file: add `Kuid` wrapper Alice Ryhl
2023-12-06 12:34 ` Peter Zijlstra
2023-12-06 12:57 ` Alice Ryhl
2023-12-06 13:40 ` Peter Zijlstra
2023-12-06 13:50 ` Alice Ryhl
2023-12-06 16:49 ` Nick Desaulniers
2023-12-08 16:31 ` Miguel Ojeda
2023-12-08 16:57 ` Peter Zijlstra
2023-12-08 18:18 ` Kees Cook
2023-12-08 20:45 ` Peter Zijlstra
2023-12-08 20:57 ` Kees Cook
2023-12-11 21:13 ` Kent Overstreet
2023-12-08 16:40 ` Benno Lossin
2023-12-08 16:43 ` Boqun Feng
2023-12-11 15:58 ` Kent Overstreet
2023-12-11 17:04 ` Benno Lossin
2023-12-11 15:34 ` Alice Ryhl
2023-12-06 11:59 ` [PATCH v2 6/7] rust: file: add `DeferredFdCloser` Alice Ryhl
2023-12-08 17:39 ` Benno Lossin
2023-12-11 15:34 ` Alice Ryhl
2023-12-11 17:23 ` Benno Lossin
2023-12-12 9:35 ` Alice Ryhl
2023-12-12 16:50 ` Benno Lossin
2023-12-11 17:41 ` Boqun Feng
2023-12-12 1:25 ` Boqun Feng
2023-12-12 20:57 ` Boqun Feng
2023-12-13 11:04 ` Alice Ryhl
2023-12-06 11:59 ` [PATCH v2 7/7] rust: file: add abstraction for `poll_table` Alice Ryhl
2023-12-08 17:53 ` Benno Lossin
2023-12-12 9:59 ` Alice Ryhl
2023-12-12 17:01 ` Benno Lossin
2023-12-13 1:35 ` Boqun Feng [this message]
2023-12-13 9:12 ` Benno Lossin
2023-12-13 10:09 ` Alice Ryhl
2023-12-13 17:05 ` Boqun Feng
2023-12-13 11:02 ` Alice Ryhl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZXkKTSTCuQMt2ge6@boqun-archlinux \
--to=boqun.feng@gmail.com \
--cc=a.hindborg@samsung.com \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=arve@android.com \
--cc=benno.lossin@proton.me \
--cc=bjorn3_gh@protonmail.com \
--cc=brauner@kernel.org \
--cc=cmllamas@google.com \
--cc=dan.j.williams@intel.com \
--cc=dxu@dxuuu.xyz \
--cc=gary@garyguo.net \
--cc=gregkh@linuxfoundation.org \
--cc=joel@joelfernandes.org \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maco@android.com \
--cc=ojeda@kernel.org \
--cc=peterz@infradead.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=surenb@google.com \
--cc=tglx@linutronix.de \
--cc=tkjos@android.com \
--cc=viro@zeniv.linux.org.uk \
--cc=wedsonaf@gmail.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).