[PATCH] docs: rust: extend abstraction and binding documentation

[PATCH] docs: rust: extend abstraction and binding documentation

[Dirk Behme]

Add some basics explained by Miguel in [1] to the documentation.
And connect it with some hints where this is implemented in the
kernel.

[1] https://www.linuxfoundation.org/webinars/rust-for-linux-writing-abstractions-and-drivers

CC: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 Documentation/rust/general-information.rst | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/Documentation/rust/general-information.rst b/Documentation/rust/general-information.rst
index 081397827a7ea..616e2f2c09220 100644
--- a/Documentation/rust/general-information.rst
+++ b/Documentation/rust/general-information.rst
@@ -64,6 +64,62 @@ but it is intended that coverage is expanded as time goes on. "Leaf" modules
 (e.g. drivers) should not use the C bindings directly. Instead, subsystems
 should provide as-safe-as-possible abstractions as needed.
 
+.. code-block::
+
+	                                                rust/bindings/
+                                                       (rust/helpers.c)
+
+                                                          include/ -----+ <-+
+                                                                        |   |
+         drivers/              rust/kernel/              +----------+ <-+   |
+           fs/                                           | bindgen  |       |
+          .../            +-------------------+          +----------+ --+   |
+                          |    Abstractions   |                         |   |
+       +---------+        | +------+ +------+ |          +----------+   |   |
+       | my_foo  | -----> | | foo  | | bar  | | -------> | Bindings | <-+   |
+       | driver  |  Safe  | | sub- | | sub- | |  Unsafe  |          |       |
+       +---------+        | |system| |system| |          | bindings | <-----+
+            |             | +------+ +------+ |          |  crate   |       |
+            |             |   kernel crate    |          +----------+       |
+            |             +-------------------+                             |
+            |                                                               |
+            +------------------# FORBIDDEN #--------------------------------+
+
+The main idea is to encapsulate all ``unsafe()`` handling in the abstractions reviewed
+and documented carefully. These are considered to be sound then. With this model
+it is assumed that users of the abstractions ("my_foo driver") can't do anything
+unsound if
+
+#. the abstractions are sound
+#. they don't use ``unsafe()``
+
+Bindings
+~~~~~~~~
+
+By including a C header from ``include/`` into ``rust/bindings/bindings_helper.h``
+the ``bindgen`` tool will auto-generate the bindings for the included subsystem.
+See the ``*_generated.rs`` output files in the ``rust/bindings/`` directory.
+
+For parts of the C header ``bindgen`` doesn't auto generate, e.g. C ``inline``
+functions, there is the option to add a small wrapper function to ``rust/helpers.c``
+to make it available for the Rust side as well.
+
+Abstractions
+~~~~~~~~~~~~
+
+Abstractions are the glue layer between the bindings and the in-kernel users. E.g.
+the drivers or file systems written in Rust. They are located in ``rust/kernel/``
+and are supposed to make the ``unsafe()`` access to the bindings as-safe-as-possible
+for their users.
+
+Tutorial
+~~~~~~~~
+
+The webinar `Rust For Linux: Writing Safe Abstractions & Drivers`
+
+	https://www.linuxfoundation.org/webinars/rust-for-linux-writing-abstractions-and-drivers
+
+has an introduction to this topic.
 
 Conditional compilation
 -----------------------
-- 
2.28.0

Re: [PATCH] docs: rust: extend abstraction and binding documentation

[Valentin Obst]

+.. code-block::
+
+	                                                rust/bindings/
+                                                       (rust/helpers.c)
+
+                                                          include/ -----+ <-+
+                                                                        |   |
+         drivers/              rust/kernel/              +----------+ <-+   |
+           fs/                                           | bindgen  |       |
+          .../            +-------------------+          +----------+ --+   |
+                          |    Abstractions   |                         |   |
+       +---------+        | +------+ +------+ |          +----------+   |   |
+       | my_foo  | -----> | | foo  | | bar  | | -------> | Bindings | <-+   |
+       | driver  |  Safe  | | sub- | | sub- | |  Unsafe  |          |       |
+       +---------+        | |system| |system| |          | bindings | <-----+
+            |             | +------+ +------+ |          |  crate   |       |
+            |             |   kernel crate    |          +----------+       |
+            |             +-------------------+                             |
+            |                                                               |
+            +------------------# FORBIDDEN #--------------------------------+
+
+The main idea is to encapsulate all ``unsafe()`` handling in the abstractions reviewed
+and documented carefully. These are considered to be sound then.

