| Message ID | 20220805154231.31257-7-ojeda@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rust support | expand |
On Fri, Aug 05, 2022 at 05:41:51PM +0200, Miguel Ojeda wrote: > This source file contains forwarders to C macros and inlined > functions. Perhaps: "Introduce the source file that will contain forwarders to common C macros as inlined Rust functions. Initially this only contains type size asserts, but will gain more helpers in subsequent patches." > > Co-developed-by: Alex Gaynor <alex.gaynor@gmail.com> > Signed-off-by: Alex Gaynor <alex.gaynor@gmail.com> > Co-developed-by: Geoffrey Thomas <geofft@ldpreload.com> > Signed-off-by: Geoffrey Thomas <geofft@ldpreload.com> > Co-developed-by: Wedson Almeida Filho <wedsonaf@google.com> > Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com> > Co-developed-by: Sven Van Asbroeck <thesven73@gmail.com> > Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com> > Co-developed-by: Gary Guo <gary@garyguo.net> > Signed-off-by: Gary Guo <gary@garyguo.net> > Co-developed-by: Boqun Feng <boqun.feng@gmail.com> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > Co-developed-by: Maciej Falkowski <m.falkowski@samsung.com> > Signed-off-by: Maciej Falkowski <m.falkowski@samsung.com> > Co-developed-by: Wei Liu <wei.liu@kernel.org> > Signed-off-by: Wei Liu <wei.liu@kernel.org> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > --- > rust/helpers.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > create mode 100644 rust/helpers.c > > diff --git a/rust/helpers.c b/rust/helpers.c > new file mode 100644 > index 000000000000..b4f15eee2ffd > --- /dev/null > +++ b/rust/helpers.c > @@ -0,0 +1,51 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Non-trivial C macros cannot be used in Rust. Similarly, inlined C functions > + * cannot be called either. This file explicitly creates functions ("helpers") > + * that wrap those so that they can be called from Rust. > + * > + * Even though Rust kernel modules should never use directly the bindings, some > + * of these helpers need to be exported because Rust generics and inlined > + * functions may not get their code generated in the crate where they are > + * defined. Other helpers, called from non-inline functions, may not be > + * exported, in principle. However, in general, the Rust compiler does not > + * guarantee codegen will be performed for a non-inline function either. > + * Therefore, this file exports all the helpers. In the future, this may be > + * revisited to reduce the number of exports after the compiler is informed > + * about the places codegen is required. > + * > + * All symbols are exported as GPL-only to guarantee no GPL-only feature is > + * accidentally exposed. > + */ > + > +#include <linux/bug.h> > +#include <linux/build_bug.h> > + > +__noreturn void rust_helper_BUG(void) > +{ > + BUG(); > +} > +EXPORT_SYMBOL_GPL(rust_helper_BUG); Given the distaste for ever using BUG()[1], why does this helper exist? > + > +/* > + * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type > + * as the Rust `usize` type, so we can use it in contexts where Rust > + * expects a `usize` like slice (array) indices. `usize` is defined to be > + * the same as C's `uintptr_t` type (can hold any pointer) but not > + * necessarily the same as `size_t` (can hold the size of any single > + * object). Most modern platforms use the same concrete integer type for > + * both of them, but in case we find ourselves on a platform where > + * that's not true, fail early instead of risking ABI or > + * integer-overflow issues. > + * > + * If your platform fails this assertion, it means that you are in > + * danger of integer-overflow bugs (even if you attempt to remove > + * `--size_t-is-usize`). It may be easiest to change the kernel ABI on > + * your platform such that `size_t` matches `uintptr_t` (i.e., to increase > + * `size_t`, because `uintptr_t` has to be at least as big as `size_t`). > + */ > +static_assert( > + sizeof(size_t) == sizeof(uintptr_t) && > + __alignof__(size_t) == __alignof__(uintptr_t), > + "Rust code expects C `size_t` to match Rust `usize`" > +); -Kees [1] https://docs.kernel.org/process/deprecated.html#bug-and-bug-on
On Wed, Aug 17, 2022 at 9:44 PM Kees Cook <keescook@chromium.org> wrote: > > "Introduce the source file that will contain forwarders to common C > macros as inlined Rust functions. Initially this only contains type > size asserts, but will gain more helpers in subsequent patches." Yeah, I will reword it, it doesn't make as much sense now that it is trimmed. > Given the distaste for ever using BUG()[1], why does this helper exist? We use it exclusively for the Rust panic handler, which does not return (we use fallible operations as much as possible, of course, but we need to provide a panic handler nevertheless). Killing the entire machine is definitely too aggressive for some setups/situations, so at some point last year we discussed potential alternatives (e.g. `make_task_dead()` or similar) with, if I recall correctly, Greg. Maybe we want to make it configurable too. We are open to suggestions! Cheers, Miguel
On Wed, Aug 17, 2022 at 10:22:37PM +0200, Miguel Ojeda wrote: > On Wed, Aug 17, 2022 at 9:44 PM Kees Cook <keescook@chromium.org> wrote: > > Given the distaste for ever using BUG()[1], why does this helper exist? > > We use it exclusively for the Rust panic handler, which does not > return (we use fallible operations as much as possible, of course, but > we need to provide a panic handler nevertheless). Gotcha -- it's for the implicit situations (e.g. -C overflow-checks=on), nothing is expected to explicitly call the Rust panic handler? > Killing the entire machine is definitely too aggressive for some > setups/situations, so at some point last year we discussed potential > alternatives (e.g. `make_task_dead()` or similar) with, if I recall > correctly, Greg. Maybe we want to make it configurable too. We are > open to suggestions! I suffer the same problems trying to fix C and the old "can never fail" interfaces. Mainly we've just been systematically replacing such APIs with APIs that return error codes, allowing the error to bubble back up. (Which I know is exactly what you've already done with the allocator, etc. Yay!) -Kees
On Wed, Aug 17, 2022 at 10:34 PM Kees Cook <keescook@chromium.org> wrote: > > Gotcha -- it's for the implicit situations (e.g. -C overflow-checks=on), Yeah, exactly. > nothing is expected to explicitly call the Rust panic handler? If by explicitly you mean calling `panic!()`, then in the `kernel` crate in the v9 patches there is none. Though we may want to call it in the future (we have 4 instances in the full code not submitted here, e.g. for mismatching an independent lock guard with its owner). They can be avoided depending on how we want the design to be and, I guess, what the "Rust panic" policy will finally be (i.e. `BUG()` or something softer). Outside the `kernel` crate, there are also instances in proc macros and Rust hostprogs/scripts (compilation-time in the host), in the `alloc` crate (compiled-out) and in the `compiler_builtins` crate (for e.g. `u128` support that eventually we would like to not see compiled-in). Cheers, Miguel
On Wed, Aug 17, 2022 at 11:44:53PM +0200, Miguel Ojeda wrote: > On Wed, Aug 17, 2022 at 10:34 PM Kees Cook <keescook@chromium.org> wrote: > > > > Gotcha -- it's for the implicit situations (e.g. -C overflow-checks=on), > > Yeah, exactly. > > > nothing is expected to explicitly call the Rust panic handler? > > If by explicitly you mean calling `panic!()`, then in the `kernel` > crate in the v9 patches there is none. Perfect. It may be worth stating this explicitly with the helper. i.e. "This is for handling any panic!() calls in core Rust, but should not ever be used in the 'kernel' create; failures should be handled." > Though we may want to call it in the future (we have 4 instances in > the full code not submitted here, e.g. for mismatching an independent > lock guard with its owner). They can be avoided depending on how we > want the design to be and, I guess, what the "Rust panic" policy will > finally be (i.e. `BUG()` or something softer). > > Outside the `kernel` crate, there are also instances in proc macros > and Rust hostprogs/scripts (compilation-time in the host), in the > `alloc` crate (compiled-out) and in the `compiler_builtins` crate (for > e.g. `u128` support that eventually we would like to not see > compiled-in). Sounds good! -Kees
On Thu, Aug 18, 2022 at 1:56 AM Kees Cook <keescook@chromium.org> wrote: > > Perfect. It may be worth stating this explicitly with the helper. i.e. > "This is for handling any panic!() calls in core Rust, but should not > ever be used in the 'kernel' create; failures should be handled." I am not sure we should say "ever", because there are sometimes situations where we statically know a situation is impossible. Of course, "impossible" in practice is possible -- even if it is due to a single-event upset. For the "statically impossible" cases, we could simply trigger UB instead of panicking. However, while developing and debugging one would like to detect bugs as soon as possible. Moreover, in production, people may have use cases where killing the world is better as soon as anything "funny" is detected, no matter what. So we could make it configurable, so that "Rust statically impossible panics" can be defined as UB, `make_task_dead()` or a full `BUG()`. By the way, I should have mentioned the `unwrap()s` too, since they are pretty much explicit panics. We don't have any in v9 either, but we do have a couple dozens in the full code (in the 97% not submitted) in non-test or examples code. Many are of the "statically impossible" kind, but any that is not merits some discussion, which we can do as we upstream the different pieces. Cheers, Miguel
On Thu, Aug 18, 2022 at 06:03:04PM +0200, Miguel Ojeda wrote: > On Thu, Aug 18, 2022 at 1:56 AM Kees Cook <keescook@chromium.org> wrote: > > > > Perfect. It may be worth stating this explicitly with the helper. i.e. > > "This is for handling any panic!() calls in core Rust, but should not > > ever be used in the 'kernel' create; failures should be handled." > > I am not sure we should say "ever", because there are sometimes > situations where we statically know a situation is impossible. Of > course, "impossible" in practice is possible -- even if it is due to a > single-event upset. > > For the "statically impossible" cases, we could simply trigger UB > instead of panicking. However, while developing and debugging one > would like to detect bugs as soon as possible. Moreover, in > production, people may have use cases where killing the world is > better as soon as anything "funny" is detected, no matter what. Please, no UB. I will take a panic over UB any day. It'd be best to handle things with some error path, but those are the rare exception. > So we could make it configurable, so that "Rust statically impossible > panics" can be defined as UB, `make_task_dead()` or a full `BUG()`. C is riddled with UB and it's just terrible. Let's make sure we don't continue that mistake. :) > By the way, I should have mentioned the `unwrap()s` too, since they > are pretty much explicit panics. We don't have any in v9 either, but > we do have a couple dozens in the full code (in the 97% not submitted) > in non-test or examples code. Many are of the "statically impossible" > kind, but any that is not merits some discussion, which we can do as > we upstream the different pieces. The simple answer is that if an "impossible" situation can be recovered from, it should error instead of panic. As long as that's the explicit design goal, I think we're good. Yes there will be cases where it is really and truly unrecoverable, but those will be rare and can be well documented.
On Thu, Aug 18, 2022 at 6:08 PM Kees Cook <keescook@chromium.org> wrote: > > Please, no UB. I will take a panic over UB any day. It'd be best to > handle things with some error path, but those are the rare exception. > > C is riddled with UB and it's just terrible. Let's make sure we don't > continue that mistake. :) I definitely agree on avoiding UB :) > The simple answer is that if an "impossible" situation can be recovered > from, it should error instead of panic. As long as that's the explicit > design goal, I think we're good. Yes there will be cases where it is > really and truly unrecoverable, but those will be rare and can be well > documented. Yeah, that is the goal and we always take that into account, but there are always tricky cases which is best to consider case-by-case. Cheers, Miguel
diff --git a/rust/helpers.c b/rust/helpers.c new file mode 100644 index 000000000000..b4f15eee2ffd --- /dev/null +++ b/rust/helpers.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Non-trivial C macros cannot be used in Rust. Similarly, inlined C functions + * cannot be called either. This file explicitly creates functions ("helpers") + * that wrap those so that they can be called from Rust. + * + * Even though Rust kernel modules should never use directly the bindings, some + * of these helpers need to be exported because Rust generics and inlined + * functions may not get their code generated in the crate where they are + * defined. Other helpers, called from non-inline functions, may not be + * exported, in principle. However, in general, the Rust compiler does not + * guarantee codegen will be performed for a non-inline function either. + * Therefore, this file exports all the helpers. In the future, this may be + * revisited to reduce the number of exports after the compiler is informed + * about the places codegen is required. + * + * All symbols are exported as GPL-only to guarantee no GPL-only feature is + * accidentally exposed. + */ + +#include <linux/bug.h> +#include <linux/build_bug.h> + +__noreturn void rust_helper_BUG(void) +{ + BUG(); +} +EXPORT_SYMBOL_GPL(rust_helper_BUG); + +/* + * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type + * as the Rust `usize` type, so we can use it in contexts where Rust + * expects a `usize` like slice (array) indices. `usize` is defined to be + * the same as C's `uintptr_t` type (can hold any pointer) but not + * necessarily the same as `size_t` (can hold the size of any single + * object). Most modern platforms use the same concrete integer type for + * both of them, but in case we find ourselves on a platform where + * that's not true, fail early instead of risking ABI or + * integer-overflow issues. + * + * If your platform fails this assertion, it means that you are in + * danger of integer-overflow bugs (even if you attempt to remove + * `--size_t-is-usize`). It may be easiest to change the kernel ABI on + * your platform such that `size_t` matches `uintptr_t` (i.e., to increase + * `size_t`, because `uintptr_t` has to be at least as big as `size_t`). + */ +static_assert( + sizeof(size_t) == sizeof(uintptr_t) && + __alignof__(size_t) == __alignof__(uintptr_t), + "Rust code expects C `size_t` to match Rust `usize`" +);