rust/kernel/of: construct `of_match_table` structure at build time #380

TheSven73 · 2021-06-15T20:40:50Z

Use const generics and const fns to construct our of_match_table
structure at build time. A few runtime errors now become build time
errors, which is a good thing.

To avoid the const generics propagating into the platdev's
Registration structure definition, use a dyn trait to pass
the table into the registration.

Signed-off-by: Sven Van Asbroeck [email protected]

rust/kernel/of.rs

rust/kernel/lib.rs

rust/kernel/of.rs

nbdd0121 · 2021-06-16T17:43:46Z

rust/kernel/of.rs

-///
-/// The final array element is always filled with zeros (the default).
-pub struct OfMatchTable(InnerTable);
+pub trait OfMatchTable {


I don't think trait is correct abstraction here. It should look more like CStr and CBoundedStr.

You think so? All an OfMatchTable consumer (i.e. a driver) needs, is a way to retrieve a static-lifetime pointer to the C table, right?

I suspect the strange thing you observe is related to the usage of incomplete features.

I haven't seen any strange things yet - I was just wondering if the array behaviour could be different for platforms I cannot test. Good to hear that it's correct, and no repr is required.

@nbdd0121 can you elaborate why trait isn't the correct abstraction here?

Conceptually OfMatchTable is to ConstOfMatchTable as slice is to array. Obvious slice is not abstracted as a trait that all arrays implement.

I think what you should do here is to have a

/// INVARIANT: end with a sentinel #[repr(transparent)] pub struct OfMatchTable([bindings::of_device_id]);

or maybe

/// INVARIANT: point to a slice that end with a sentinel #[repr(transparent)] pub struct OfMatchTable(bindings::of_device_id);

and then have ConstOfMatchTable implement Deref<Target=OfMatchTable>. This way you'll only need to accept a &'static OfMatchTable.

I agree with you that, in general, a String/&str type abstraction pair looks prettier than dyn trait. However, I can't find a way to implement your suggestion without using unsafe code:

/// Wraps a kernel Open Firmware / devicetree match table. #[repr(transparent)] pub struct OfMatchTable([bindings::of_device_id]); impl<const N: usize> Deref for ConstOfMatchTable<N> { type Target = OfMatchTable; fn deref(&self) -> &OfMatchTable { unsafe { &*(&self.table[..] as *const [bindings::of_device_id] as *const OfMatchTable) } } }

That's really unfortunate, because the dyn trait solution uses only safe Rust. Even coercing the *const reference is fully safe, as the pointer has 'static lifetime.

Would I be able to convince you to prefer the slightly more unseemly, yet safe, abstraction, over the pretty-yet-unsafe version?

I think the kernel crate should aim to provide ergonomic abstractions, accepting internal usage of unsafe. What kernel crate is to the drivers is pretty similar to what std is to userspace Rust programs, and it is ubiquitous in std to perform this sort of cast (e.g. https://doc.rust-lang.org/stable/src/std/path.rs.html#1791-1793).

I think we should stop demonizing unsafes and avoiding them at all cost. If usage of unsafe is helpful, then use it and just justify why it's safe with safety comments. It's not "pretty-yet-unsafe" abstraction; it is a pretty-and-safe abstraction, with a justified use of unsafe operation internally.

Nevermind - I think there is a way that is provably correct.

I think we should stop demonizing unsafes and avoiding them at all cost. If usage of unsafe is helpful, then use it and just justify why it's safe with safety comments. It's not "pretty-yet-unsafe" abstraction; it is a pretty-and-safe abstraction, with a justified use of unsafe operation internally.

The main reason I want rust in the kernel is its safe subset. I generally agree that when the use of unsafe brings a great usability improvement, then we should consider it. However, I think we should still avoid it -- we are all susceptible to making mistakes, having the compiler catch them for us is highly desirable, especially in the kernel crate.

Like @wedsonaf I believe that we should avoid unsafe unless there's a great usability improvement. That said, I'll go with the consensus here. Wedson, you are currently reviewing the unsafe version. Would you like to see the safe version as well? It's here.

TheSven73 · 2021-06-18T13:37:54Z

v1 -> v2

found a way to eliminate the need for incomplete features

TheSven73 · 2021-06-22T13:50:43Z

v2 -> v3

nbdd0121:

use &str / &Path type of abstraction to pass table to driver

rust/kernel/of.rs

TheSven73 · 2021-06-22T17:28:15Z

v3 -> v4

wedsonaf:

restore safe version of PR for review
use while loops in const fns
improve comment conjugations

nbdd0121 · 2021-06-22T21:28:45Z

Your "safe" abstraction is unsound:

struct Foo;
impl OfMatchTable for Foo {
	fn as_ptr(&'static self) -> *const bindings::of_device_id {
		core::ptr::null()
	}
}

static foo: Foo = Foo;
let table: &'static dyn OfMatchTable = &foo;
// Using table causes UB.

To solve soundness you'll have to mark trait OfMatchTable unsafe to implement.

TheSven73 · 2021-06-22T21:53:17Z

Your "safe" abstraction is unsound:

Thanks for noticing Gary !
Well, potentially unsound, if someone decides to return null in the future, but point well taken :)

We should be able to make this sound without the use of unsafe by returning a &'static instead of a *const. I'll push that as a v5. Let me know what you think.

TheSven73 · 2021-06-22T21:54:36Z

v4 -> v5

nbdd0121:

fix unsound abstraction by returning &'static instead of *const

nbdd0121 · 2021-06-22T21:57:13Z

We should be able to make this sound without the use of unsafe by returning a &'static instead of a *const. I'll push that as a v5. Let me know what you think.

No, it's still unsound unfortunately; the last element needs to be sentinel which cannot be enforced:

struct Foo;
impl OfMatchTable for Foo {
	fn as_ptr(&'static self) -> &'static bindings::of_device_id {
		static DEVICE_ID: bindings::of_device_id = /* something */;
		// This does not end with sentinel, so is still unsound
		&DEVICE_ID
	}
}

TheSven73 · 2021-06-22T22:23:49Z

Hm, this problem exists independently of whether we use a dyn trait or a #[repr(transparent)], right? How would you address it?

nbdd0121 · 2021-06-22T22:28:26Z

This won't be an issue for #[repr(transparent)], because you can have that as a type invariant. The very nature that unsafe is needed to create a &OfMatchTable actually acts to prevent safe code from creating any unsoundness here.

TheSven73 · 2021-06-22T22:38:21Z

Edited: I think I understand now... Thanks ! Sounds like we do need the unsafe block after all then. It was worth a try :)

TheSven73 · 2021-06-23T02:48:30Z

v5 -> v6

nbdd0121:

restore use of use &str / &Path type of abstraction to pass table to driver
make OfMatchTable abstraction sound by moving from safe to unsafe Rust (aka. The Guo Paradox) and using type invariants.
added a Suggested-by: Gary Guo <[email protected]> tag to the commit message

TheSven73 · 2021-06-23T14:02:02Z

Rebased against the latest rust branch.

wedsonaf · 2021-06-23T14:33:58Z

rust/kernel/of.rs

+        Self {
+            table,
+            // INVARIANTS: we zero the sentinel here, and never change it
+            // anwhere. Therefore it always contains zeroes.


nit: typo: anywhere

Use const generics and const fns to construct our `of_match_table` structure at build time. A few runtime errors now become build time errors, which is a good thing. If the platdev's `Registration` structure takes a const generics table object directly, it acquires/inherits the table's const generics, which looks very complex. Prevent this from happening by passing the table to `Registration` as a non-generic "reference object" in the style of `&str` and `&Path`. Suggested-by: Gary Guo <[email protected]> Signed-off-by: Sven Van Asbroeck <[email protected]>

ksquirrel · 2021-06-23T14:47:23Z

Review of 5b2ed146301b:

✔️ Commit 5b2ed14: Looks fine!

TheSven73 · 2021-06-23T14:47:51Z

Corrected typo anwhere -> anywhere

ojeda · 2021-06-23T15:23:25Z

rust/kernel/of.rs

+        let compatible = compatible.as_bytes_with_nul();
+        let mut i = 0;
+        while i < compatible.len() {
+            // if `compatible` does not fit in `id.compatible`, an


Suggested change

// if `compatible` does not fit in `id.compatible`, an

// If `compatible` does not fit in `id.compatible`, an

ojeda · 2021-06-23T15:23:52Z

rust/kernel/of.rs

+        let head = &self.table[0] as *const bindings::of_device_id as *const OfMatchTable;
+        // SAFETY: The returned reference must remain valid for the lifetime of `self`.


Suggested change

let head = &self.table[0] as *const bindings::of_device_id as *const OfMatchTable;

// SAFETY: The returned reference must remain valid for the lifetime of `self`.

let head = &self.table[0] as *const bindings::of_device_id as *const OfMatchTable;

// SAFETY: The returned reference must remain valid for the lifetime of `self`.

ojeda · 2021-06-23T15:27:13Z

rust/kernel/platdev.rs

-        //       `bindings::platform_driver_unregister()`, or
-        //      - null.
+        //   - `of_match_table` is either a raw pointer with static lifetime,
+        //      as guaranteed by the [`of::OfMatchTable::as_ptr()`] invariant,


Suggested change

// as guaranteed by the [`of::OfMatchTable::as_ptr()`] invariant,

// as guaranteed by the [`of::OfMatchTable::as_ptr()`] return type,

?

ojeda · 2021-06-23T15:27:46Z

rust/kernel/of.rs

+        let mut i = 0;
+        while i < compatible.len() {
+            // if `compatible` does not fit in `id.compatible`, an
+            // "index out of bounds" build time exception will be triggered.


Suggested change

// "index out of bounds" build time exception will be triggered.

// "index out of bounds" build time error will be triggered.

ojeda · 2021-06-23T15:33:50Z

Some nits for later since it was merged :-P

wedsonaf · 2021-06-23T15:33:56Z

Oops, sorry Miguel, I thought there were no more comments on this.

TheSven73 · 2021-06-23T15:35:25Z

@ojeda I can do a quick PR for those nits, if you like.

Some documentation suggestions on Rust-for-Linux#380 were not applied when that PR was merged. Apply them in this PR instead. Signed-off-by: Sven Van Asbroeck <[email protected]>

ojeda · 2021-06-23T16:41:26Z

@wedsonaf No worries!
@TheSven73 Thanks!

TheSven73 · 2021-06-23T18:53:17Z

PS @nbdd0121 Thank you for your advice and expertise with this PR Gary, I really appreciate it.