Maybe reword a bit to make it smoother:

The main idea is to encapsulate all ``unsafe()`` handling in carefully
reviewed and documented abstractions. These are then considered to be
sound.

> 								  With this model
> +it is assumed that users of the abstractions ("my_foo driver") can't do anything
> +unsound if

Not sure if 'assumed' is the best word here. Isn't the soundness
of safe code implied by the soundness of the safe abstractions it is
using? In that case one could use stronger words and say, e.g.,
"... ensured that ...".

> +
> +#. the abstractions are sound
> +#. they don't use ``unsafe()``
> +
> +Bindings
> +~~~~~~~~
> +
> +By including a C header from ``include/`` into ``rust/bindings/bindings_helper.h``
> +the ``bindgen`` tool will auto-generate the bindings for the included subsystem.
> +See the ``*_generated.rs`` output files in the ``rust/bindings/`` directory.
> +
> +For parts of the C header ``bindgen`` doesn't auto generate, e.g. C ``inline``
> +functions, there is the option to add a small wrapper function to ``rust/helpers.c``
> +to make it available for the Rust side as well.
> +
> +Abstractions
> +~~~~~~~~~~~~
> +
> +Abstractions are the glue layer between the bindings and the in-kernel users. E.g.
> +the drivers or file systems written in Rust.

Wouldn't start a sentence with 'e.g.'. I'd either write it out, i.e.,
"For example,", or not start a new sentence: "... users, e.g. ...".

> 						They are located in ``rust/kernel/``
> +and are supposed to make the ``unsafe()`` access to the bindings as-safe-as-possible
> +for their users.
> +

I think one could make it more clear that users are not expected to
access the bindings directly, e.g.:

... and their role is to encapsulate the ``unsafe()`` access to the
bindings into a safe API that they expose to their users.

Note: Looks like you only sent this to Miguel and RFL list (intentional?).

	- Best Valentin

Re: [PATCH] docs: rust: extend abstraction and binding documentation

[Miguel Ojeda]

On Tue, Feb 6, 2024 at 7:54 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
> CC: Miguel Ojeda <ojeda@kernel.org>

Nit: it is typically spelled Cc (i.e. camel case).

> +.. code-block::
> +
> +                                                       rust/bindings/
> +                                                       (rust/helpers.c)
> +
> +                                                          include/ -----+ <-+
> +                                                                        |   |
> +         drivers/              rust/kernel/              +----------+ <-+   |
> +           fs/                                           | bindgen  |       |
> +          .../            +-------------------+          +----------+ --+   |
> +                          |    Abstractions   |                         |   |
> +       +---------+        | +------+ +------+ |          +----------+   |   |
> +       | my_foo  | -----> | | foo  | | bar  | | -------> | Bindings | <-+   |
> +       | driver  |  Safe  | | sub- | | sub- | |  Unsafe  |          |       |
> +       +---------+        | |system| |system| |          | bindings | <-----+
> +            |             | +------+ +------+ |          |  crate   |       |
> +            |             |   kernel crate    |          +----------+       |
> +            |             +-------------------+                             |
> +            |                                                               |
> +            +------------------# FORBIDDEN #--------------------------------+

Thanks a lot for the effort of making that slide into ASCII :)

I could try to see if I can export the diagram into SVG and include it.

> +The main idea is to encapsulate all ``unsafe()`` handling in the abstractions reviewed
> +and documented carefully. These are considered to be sound then. With this model
> +it is assumed that users of the abstractions ("my_foo driver") can't do anything
> +unsound if
> +
> +#. the abstractions are sound
> +#. they don't use ``unsafe()``

Like Valentin, I feel the wording needs some improvement here. Part of
the confusion may be the `unsafe()` there -- it is unclear what it
means (especially given the parentheses).

It is true we want to encapsulate C APIs and we forbid calling
directly `bindings` outside subsystems / exceptional cases; and
ideally provide safe abstractions as much as possible. However, we are
not the ones "considering" the abstractions sound (or at least that
way of putting it may confuse readers). We do our best to have sound
abstractions, but it is not something we "decide". So if e.g. someone
passes a raw pointer and just dereferences it in a safe function, that
is most likely unsound and it should be fixed.

That is, in turn, what allows callers to avoid breaking things. In
other words, we don't "assume that users cannot do anything unsound"
(introduce UB?) -- quite the opposite: what we want (as much as
possible) is that they cannot introduce UB even if they tried.

And that applies even if users use `unsafe`, as long as they use it
correctly, i.e. as long as they don't break the preconditions of what
they use. It is just that the best case is that they do not even need
to use `unsafe` calls (this is why we want that APIs are as safe as
possible, because then we minimize the chances that users get it wrong
and introduce UB).

> +Abstractions
> +~~~~~~~~~~~~
> +
> +Abstractions are the glue layer between the bindings and the in-kernel users. E.g.

Perhaps we should avoid "glue layer", since that sometimes implies
that it is a "thin" layer / "uninteresting" layer. However, the
abstractions are the actual interesting bits (and can be quite
complex, precisely to achieve the relaxation of safety preconditions,
i.e. making APIs "as safe as possible", that you mention below).

But it is quite subjective.

> +The webinar `Rust For Linux: Writing Safe Abstractions & Drivers`
> +
> +       https://www.linuxfoundation.org/webinars/rust-for-linux-writing-abstractions-and-drivers
> +
> +has an introduction to this topic.

I appreciate this, though some of the things in there are outdated, so
I am not sure about including this nowadays. Perhaps we should record
new ones instead :) Or, even better, I could "write" the talk in
written form here, which can be maintained and be kept up to date.
Relatedly, Benno is working on some extra documentation about how to
write safety preconditions that we will probably add around here when
it is ready.

Cheers,
Miguel

Re: [PATCH] docs: rust: extend abstraction and binding documentation

[Behme Dirk (CM/ESO2)]

On 06.02.2024 20:35, Miguel Ojeda wrote:
> On Tue, Feb 6, 2024 at 7:54 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>>
>> CC: Miguel Ojeda <ojeda@kernel.org>
> 
> Nit: it is typically spelled Cc (i.e. camel case).
> 
>> +.. code-block::
>> +
>> +                                                       rust/bindings/
>> +                                                       (rust/helpers.c)
>> +
>> +                                                          include/ -----+ <-+
>> +                                                                        |   |
>> +         drivers/              rust/kernel/              +----------+ <-+   |
>> +           fs/                                           | bindgen  |       |
>> +          .../            +-------------------+          +----------+ --+   |
>> +                          |    Abstractions   |                         |   |
>> +       +---------+        | +------+ +------+ |          +----------+   |   |
>> +       | my_foo  | -----> | | foo  | | bar  | | -------> | Bindings | <-+   |
>> +       | driver  |  Safe  | | sub- | | sub- | |  Unsafe  |          |       |
>> +       +---------+        | |system| |system| |          | bindings | <-----+
>> +            |             | +------+ +------+ |          |  crate   |       |
>> +            |             |   kernel crate    |          +----------+       |
>> +            |             +-------------------+                             |
>> +            |                                                               |
>> +            +------------------# FORBIDDEN #--------------------------------+
> 
> Thanks a lot for the effort of making that slide into ASCII :)
> 
> I could try to see if I can export the diagram into SVG and include it.
> 
>> +The main idea is to encapsulate all ``unsafe()`` handling in the abstractions reviewed
>> +and documented carefully. These are considered to be sound then. With this model
>> +it is assumed that users of the abstractions ("my_foo driver") can't do anything
>> +unsound if
>> +
>> +#. the abstractions are sound
>> +#. they don't use ``unsafe()``
> 
> Like Valentin, I feel the wording needs some improvement here. Part of
> the confusion may be the `unsafe()` there -- it is unclear what it
> means (especially given the parentheses).
> 
> It is true we want to encapsulate C APIs and we forbid calling
> directly `bindings` outside subsystems / exceptional cases; and
> ideally provide safe abstractions as much as possible. However, we are
> not the ones "considering" the abstractions sound (or at least that
> way of putting it may confuse readers). We do our best to have sound
> abstractions, but it is not something we "decide". So if e.g. someone
> passes a raw pointer and just dereferences it in a safe function, that
> is most likely unsound and it should be fixed.
> 
> That is, in turn, what allows callers to avoid breaking things. In
> other words, we don't "assume that users cannot do anything unsound"
> (introduce UB?) -- quite the opposite: what we want (as much as
> possible) is that they cannot introduce UB even if they tried.
> 
> And that applies even if users use `unsafe`, as long as they use it
> correctly, i.e. as long as they don't break the preconditions of what
> they use. It is just that the best case is that they do not even need
> to use `unsafe` calls (this is why we want that APIs are as safe as
> possible, because then we minimize the chances that users get it wrong
> and introduce UB).
> 
>> +Abstractions
>> +~~~~~~~~~~~~
>> +
>> +Abstractions are the glue layer between the bindings and the in-kernel users. E.g.
> 
> Perhaps we should avoid "glue layer", since that sometimes implies
> that it is a "thin" layer / "uninteresting" layer. However, the
> abstractions are the actual interesting bits (and can be quite
> complex, precisely to achieve the relaxation of safety preconditions,
> i.e. making APIs "as safe as possible", that you mention below).
> 
> But it is quite subjective.
> 
>> +The webinar `Rust For Linux: Writing Safe Abstractions & Drivers`
>> +
>> +       https://www.linuxfoundation.org/webinars/rust-for-linux-writing-abstractions-and-drivers
>> +
>> +has an introduction to this topic.
> 
> I appreciate this, though some of the things in there are outdated, so
> I am not sure about including this nowadays. Perhaps we should record
> new ones instead :) Or, even better, I could "write" the talk in
> written form here, which can be maintained and be kept up to date.


Yes, that sounds really great! :)


> Relatedly, Benno is working on some extra documentation about how to
> write safety preconditions that we will probably add around here when
> it is ready.

Benno, is it worth to wait for that? I.e. drop this patch and wait for 
your version?


In general, I try to document/extend in Documentation/rust/ what I 
understood in RFL is different to "standard" Rust. This 
abstraction/bindings part is one of it. I have some additional topics 
like no_std usage (I think this not mentioned anywhere, yet?) and maybe 
error handling (in most cases we might not want to "crash" the kernel 
with panic).


Best regards

Dirk

Re: [PATCH] docs: rust: extend abstraction and binding documentation

[Benno Lossin]

On 2/7/24 06:32, Behme Dirk (CM/ESO2) wrote:
> On 06.02.2024 20:35, Miguel Ojeda wrote:
>> On Tue, Feb 6, 2024 at 7:54 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>>>
>>> CC: Miguel Ojeda <ojeda@kernel.org>
>>
>> Nit: it is typically spelled Cc (i.e. camel case).
>>
>>> +.. code-block::
>>> +
>>> +                                                       rust/bindings/
>>> +                                                       (rust/helpers.c)
>>> +
>>> +                                                          include/ -----+ <-+
>>> +                                                                        |   |
>>> +         drivers/              rust/kernel/              +----------+ <-+   |
>>> +           fs/                                           | bindgen  |       |
>>> +          .../            +-------------------+          +----------+ --+   |
>>> +                          |    Abstractions   |                         |   |
>>> +       +---------+        | +------+ +------+ |          +----------+   |   |
>>> +       | my_foo  | -----> | | foo  | | bar  | | -------> | Bindings | <-+   |
>>> +       | driver  |  Safe  | | sub- | | sub- | |  Unsafe  |          |       |
>>> +       +---------+        | |system| |system| |          | bindings | <-----+
>>> +            |             | +------+ +------+ |          |  crate   |       |
>>> +            |             |   kernel crate    |          +----------+       |
>>> +            |             +-------------------+                             |
>>> +            |                                                               |
>>> +            +------------------# FORBIDDEN #--------------------------------+
>>
>> Thanks a lot for the effort of making that slide into ASCII :)
>>
>> I could try to see if I can export the diagram into SVG and include it.
>>
>>> +The main idea is to encapsulate all ``unsafe()`` handling in the abstractions reviewed
>>> +and documented carefully. These are considered to be sound then. With this model
>>> +it is assumed that users of the abstractions ("my_foo driver") can't do anything
>>> +unsound if
>>> +
>>> +#. the abstractions are sound
>>> +#. they don't use ``unsafe()``
>>
>> Like Valentin, I feel the wording needs some improvement here. Part of
>> the confusion may be the `unsafe()` there -- it is unclear what it
>> means (especially given the parentheses).
>>
>> It is true we want to encapsulate C APIs and we forbid calling
>> directly `bindings` outside subsystems / exceptional cases; and
>> ideally provide safe abstractions as much as possible. However, we are
>> not the ones "considering" the abstractions sound (or at least that
>> way of putting it may confuse readers). We do our best to have sound
>> abstractions, but it is not something we "decide". So if e.g. someone
>> passes a raw pointer and just dereferences it in a safe function, that
>> is most likely unsound and it should be fixed.
>>
>> That is, in turn, what allows callers to avoid breaking things. In
>> other words, we don't "assume that users cannot do anything unsound"
>> (introduce UB?) -- quite the opposite: what we want (as much as
>> possible) is that they cannot introduce UB even if they tried.
>>
>> And that applies even if users use `unsafe`, as long as they use it
>> correctly, i.e. as long as they don't break the preconditions of what
>> they use. It is just that the best case is that they do not even need
>> to use `unsafe` calls (this is why we want that APIs are as safe as
>> possible, because then we minimize the chances that users get it wrong
>> and introduce UB).
>>
>>> +Abstractions
>>> +~~~~~~~~~~~~
>>> +
>>> +Abstractions are the glue layer between the bindings and the in-kernel users. E.g.
>>
>> Perhaps we should avoid "glue layer", since that sometimes implies
>> that it is a "thin" layer / "uninteresting" layer. However, the
>> abstractions are the actual interesting bits (and can be quite
>> complex, precisely to achieve the relaxation of safety preconditions,
>> i.e. making APIs "as safe as possible", that you mention below).
>>
>> But it is quite subjective.
>>
>>> +The webinar `Rust For Linux: Writing Safe Abstractions & Drivers`
>>> +
>>> +       https://www.linuxfoundation.org/webinars/rust-for-linux-writing-abstractions-and-drivers
>>> +
>>> +has an introduction to this topic.
>>
>> I appreciate this, though some of the things in there are outdated, so
>> I am not sure about including this nowadays. Perhaps we should record
>> new ones instead :) Or, even better, I could "write" the talk in
>> written form here, which can be maintained and be kept up to date.
> 
> 
> Yes, that sounds really great! :)
> 
> 
>> Relatedly, Benno is working on some extra documentation about how to
>> write safety preconditions that we will probably add around here when
>> it is ready.
> 
> Benno, is it worth to wait for that? I.e. drop this patch and wait for
> your version?

My stuff focuses more on how to to write safety documentation and what
our motivations for certain conventions are. This gives a higher level
overview and useful on its own, so you can keep it.
I agree with Miguel that some of the wording should be improved. Maybe
he can help you with that.

-- 
Cheers,
Benno