[RFC PATCH 0/5] Introduce the Rust Safety Standard

[RFC PATCH 0/5] Introduce the Rust Safety Standard

[Benno Lossin]

`unsafe` Rust code in the kernel is required to have safety
documentation. This is to ensure the correctness of `unsafe` code and is
thus very important.
However, at this point in time there does not exist a standard way of
writing safety documentation. This leads to confusion, as authors
struggle to find the right way to convey their desired intentions.
Readers similarly struggle with correctly interpreting the 
documentation; it's the wild west.

To alleviate this issue and to raise the quality of safety
documentation, this patch series introduces a standard way of writing
safety documentation.

Because creating a standard for every possible requirement is a tall
task, this series starts off very small. I hope to start more
discussion, in order to find the best form of safety documentation for
the kernel.
Please do not hesitate to give any kind of comment. I could benefit
especially from improvements to the visual look of the documentation, as
this is my first time writing Sphinx documentation. For example, I am
not satisfied with how the tables renders in HTML.

Benno Lossin (5):
  doc: rust: create safety standard
  doc: rust: safety standard: add examples
  doc: rust: safety standard: add guarantees and type invariants
  doc: rust: safety standard: add safety requirements
  doc: rust: safety standard: add justifications

 Documentation/rust/general-information.rst    |   1 +
 Documentation/rust/index.rst                  |   1 +
 .../rust/safety-standard/examples.rst         |  70 +++++
 .../rust/safety-standard/guarantee.rst        |   7 +
 Documentation/rust/safety-standard/index.rst  | 281 ++++++++++++++++++
 .../rust/safety-standard/justifications.rst   |  40 +++
 .../rust/safety-standard/requirements.rst     |  80 +++++
 .../rust/safety-standard/type-invariants.rst  |  18 ++
 8 files changed, 498 insertions(+)
 create mode 100644 Documentation/rust/safety-standard/examples.rst
 create mode 100644 Documentation/rust/safety-standard/guarantee.rst
 create mode 100644 Documentation/rust/safety-standard/index.rst
 create mode 100644 Documentation/rust/safety-standard/justifications.rst
 create mode 100644 Documentation/rust/safety-standard/requirements.rst
 create mode 100644 Documentation/rust/safety-standard/type-invariants.rst

-- 
2.45.1

[RFC PATCH 1/5] doc: rust: create safety standard

[Benno Lossin]

`unsafe` Rust code in the kernel is required to have safety
documentation. This is to ensure the correctness of `unsafe` code and is
thus very important.
However, at this point in time there does not exist a standard way of
writing safety documentation. This leads to confusion, as authors
struggle to find the right way to convey their desired intentions.
Readers struggle with correctly interpreting the existing documentation.

Add the safety standard that will document the meaning of safety
documentation. This first document gives an overview of the problem and
gives general information about the topic.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
 Documentation/rust/general-information.rst   |   1 +
 Documentation/rust/index.rst                 |   1 +
 Documentation/rust/safety-standard/index.rst | 246 +++++++++++++++++++
 3 files changed, 248 insertions(+)
 create mode 100644 Documentation/rust/safety-standard/index.rst

diff --git a/Documentation/rust/general-information.rst b/Documentation/rust/general-information.rst
index e3f388ef4ee4..ddfe4e2e5307 100644
--- a/Documentation/rust/general-information.rst
+++ b/Documentation/rust/general-information.rst
@@ -54,6 +54,7 @@ the same invocation used for compilation, e.g.::
 Please note that Clippy may change code generation, thus it should not be
 enabled while building a production kernel.
 
+.. _rust-abstractions:
 
 Abstractions vs. bindings
 -------------------------
diff --git a/Documentation/rust/index.rst b/Documentation/rust/index.rst
index 46d35bd395cf..968e9aace301 100644
--- a/Documentation/rust/index.rst
+++ b/Documentation/rust/index.rst
@@ -39,6 +39,7 @@ configurations.
     quick-start
     general-information
     coding-guidelines
+    safety-standard/index
     arch-support
     testing
 
diff --git a/Documentation/rust/safety-standard/index.rst b/Documentation/rust/safety-standard/index.rst
new file mode 100644
index 000000000000..1cbc8d3dea04
--- /dev/null
+++ b/Documentation/rust/safety-standard/index.rst
@@ -0,0 +1,246 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. highlight:: rust
+
+====================
+Rust Safety Standard
+====================
+
+Safe Rust code cannot have memory related bugs. This is a guarantee by the Rust compiler. Of course
+it is not without caveats: no compiler bugs, no bugs in the specification etc. But the possibly most
+important caveat is that of ``unsafe`` code. ``unsafe`` code needs to follow certain rules in order
+for safe code to enjoy the no-memory-bugs privilege. A simple example of such a rule is that
+references must be valid for the duration of their lifetime. If any rule is violated, it can lead
+to undefined behavior even in safe code! The term undefined behavior in Rust has a lot stricter
+meaning than in C or C++: UB in Rust is totally forbidden. In C one might rely on the compiler
+implementation to ensure correct code generation, but that is not the case for Rust. You can read
+more about UB in Rust
+`here <https://doc.rust-lang.org/reference/behavior-considered-undefined.html>`_.
+
+If ``unsafe`` code makes our life this difficult, one might ask the question "why do we even need
+it?" and the answer to that is that it gives users an escape hatch to do things that the compiler
+normally forbids. ``unsafe`` code is a tool that enables programmers to write more performant code,
+or code that interacts with hardware or C. These things are particularly important in kernel
+development.
+
+The most effective way to prevent issues in ``unsafe`` code is to just not write ``unsafe`` code in
+the first place. That is why minimizing the amount of ``unsafe`` code is very important. For
+example, drivers are not allowed to directly interface with the C side. Instead of directly
+communicating with C functions, they interact with Rust abstractions. This concentrates the usage
+of ``unsafe`` code, making it easy to fix issues, since only the abstraction needs to be fixed.
+Abstractions also allow taking advantage of other Rust language features. Read more in
+:ref:`rust-abstractions`.
+
+Since the correctness of the abstractions is integral for safe code to also be correct, extra effort
+is expended to get them right. Part of that is good safety documentation.
+
+The goals of safety documentation are:
+
+* reduce the amount of bugs in ``unsafe`` code,
+* help readers know why a given piece of ``unsafe`` code is sound,
+* help writers write ``unsafe`` code with confidence,
+* simplify the work of reviewers.
+
+This document standardizes safety documentation. The necessity for this is simple, only a common
+language that all parties understand is effective at the above task. We want to avoid
+misunderstandings in safety related matters. An additional benefit is that programmers will not have
+to ponder for the correct phrasing, since they can find it here.
+
+This document assumes that the reader is familiar with Rust code and understands the most important
+concepts of ``unsafe`` Rust. It is recommended that the reader has read the `Rust Book`_. Since this
+document is about safety documentation, almost all examples are going to contain ``unsafe`` code.
+For this reason it is also recommended to read the `Rustonomicon`_, one of the best resources on
+``unsafe`` code.
+
+.. _Rustonomicon: https://doc.rust-lang.org/nomicon/index.html
+.. _Rust Book: https://doc.rust-lang.org/stable/book/
+
+If you need help coming up with an abstraction, or with writing the safety documentation for an
+abstraction, feel free to reach out on `zulip`_ or the `list`_.
+
+.. _zulip: https://rust-for-linux.zulipchat.com
+.. _list: https://lore.kernel.org/rust-for-linux
+
+Soundness
+=========
+
+``unsafe`` operations (e.g. ``unsafe`` functions, dereferencing raw pointers etc.) have certain
+conditions that need to be fulfilled in order for the operation to not be UB.
+To evaluate if the ``unsafe`` code usage is correct, one needs to consider the API that wraps said
+``unsafe`` code. If under all possible safe uses of the API, the conditions for the ``unsafe``
+operation are fulfilled, the API is *sound*. Otherwise it is *unsound*. Here is a simple example::
+
+    pub struct Data {
+        a: usize,
+    }
+
+    pub fn access_a(data: *mut Data) -> usize {
+        unsafe { (*data).a }
+    }
+
+    fn main() {
+        let mut d = Data { a: 42 };
+        println!("{}", access_a(&mut d));
+    }
+
+While this example has no UB, the function ``access_a`` is unsound. This is because one could just
+write the following safe usage::
+
+    println!("{}", access_a(core::ptr::null_mut()));
+
+And this would result in a dereference of a null pointer.
+
+In its essence, a sound API means that if someone only writes safe code, they can never encounter UB
+even if they call safe code that calls ``unsafe`` code behind the scenes.
+
+Because unsoundness issues have the potential for allowing safe code to experience UB, they are
+treated similarly to actual bugs with UB. Their fixes should also be included in the  stable tree.
+
+Safety Documentation
+====================
+
+After trying to minimize and remove as much ``unsafe`` code as possible, there still is some left.
+This is because some things are just not possible in only safe code. This last part of ``unsafe``
+code must still be correct. Helping with that is the safety documentation: it meticulously documents
+the various requirements and justifications for every line of ``unsafe`` code. That way it can be
+ensured that all ``unsafe`` code is sound without anyone needing to know the whole kernel at once.
+The gist of the idea is this: every ``unsafe`` operation documents its requirements and every
+location that uses an ``unsafe`` operation documents for every requirement a justification why they
+are fulfilled. If now all requirements and justifications are correct, then there can only be sound
+``unsafe`` code.
+
+The ``unsafe`` keywords has two different meanings depending on the context it is used in:
+
+* granting access to an unchecked operation,
+* declaring that something is an unchecked operation.
+
+In both cases we have to add safety documentation. In the first case, we have to justify why we can
+always guarantee that the requirements of the unchecked operation are fulfilled. In the second case,
+we have to list the requirements that have to be fulfilled for the operation to be sound.
+
+In the following sections we will go over each location where ``unsafe`` can be used.
+
+.. _unsafe-Functions:
+
+``unsafe`` Functions
+--------------------
+
+``unsafe`` on function declarations is used to state that this function has special requirements
+that callers have to ensure when calling the function::
+
+    unsafe fn foo() {
+        // ...
+    }
+
+These requirements are called the safety requirements of the function. These requirements can take
+any shape and range from simple requirements like "``ptr_arg`` is valid" (``ptr_arg`` refers to some
+argument with the type matching ``*mut T`` or ``*const T``) to more complex requirements like
+"``ptr`` must be valid, point to a ``NUL``-terminated C string, and it must be valid for at least
+``'a``. While the returned value is alive, the memory at ``ptr`` must not be mutated.".
+
+The safety requirements have to be documented in the so called safety section::
+
+    /// <oneline description of the function>
+    ///
+    /// <full description of the function>
+    ///
+    /// # Safety
+    ///
+    /// <safety requirements>
+    unsafe fn foo() {
+        // ...
+    }
+
+.. _unsafe-Blocks:
+
+``unsafe`` Blocks
+-----------------
+
+``unsafe`` code blocks are used to call ``unsafe`` functions and perform built-in ``unsafe``
+operations such as dereferencing a raw pointer::
+
+    unsafe { foo() };
+
+In order to ensure that all safety requirements of ``unsafe`` operations are upheld, a safety
+comment is mandatory for all ``unsafe`` blocks. This safety comment needs to provide a correct
+justification for every safety requirements of every operation within the block::
+
+    // SAFETY: <justifications>
+    unsafe { foo() };
+
+For transparency it is best practice to have only a single ``unsafe`` operation per ``unsafe``
+block, since then it is more clear what the justifications are trying to justify. Safe operations
+should not be included in the block, since it adds confusion as to which operation is the ``unsafe``
+one. In certain cases however it makes it easier to understand if there is only a single ``unsafe``
+block. For example::
+
+    // SAFETY: `ptr` is valid for writes.
+    unsafe {
+        (*ptr).field1 = 42;
+        (*ptr).field2 = 24;
+        (*ptr).field3 = 2442;
+    }
+
+In this case it is more readable to not split the block into multiple parts.
+
+``unsafe`` Traits
+-----------------
+
+When ``unsafe`` is on a ``trait`` declaration::
+
+    unsafe trait Foo {}
+
+The ``trait`` has special requirements for implementing it. Similar to :ref:`unsafe-Functions`, these
+are called safety requirements and need to be documented in the same way::
+
+    /// <oneline description of the trait>
+    ///
+    /// <full description of the trait>
+    ///
+    /// # Safety
+    ///
+    /// <safety requirements>
+    unsafe trait Foo {}
+
+``unsafe`` Impls
+----------------
+
+When ``unsafe`` is on an ``impl`` item::
+
+    unsafe impl Foo for Bar {}
+
+The ``Foo`` ``trait`` has to be ``unsafe`` and its safety requirements need to be justified
+similarly to :ref:`unsafe-Blocks`::
+
+    // SAFETY: <justification>
+    unsafe impl Foo for Bar {}
+
+General Rules
+=============
+
+The general thought behind all rules in the safety standard is that everything that cannot be
+statically checked by the Rust compiler and guaranteed, needs to be either checked at runtime, or
+have to have safety documentation.
+
+The Kernel uses ``deny(unsafe_op_in_unsafe_fn)``, disallowing ``unsafe`` operations to be contained
+in ``unsafe`` functions without a surrounding ``unsafe`` block, an example violating that would be::
+
+    unsafe fn zero_ptr(ptr: *mut u32) {
+        *ptr = 0;
+    }
+
+Denying code like this is becoming the default in modern editions of the Rust language. It is also
+easy to see why we would want to deny such code: where would we put the ``SAFETY`` comment for the
+pointer dereference?
+
+Further Pages
+-------------
+
+.. toctree::
+   :maxdepth: 1
+
+.. only::  subproject and html
+
+   Indices
+   =======
+
+   * :ref:`genindex`
-- 
2.45.1

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Greg KH]

On Wed, Jul 17, 2024 at 10:12:29PM +0000, Benno Lossin wrote:
> +Because unsoundness issues have the potential for allowing safe code to experience UB, they are
> +treated similarly to actual bugs with UB. Their fixes should also be included in the  stable tree.

Odd extra space before "stable".

Also, link to the stable kernel rules here when you reference "stable
tree"?  That will explain what you mean here.

thanks,

greg k-h

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Benno Lossin]

On 18.07.24 06:45, Greg KH wrote:
> On Wed, Jul 17, 2024 at 10:12:29PM +0000, Benno Lossin wrote:
>> +Because unsoundness issues have the potential for allowing safe code to experience UB, they are
>> +treated similarly to actual bugs with UB. Their fixes should also be included in the  stable tree.
> 
> Odd extra space before "stable".
> 
> Also, link to the stable kernel rules here when you reference "stable
> tree"?  That will explain what you mean here.

Sure will add it, do you mean Documentation/process/stable-kernel-rules.rst?
Or a different file?

---
Cheers,
Benno

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Greg KH]

On Wed, Jul 24, 2024 at 07:13:08PM +0000, Benno Lossin wrote:
> On 18.07.24 06:45, Greg KH wrote:
> > On Wed, Jul 17, 2024 at 10:12:29PM +0000, Benno Lossin wrote:
> >> +Because unsoundness issues have the potential for allowing safe code to experience UB, they are
> >> +treated similarly to actual bugs with UB. Their fixes should also be included in the  stable tree.
> > 
> > Odd extra space before "stable".
> > 
> > Also, link to the stable kernel rules here when you reference "stable
> > tree"?  That will explain what you mean here.
> 
> Sure will add it, do you mean Documentation/process/stable-kernel-rules.rst?

Yes please.

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Alice Ryhl]

On Thu, Jul 18, 2024 at 12:12 AM Benno Lossin <benno.lossin@proton.me> wrote:
> +Soundness
> +=========
> +
> +``unsafe`` operations (e.g. ``unsafe`` functions, dereferencing raw pointers etc.) have certain
> +conditions that need to be fulfilled in order for the operation to not be UB.
> +To evaluate if the ``unsafe`` code usage is correct, one needs to consider the API that wraps said
> +``unsafe`` code. If under all possible safe uses of the API, the conditions for the ``unsafe``
> +operation are fulfilled, the API is *sound*. Otherwise it is *unsound*. Here is a simple example::
> +
> +    pub struct Data {
> +        a: usize,
> +    }
> +
> +    pub fn access_a(data: *mut Data) -> usize {
> +        unsafe { (*data).a }
> +    }
> +
> +    fn main() {
> +        let mut d = Data { a: 42 };
> +        println!("{}", access_a(&mut d));
> +    }
> +
> +While this example has no UB, the function ``access_a`` is unsound. This is because one could just
> +write the following safe usage::
> +
> +    println!("{}", access_a(core::ptr::null_mut()));
> +
> +And this would result in a dereference of a null pointer.
> +
> +In its essence, a sound API means that if someone only writes safe code, they can never encounter UB
> +even if they call safe code that calls ``unsafe`` code behind the scenes.

I think this section on soundness could be more clear. You never
really give a definition for what soundness is; you just jump into an
example immediately.

It may be nice to talk about how a sound API must prevent memory
safety issues, even if the safe code required to do so is silly or
unrealistic. Doing so is necessary to maintain the "safe code never
has memory safety bugs" guarantee.

> +Because unsoundness issues have the potential for allowing safe code to experience UB, they are
> +treated similarly to actual bugs with UB. Their fixes should also be included in the  stable tree.
> +
> +Safety Documentation
> +====================
> +
> +After trying to minimize and remove as much ``unsafe`` code as possible, there still is some left.
> +This is because some things are just not possible in only safe code. This last part of ``unsafe``
> +code must still be correct. Helping with that is the safety documentation: it meticulously documents
> +the various requirements and justifications for every line of ``unsafe`` code. That way it can be
> +ensured that all ``unsafe`` code is sound without anyone needing to know the whole kernel at once.
> +The gist of the idea is this: every ``unsafe`` operation documents its requirements and every
> +location that uses an ``unsafe`` operation documents for every requirement a justification why they
> +are fulfilled. If now all requirements and justifications are correct, then there can only be sound
> +``unsafe`` code.
> +
> +The ``unsafe`` keywords has two different meanings depending on the context it is used in:
> +
> +* granting access to an unchecked operation,
> +* declaring that something is an unchecked operation.
> +
> +In both cases we have to add safety documentation. In the first case, we have to justify why we can
> +always guarantee that the requirements of the unchecked operation are fulfilled. In the second case,
> +we have to list the requirements that have to be fulfilled for the operation to be sound.
> +
> +In the following sections we will go over each location where ``unsafe`` can be used.
> +
> +.. _unsafe-Functions:
> +
> +``unsafe`` Functions
> +--------------------
> +
> +``unsafe`` on function declarations is used to state that this function has special requirements
> +that callers have to ensure when calling the function::
> +
> +    unsafe fn foo() {
> +        // ...
> +    }
> +
> +These requirements are called the safety requirements of the function. These requirements can take
> +any shape and range from simple requirements like "``ptr_arg`` is valid" (``ptr_arg`` refers to some
> +argument with the type matching ``*mut T`` or ``*const T``) to more complex requirements like
> +"``ptr`` must be valid, point to a ``NUL``-terminated C string, and it must be valid for at least
> +``'a``. While the returned value is alive, the memory at ``ptr`` must not be mutated.".
> +
> +The safety requirements have to be documented in the so called safety section::
> +
> +    /// <oneline description of the function>
> +    ///
> +    /// <full description of the function>
> +    ///
> +    /// # Safety
> +    ///
> +    /// <safety requirements>
> +    unsafe fn foo() {
> +        // ...
> +    }

I would love to see a proper example here.

> +.. _unsafe-Blocks:
> +
> +``unsafe`` Blocks
> +-----------------
> +
> +``unsafe`` code blocks are used to call ``unsafe`` functions and perform built-in ``unsafe``
> +operations such as dereferencing a raw pointer::
> +
> +    unsafe { foo() };
> +
> +In order to ensure that all safety requirements of ``unsafe`` operations are upheld, a safety
> +comment is mandatory for all ``unsafe`` blocks. This safety comment needs to provide a correct
> +justification for every safety requirements of every operation within the block::
> +
> +    // SAFETY: <justifications>
> +    unsafe { foo() };

I think it is worth explicitly pointing out that the safety comment
must explain why the preconditions are satisfied, *not* what the
preconditions are. It's a really really common mistake to mix up
these, and it probably even makes sense to include two examples
showing the difference.

> +For transparency it is best practice to have only a single ``unsafe`` operation per ``unsafe``
> +block, since then it is more clear what the justifications are trying to justify. Safe operations
> +should not be included in the block, since it adds confusion as to which operation is the ``unsafe``
> +one. In certain cases however it makes it easier to understand if there is only a single ``unsafe``
> +block. For example::
> +
> +    // SAFETY: `ptr` is valid for writes.
> +    unsafe {
> +        (*ptr).field1 = 42;
> +        (*ptr).field2 = 24;
> +        (*ptr).field3 = 2442;
> +    }
> +
> +In this case it is more readable to not split the block into multiple parts.

It would be nice with an example of a completely normal safety comment
example. Also would be nice to call out that the semicolon goes
outside the unsafe block to improve formatting, when it's a single
operation.

It would also be nice with an example that shows how to reference the
safety requirements of the current function. (That is, an example that
combines unsafe fn with unsafe block.)

Alice

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Benno Lossin]

On 18.07.24 14:20, Alice Ryhl wrote:
> On Thu, Jul 18, 2024 at 12:12 AM Benno Lossin <benno.lossin@proton.me> wrote:
>> +Soundness
>> +=========
>> +
>> +``unsafe`` operations (e.g. ``unsafe`` functions, dereferencing raw pointers etc.) have certain
>> +conditions that need to be fulfilled in order for the operation to not be UB.
>> +To evaluate if the ``unsafe`` code usage is correct, one needs to consider the API that wraps said
>> +``unsafe`` code. If under all possible safe uses of the API, the conditions for the ``unsafe``
>> +operation are fulfilled, the API is *sound*. Otherwise it is *unsound*. Here is a simple example::
>> +
>> +    pub struct Data {
>> +        a: usize,
>> +    }
>> +
>> +    pub fn access_a(data: *mut Data) -> usize {
>> +        unsafe { (*data).a }
>> +    }
>> +
>> +    fn main() {
>> +        let mut d = Data { a: 42 };
>> +        println!("{}", access_a(&mut d));
>> +    }
>> +
>> +While this example has no UB, the function ``access_a`` is unsound. This is because one could just
>> +write the following safe usage::
>> +
>> +    println!("{}", access_a(core::ptr::null_mut()));
>> +
>> +And this would result in a dereference of a null pointer.
>> +
>> +In its essence, a sound API means that if someone only writes safe code, they can never encounter UB
>> +even if they call safe code that calls ``unsafe`` code behind the scenes.
> 
> I think this section on soundness could be more clear. You never
> really give a definition for what soundness is; you just jump into an
> example immediately.

I would argue that the sentence "If under all possible safe uses of the
API, the conditions for the ``unsafe`` operation are fulfilled, the API
is *sound*. Otherwise it is *unsound*." is a definition. Is this lacking
anything, or do you have an idea to improve the wording?
I am not really understanding what kind of additional definition you
would like to see.

> It may be nice to talk about how a sound API must prevent memory
> safety issues, even if the safe code required to do so is silly or
> unrealistic. Doing so is necessary to maintain the "safe code never
> has memory safety bugs" guarantee.

That is a good way to phrase things, I will add that.

>> +Because unsoundness issues have the potential for allowing safe code to experience UB, they are
>> +treated similarly to actual bugs with UB. Their fixes should also be included in the  stable tree.
>> +
>> +Safety Documentation
>> +====================
>> +
>> +After trying to minimize and remove as much ``unsafe`` code as possible, there still is some left.
>> +This is because some things are just not possible in only safe code. This last part of ``unsafe``
>> +code must still be correct. Helping with that is the safety documentation: it meticulously documents
>> +the various requirements and justifications for every line of ``unsafe`` code. That way it can be
>> +ensured that all ``unsafe`` code is sound without anyone needing to know the whole kernel at once.
>> +The gist of the idea is this: every ``unsafe`` operation documents its requirements and every
>> +location that uses an ``unsafe`` operation documents for every requirement a justification why they
>> +are fulfilled. If now all requirements and justifications are correct, then there can only be sound
>> +``unsafe`` code.
>> +
>> +The ``unsafe`` keywords has two different meanings depending on the context it is used in:
>> +
>> +* granting access to an unchecked operation,
>> +* declaring that something is an unchecked operation.
>> +
>> +In both cases we have to add safety documentation. In the first case, we have to justify why we can
>> +always guarantee that the requirements of the unchecked operation are fulfilled. In the second case,
>> +we have to list the requirements that have to be fulfilled for the operation to be sound.
>> +
>> +In the following sections we will go over each location where ``unsafe`` can be used.
>> +
>> +.. _unsafe-Functions:
>> +
>> +``unsafe`` Functions
>> +--------------------
>> +
>> +``unsafe`` on function declarations is used to state that this function has special requirements
>> +that callers have to ensure when calling the function::
>> +
>> +    unsafe fn foo() {
>> +        // ...
>> +    }
>> +
>> +These requirements are called the safety requirements of the function. These requirements can take
>> +any shape and range from simple requirements like "``ptr_arg`` is valid" (``ptr_arg`` refers to some
>> +argument with the type matching ``*mut T`` or ``*const T``) to more complex requirements like
>> +"``ptr`` must be valid, point to a ``NUL``-terminated C string, and it must be valid for at least
>> +``'a``. While the returned value is alive, the memory at ``ptr`` must not be mutated.".
>> +
>> +The safety requirements have to be documented in the so called safety section::
>> +
>> +    /// <oneline description of the function>
>> +    ///
>> +    /// <full description of the function>
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// <safety requirements>
>> +    unsafe fn foo() {
>> +        // ...
>> +    }
> 
> I would love to see a proper example here.

Depending on how the discussion goes with this series, I plan to go
through the tree and try to improve the safety comments. That way I will
have a good source for examples, since otherwise they are extremely hard
to come up with.
(I am also open to any suggestions :)

>> +.. _unsafe-Blocks:
>> +
>> +``unsafe`` Blocks
>> +-----------------
>> +
>> +``unsafe`` code blocks are used to call ``unsafe`` functions and perform built-in ``unsafe``
>> +operations such as dereferencing a raw pointer::
>> +
>> +    unsafe { foo() };
>> +
>> +In order to ensure that all safety requirements of ``unsafe`` operations are upheld, a safety
>> +comment is mandatory for all ``unsafe`` blocks. This safety comment needs to provide a correct
>> +justification for every safety requirements of every operation within the block::
>> +
>> +    // SAFETY: <justifications>
>> +    unsafe { foo() };
> 
> I think it is worth explicitly pointing out that the safety comment
> must explain why the preconditions are satisfied, *not* what the
> preconditions are. It's a really really common mistake to mix up
> these, and it probably even makes sense to include two examples
> showing the difference.

Good idea, but I will put that into justifications.rst.

>> +For transparency it is best practice to have only a single ``unsafe`` operation per ``unsafe``
>> +block, since then it is more clear what the justifications are trying to justify. Safe operations
>> +should not be included in the block, since it adds confusion as to which operation is the ``unsafe``
>> +one. In certain cases however it makes it easier to understand if there is only a single ``unsafe``
>> +block. For example::
>> +
>> +    // SAFETY: `ptr` is valid for writes.
>> +    unsafe {
>> +        (*ptr).field1 = 42;
>> +        (*ptr).field2 = 24;
>> +        (*ptr).field3 = 2442;
>> +    }
>> +
>> +In this case it is more readable to not split the block into multiple parts.
> 
> It would be nice with an example of a completely normal safety comment
> example. Also would be nice to call out that the semicolon goes
> outside the unsafe block to improve formatting, when it's a single
> operation.

Will add.

> It would also be nice with an example that shows how to reference the
> safety requirements of the current function. (That is, an example that
> combines unsafe fn with unsafe block.)

See above.

---
Cheers,
Benno

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Daniel Almeida]

Hi Benno,

It’s nice to see this shaping up. I do agree that it’s a bit of a wild
west right now.

IMHO, we need a lint to enforce compliance, unless we plan to have every patch
reviewed by the RFL community, which is unrealistic as time goes forward. I
myself have forgotten to properly document unsafe blocks because it’s easy
to miss things when submitting more than a thousand LOC.

A new clippy lint would make sense here, since we already have clippy support
in the kernel anyways.

> On 17 Jul 2024, at 19:12, Benno Lossin <benno.lossin@proton.me> wrote:
> 
> `unsafe` Rust code in the kernel is required to have safety
> documentation. This is to ensure the correctness of `unsafe` code and is
> thus very important.
> However, at this point in time there does not exist a standard way of
> writing safety documentation. This leads to confusion, as authors
> struggle to find the right way to convey their desired intentions.
> Readers struggle with correctly interpreting the existing documentation.
> 
> Add the safety standard that will document the meaning of safety
> documentation. This first document gives an overview of the problem and
> gives general information about the topic.
> 
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> Documentation/rust/general-information.rst   |   1 +
> Documentation/rust/index.rst                 |   1 +
> Documentation/rust/safety-standard/index.rst | 246 +++++++++++++++++++
> 3 files changed, 248 insertions(+)
> create mode 100644 Documentation/rust/safety-standard/index.rst
> 
> diff --git a/Documentation/rust/general-information.rst b/Documentation/rust/general-information.rst
> index e3f388ef4ee4..ddfe4e2e5307 100644
> --- a/Documentation/rust/general-information.rst
> +++ b/Documentation/rust/general-information.rst
> @@ -54,6 +54,7 @@ the same invocation used for compilation, e.g.::
> Please note that Clippy may change code generation, thus it should not be
> enabled while building a production kernel.
> 
> +.. _rust-abstractions:
> 
> Abstractions vs. bindings
> -------------------------
> diff --git a/Documentation/rust/index.rst b/Documentation/rust/index.rst
> index 46d35bd395cf..968e9aace301 100644
> --- a/Documentation/rust/index.rst
> +++ b/Documentation/rust/index.rst
> @@ -39,6 +39,7 @@ configurations.
>     quick-start
>     general-information
>     coding-guidelines
> +    safety-standard/index
>     arch-support
>     testing
> 
> diff --git a/Documentation/rust/safety-standard/index.rst b/Documentation/rust/safety-standard/index.rst
> new file mode 100644
> index 000000000000..1cbc8d3dea04
> --- /dev/null
> +++ b/Documentation/rust/safety-standard/index.rst
> @@ -0,0 +1,246 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. highlight:: rust
> +
> +====================
> +Rust Safety Standard
> +====================
> +
> +Safe Rust code cannot have memory related bugs. This is a guarantee by the Rust compiler. Of course
> +it is not without caveats: no compiler bugs, no bugs in the specification etc. But the possibly most
> +important caveat is that of ``unsafe`` code. ``unsafe`` code needs to follow certain rules in order
> +for safe code to enjoy the no-memory-bugs privilege. A simple example of such a rule is that
> +references must be valid for the duration of their lifetime. If any rule is violated, it can lead
> +to undefined behavior even in safe code! The term undefined behavior in Rust has a lot stricter
> +meaning than in C or C++: UB in Rust is totally forbidden. In C one might rely on the compiler
> +implementation to ensure correct code generation, but that is not the case for Rust. You can read
> +more about UB in Rust
> +`here <https://doc.rust-lang.org/reference/behavior-considered-undefined.html>`_.
> +
> +If ``unsafe`` code makes our life this difficult, one might ask the question "why do we even need
> +it?" and the answer to that is that it gives users an escape hatch to do things that the compiler
> +normally forbids. ``unsafe`` code is a tool that enables programmers to write more performant code,
> +or code that interacts with hardware or C. These things are particularly important in kernel
> +development.
> +
> +The most effective way to prevent issues in ``unsafe`` code is to just not write ``unsafe`` code in
> +the first place. That is why minimizing the amount of ``unsafe`` code is very important. For
> +example, drivers are not allowed to directly interface with the C side. Instead of directly
> +communicating with C functions, they interact with Rust abstractions. This concentrates the usage
> +of ``unsafe`` code, making it easy to fix issues, since only the abstraction needs to be fixed.
> +Abstractions also allow taking advantage of other Rust language features. Read more in
> +:ref:`rust-abstractions`.

This is something that I think we should discuss at Kangrejos. I do not think
that we should set in stone that the kernel crate is the only place where
unsafe code is acceptable.

I am in no way disagreeing with the use of safe abstractions, but I think we
should have abstractions where they make sense. This is the case in the vast
majority of times, but not in *all* of them.

A simple example is a MMIO read or write. Should a driver be forbidden to call
readX/writeX for an address it knows to be valid? How can you possibly write an
abstraction for this, when the driver is the only one aware of the actual
device addresses, and when the driver author is the person with actual access
to the HW docs?

If a driver is written partially in Rust, and partially in C, and it gets a
pointer to some kcalloc’d memory in C, should It be forbidden to use unsafe
in order to build a slice from that pointer? How can you possibly design a
general abstraction for something that is, essentially, a driver-internal API?

For these corner cases, a simple safety comment should suffice. By all means,
let's strive to push as much of the unsafe bits into the kernel crate. But,
IMHO, we shouldn’t treat Rust drivers as some unprivileged entity, they’re
also kernel code, after all.

> +
> +Since the correctness of the abstractions is integral for safe code to also be correct, extra effort
> +is expended to get them right. Part of that is good safety documentation.
> +
> +The goals of safety documentation are:
> +
> +* reduce the amount of bugs in ``unsafe`` code,
> +* help readers know why a given piece of ``unsafe`` code is sound,
> +* help writers write ``unsafe`` code with confidence,
> +* simplify the work of reviewers.
> +
> +This document standardizes safety documentation. The necessity for this is simple, only a common
> +language that all parties understand is effective at the above task. We want to avoid
> +misunderstandings in safety related matters. An additional benefit is that programmers will not have
> +to ponder for the correct phrasing, since they can find it here.
> +
> +This document assumes that the reader is familiar with Rust code and understands the most important
> +concepts of ``unsafe`` Rust. It is recommended that the reader has read the `Rust Book`_. Since this
> +document is about safety documentation, almost all examples are going to contain ``unsafe`` code.
> +For this reason it is also recommended to read the `Rustonomicon`_, one of the best resources on
> +``unsafe`` code.
> +
> +.. _Rustonomicon: https://doc.rust-lang.org/nomicon/index.html
> +.. _Rust Book: https://doc.rust-lang.org/stable/book/
> +
> +If you need help coming up with an abstraction, or with writing the safety documentation for an
> +abstraction, feel free to reach out on `zulip`_ or the `list`_.
> +
> +.. _zulip: https://rust-for-linux.zulipchat.com
> +.. _list: https://lore.kernel.org/rust-for-linux
> +
> +Soundness
> +=========
> +
> +``unsafe`` operations (e.g. ``unsafe`` functions, dereferencing raw pointers etc.) have certain
> +conditions that need to be fulfilled in order for the operation to not be UB.
> +To evaluate if the ``unsafe`` code usage is correct, one needs to consider the API that wraps said
> +``unsafe`` code. If under all possible safe uses of the API, the conditions for the ``unsafe``
> +operation are fulfilled, the API is *sound*. Otherwise it is *unsound*. Here is a simple example::
> +
> +    pub struct Data {
> +        a: usize,
> +    }
> +
> +    pub fn access_a(data: *mut Data) -> usize {
> +        unsafe { (*data).a }
> +    }
> +
> +    fn main() {
> +        let mut d = Data { a: 42 };
> +        println!("{}", access_a(&mut d));
> +    }
> +
> +While this example has no UB, the function ``access_a`` is unsound. This is because one could just
> +write the following safe usage::
> +
> +    println!("{}", access_a(core::ptr::null_mut()));
> +
> +And this would result in a dereference of a null pointer.
> +
> +In its essence, a sound API means that if someone only writes safe code, they can never encounter UB
> +even if they call safe code that calls ``unsafe`` code behind the scenes.
> +
> +Because unsoundness issues have the potential for allowing safe code to experience UB, they are
> +treated similarly to actual bugs with UB. Their fixes should also be included in the  stable tree.
> +
> +Safety Documentation
> +====================
> +
> +After trying to minimize and remove as much ``unsafe`` code as possible, there still is some left.
> +This is because some things are just not possible in only safe code. This last part of ``unsafe``
> +code must still be correct. Helping with that is the safety documentation: it meticulously documents
> +the various requirements and justifications for every line of ``unsafe`` code. That way it can be
> +ensured that all ``unsafe`` code is sound without anyone needing to know the whole kernel at once.
> +The gist of the idea is this: every ``unsafe`` operation documents its requirements and every
> +location that uses an ``unsafe`` operation documents for every requirement a justification why they
> +are fulfilled. If now all requirements and justifications are correct, then there can only be sound
> +``unsafe`` code.
> +
> +The ``unsafe`` keywords has two different meanings depending on the context it is used in:
> +
> +* granting access to an unchecked operation,
> +* declaring that something is an unchecked operation.
> +
> +In both cases we have to add safety documentation. In the first case, we have to justify why we can
> +always guarantee that the requirements of the unchecked operation are fulfilled. In the second case,
> +we have to list the requirements that have to be fulfilled for the operation to be sound.
> +
> +In the following sections we will go over each location where ``unsafe`` can be used.
> +
> +.. _unsafe-Functions:
> +
> +``unsafe`` Functions
> +--------------------
> +
> +``unsafe`` on function declarations is used to state that this function has special requirements
> +that callers have to ensure when calling the function::
> +
> +    unsafe fn foo() {
> +        // ...
> +    }
> +
> +These requirements are called the safety requirements of the function. These requirements can take
> +any shape and range from simple requirements like "``ptr_arg`` is valid" (``ptr_arg`` refers to some
> +argument with the type matching ``*mut T`` or ``*const T``) to more complex requirements like
> +"``ptr`` must be valid, point to a ``NUL``-terminated C string, and it must be valid for at least
> +``'a``. While the returned value is alive, the memory at ``ptr`` must not be mutated.".
> +
> +The safety requirements have to be documented in the so called safety section::
> +
> +    /// <oneline description of the function>
> +    ///
> +    /// <full description of the function>
> +    ///
> +    /// # Safety
> +    ///
> +    /// <safety requirements>
> +    unsafe fn foo() {
> +        // ...
> +    }
> +
> +.. _unsafe-Blocks:
> +
> +``unsafe`` Blocks
> +-----------------
> +
> +``unsafe`` code blocks are used to call ``unsafe`` functions and perform built-in ``unsafe``
> +operations such as dereferencing a raw pointer::
> +
> +    unsafe { foo() };
> +
> +In order to ensure that all safety requirements of ``unsafe`` operations are upheld, a safety
> +comment is mandatory for all ``unsafe`` blocks. This safety comment needs to provide a correct
> +justification for every safety requirements of every operation within the block::
> +
> +    // SAFETY: <justifications>
> +    unsafe { foo() };
> +
> +For transparency it is best practice to have only a single ``unsafe`` operation per ``unsafe``
> +block, since then it is more clear what the justifications are trying to justify. Safe operations
> +should not be included in the block, since it adds confusion as to which operation is the ``unsafe``
> +one. In certain cases however it makes it easier to understand if there is only a single ``unsafe``
> +block. For example::
> +
> +    // SAFETY: `ptr` is valid for writes.
> +    unsafe {
> +        (*ptr).field1 = 42;
> +        (*ptr).field2 = 24;
> +        (*ptr).field3 = 2442;
> +    }
> +
> +In this case it is more readable to not split the block into multiple parts.
> +
> +``unsafe`` Traits
> +-----------------
> +
> +When ``unsafe`` is on a ``trait`` declaration::
> +
> +    unsafe trait Foo {}
> +
> +The ``trait`` has special requirements for implementing it. Similar to :ref:`unsafe-Functions`, these
> +are called safety requirements and need to be documented in the same way::
> +
> +    /// <oneline description of the trait>
> +    ///
> +    /// <full description of the trait>
> +    ///
> +    /// # Safety
> +    ///
> +    /// <safety requirements>
> +    unsafe trait Foo {}
> +
> +``unsafe`` Impls
> +----------------
> +
> +When ``unsafe`` is on an ``impl`` item::
> +
> +    unsafe impl Foo for Bar {}
> +
> +The ``Foo`` ``trait`` has to be ``unsafe`` and its safety requirements need to be justified
> +similarly to :ref:`unsafe-Blocks`::
> +
> +    // SAFETY: <justification>
> +    unsafe impl Foo for Bar {}
> +
> +General Rules
> +=============
> +
> +The general thought behind all rules in the safety standard is that everything that cannot be
> +statically checked by the Rust compiler and guaranteed, needs to be either checked at runtime, or
> +have to have safety documentation.
> +
> +The Kernel uses ``deny(unsafe_op_in_unsafe_fn)``, disallowing ``unsafe`` operations to be contained
> +in ``unsafe`` functions without a surrounding ``unsafe`` block, an example violating that would be::
> +
> +    unsafe fn zero_ptr(ptr: *mut u32) {
> +        *ptr = 0;
> +    }
> +
> +Denying code like this is becoming the default in modern editions of the Rust language. It is also
> +easy to see why we would want to deny such code: where would we put the ``SAFETY`` comment for the
> +pointer dereference?
> +
> +Further Pages
> +-------------
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +.. only::  subproject and html
> +
> +   Indices
> +   =======
> +
> +   * :ref:`genindex`
> -- 
> 2.45.1
> 
> 


— Daniel

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Alice Ryhl]

On Fri, Jul 19, 2024 at 6:24 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
> > On 17 Jul 2024, at 19:12, Benno Lossin <benno.lossin@proton.me> wrote:
> > +====================
> > +Rust Safety Standard
> > +====================
> > +
> > +Safe Rust code cannot have memory related bugs. This is a guarantee by the Rust compiler. Of course
> > +it is not without caveats: no compiler bugs, no bugs in the specification etc. But the possibly most
> > +important caveat is that of ``unsafe`` code. ``unsafe`` code needs to follow certain rules in order
> > +for safe code to enjoy the no-memory-bugs privilege. A simple example of such a rule is that
> > +references must be valid for the duration of their lifetime. If any rule is violated, it can lead
> > +to undefined behavior even in safe code! The term undefined behavior in Rust has a lot stricter
> > +meaning than in C or C++: UB in Rust is totally forbidden. In C one might rely on the compiler
> > +implementation to ensure correct code generation, but that is not the case for Rust. You can read
> > +more about UB in Rust
> > +`here <https://doc.rust-lang.org/reference/behavior-considered-undefined.html>`_.
> > +
> > +If ``unsafe`` code makes our life this difficult, one might ask the question "why do we even need
> > +it?" and the answer to that is that it gives users an escape hatch to do things that the compiler
> > +normally forbids. ``unsafe`` code is a tool that enables programmers to write more performant code,
> > +or code that interacts with hardware or C. These things are particularly important in kernel
> > +development.
> > +
> > +The most effective way to prevent issues in ``unsafe`` code is to just not write ``unsafe`` code in
> > +the first place. That is why minimizing the amount of ``unsafe`` code is very important. For
> > +example, drivers are not allowed to directly interface with the C side. Instead of directly
> > +communicating with C functions, they interact with Rust abstractions. This concentrates the usage
> > +of ``unsafe`` code, making it easy to fix issues, since only the abstraction needs to be fixed.
> > +Abstractions also allow taking advantage of other Rust language features. Read more in
> > +:ref:`rust-abstractions`.
>
> This is something that I think we should discuss at Kangrejos. I do not think
> that we should set in stone that the kernel crate is the only place where
> unsafe code is acceptable.
>
> I am in no way disagreeing with the use of safe abstractions, but I think we
> should have abstractions where they make sense. This is the case in the vast
> majority of times, but not in *all* of them.
>
> A simple example is a MMIO read or write. Should a driver be forbidden to call
> readX/writeX for an address it knows to be valid? How can you possibly write an
> abstraction for this, when the driver is the only one aware of the actual
> device addresses, and when the driver author is the person with actual access
> to the HW docs?
>
> If a driver is written partially in Rust, and partially in C, and it gets a
> pointer to some kcalloc’d memory in C, should It be forbidden to use unsafe
> in order to build a slice from that pointer? How can you possibly design a
> general abstraction for something that is, essentially, a driver-internal API?
>
> For these corner cases, a simple safety comment should suffice. By all means,
> let's strive to push as much of the unsafe bits into the kernel crate. But,
> IMHO, we shouldn’t treat Rust drivers as some unprivileged entity, they’re
> also kernel code, after all.

Perhaps it is worth having the document explicitly talk about middle
grounds between "directly access C apis" and "don't use unsafe code"?
My file abstractions [1] expose an unsafe function called
`assume_no_fdget_pos` and I intend for Rust Binder to have an unsafe
block calling that function. Using such an unsafe API is meaningfully
different from just directly calling `bindings::fget`, but both are
unsafe.

Though it doesn't seem like the document actually says "don't use
unsafe code in drivers". It just says "use abstractions".

Alice

[1]: https://lore.kernel.org/all/20240628-alice-file-v7-3-4d701f6335f3@google.com/

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Danilo Krummrich]

On Fri, Jul 19, 2024 at 01:24:10PM -0300, Daniel Almeida wrote:
> > +The most effective way to prevent issues in ``unsafe`` code is to just not write ``unsafe`` code in
> > +the first place. That is why minimizing the amount of ``unsafe`` code is very important. For
> > +example, drivers are not allowed to directly interface with the C side. Instead of directly
> > +communicating with C functions, they interact with Rust abstractions. This concentrates the usage
> > +of ``unsafe`` code, making it easy to fix issues, since only the abstraction needs to be fixed.
> > +Abstractions also allow taking advantage of other Rust language features. Read more in
> > +:ref:`rust-abstractions`.
> 
> This is something that I think we should discuss at Kangrejos. I do not think
> that we should set in stone that the kernel crate is the only place where
> unsafe code is acceptable.
> 
> I am in no way disagreeing with the use of safe abstractions, but I think we
> should have abstractions where they make sense. This is the case in the vast
> majority of times, but not in *all* of them.
> 
> A simple example is a MMIO read or write. Should a driver be forbidden to call
> readX/writeX for an address it knows to be valid? How can you possibly write an
> abstraction for this, when the driver is the only one aware of the actual
> device addresses, and when the driver author is the person with actual access
> to the HW docs?

We can easily build abstractions that ensure that the address a driver is trying
to access is mapped properly, such that you can't have accidential out-of-bound
accesses.

Those can be implemented by the corresponding subsystem / bus that the resource
originates from.

In fact, we already have abstractions for that on the way, a generic I/O
abstraction [1] as base implementation and a specific abstraction for PCI bars
[2].

Of course, if the MMIO region comes from let's say the device tree, we still
have to assume that the information in the DT is correct, but the driver does
not need unsafe code for this.

[1] https://lore.kernel.org/rust-for-linux/20240618234025.15036-8-dakr@redhat.com/
[2] https://lore.kernel.org/rust-for-linux/20240618234025.15036-11-dakr@redhat.com/

> 
> If a driver is written partially in Rust, and partially in C, and it gets a
> pointer to some kcalloc’d memory in C, should It be forbidden to use unsafe
> in order to build a slice from that pointer? How can you possibly design a
> general abstraction for something that is, essentially, a driver-internal API?

That sounds perfectly valid to me.

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Daniel Almeida]

Hi Danilo,


> 
> We can easily build abstractions that ensure that the address a driver is trying
> to access is mapped properly, such that you can't have accidential out-of-bound
> accesses.
> 
> Those can be implemented by the corresponding subsystem / bus that the resource
> originates from.
> 
> In fact, we already have abstractions for that on the way, a generic I/O
> abstraction [1] as base implementation and a specific abstraction for PCI bars
> [2].
> 
> Of course, if the MMIO region comes from let's say the device tree, we still
> have to assume that the information in the DT is correct, but the driver does
> not need unsafe code for this.
> 
> [1] https://lore.kernel.org/rust-for-linux/20240618234025.15036-8-dakr@redhat.com/
> [2] https://lore.kernel.org/rust-for-linux/20240618234025.15036-11-dakr@redhat.com/
> 

Thanks for pointing that out. So from this:

+impl<const SIZE: usize> Io<SIZE> {
+ ///
+ ///
+ /// # Safety
+ ///
+ /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
+ /// `maxsize`.
+ pub unsafe fn new(addr: usize, maxsize: usize) -> Result<Self> {
+ if maxsize < SIZE {
+ return Err(EINVAL);
+ }
+
+ Ok(Self { addr, maxsize })
+ }

It looks like one can do this:

let io = unsafe { Io::new(<some address>, <some size>)? }; 
let value = io.readb(<some offset>)?;

Where <some address> has already been mapped for <some size> at an earlier point?

That’s fine, as I said, if an abstraction makes sense, I have nothing
against it. My point is more that we shouldn’t enact a blanket ban on
'unsafe' in drivers because corner cases do exist. But it’s good to know that this
particular example I gave does not apply.


>> 
>> If a driver is written partially in Rust, and partially in C, and it gets a
>> pointer to some kcalloc’d memory in C, should It be forbidden to use unsafe
>> in order to build a slice from that pointer? How can you possibly design a
>> general abstraction for something that is, essentially, a driver-internal API?
> 
> That sounds perfectly valid to me.
> 


— Daniel

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Danilo Krummrich]

On Fri, Jul 19, 2024 at 03:09:09PM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> 
> > 
> > We can easily build abstractions that ensure that the address a driver is trying
> > to access is mapped properly, such that you can't have accidential out-of-bound
> > accesses.
> > 
> > Those can be implemented by the corresponding subsystem / bus that the resource
> > originates from.
> > 
> > In fact, we already have abstractions for that on the way, a generic I/O
> > abstraction [1] as base implementation and a specific abstraction for PCI bars
> > [2].
> > 
> > Of course, if the MMIO region comes from let's say the device tree, we still
> > have to assume that the information in the DT is correct, but the driver does
> > not need unsafe code for this.
> > 
> > [1] https://lore.kernel.org/rust-for-linux/20240618234025.15036-8-dakr@redhat.com/
> > [2] https://lore.kernel.org/rust-for-linux/20240618234025.15036-11-dakr@redhat.com/
> > 
> 
> Thanks for pointing that out. So from this:
> 
> +impl<const SIZE: usize> Io<SIZE> {
> + ///
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
> + /// `maxsize`.
> + pub unsafe fn new(addr: usize, maxsize: usize) -> Result<Self> {
> + if maxsize < SIZE {
> + return Err(EINVAL);
> + }
> +
> + Ok(Self { addr, maxsize })
> + }
> 
> It looks like one can do this:
> 
> let io = unsafe { Io::new(<some address>, <some size>)? }; 
> let value = io.readb(<some offset>)?;
> 
> Where <some address> has already been mapped for <some size> at an earlier point?

Yes, but (at least for full Rust drivers) this shouldn't be called by the driver
directly, but the corresponding subsystem / bus should provide a `Devres`
wrapped `Io` instance, like the PCI abstraction in [2] does.

Example:

```
// Get a `Devres` managed PCI bar mapping
let bar: Devres<pci::Bar> = pdev.iomap_region(0);

let reg = bar.try_readl(0x42)?;
```
You can also let the driver assert that the requested resource must have a
minimum size:

```
// Only succeeds if the PCI bar has at least a size of 0x1000
let bar = pdev.iomap_region_size::<0x1000>(0);

// Note: `readl` does not need to return a `Result`, since the boundary checks
// can be done on compile time due to the driver specified minimal mapping size.
let reg = bar.readl(0x42);
```

> 
> That’s fine, as I said, if an abstraction makes sense, I have nothing
> against it. My point is more that we shouldn’t enact a blanket ban on
> 'unsafe' in drivers because corner cases do exist. But it’s good to know that this
> particular example I gave does not apply.
> 
> 
> >> 
> >> If a driver is written partially in Rust, and partially in C, and it gets a
> >> pointer to some kcalloc’d memory in C, should It be forbidden to use unsafe
> >> in order to build a slice from that pointer? How can you possibly design a
> >> general abstraction for something that is, essentially, a driver-internal API?
> > 
> > That sounds perfectly valid to me.
> > 
> 
> 
> — Daniel
> 
> 

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Miguel Ojeda]

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

On Fri, Jul 19, 2024 at 6:24 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> A new clippy lint would make sense here, since we already have clippy support
> in the kernel anyways.

There is one already, which we want to enable.

Here is a quick patch (untested!) of how it could look like, in case
one wants to fill the TODOs, or we can just merge it as-is and clean
it up later to avoid adding new ones.

Cheers,
Miguel

[-- Attachment #2: 0001-rust-enable-clippy-undocumented_unsafe_blocks.patch --]
[-- Type: application/x-patch, Size: 17012 bytes --]

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Daniel Almeida]

> On 19 Jul 2024, at 14:28, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Fri, Jul 19, 2024 at 6:24 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> A new clippy lint would make sense here, since we already have clippy support
>> in the kernel anyways.
> 
> There is one already, which we want to enable.
> 
> Here is a quick patch (untested!) of how it could look like, in case
> one wants to fill the TODOs, or we can just merge it as-is and clean
> it up later to avoid adding new ones.
> 
> Cheers,
> Miguel
> <0001-rust-enable-clippy-undocumented_unsafe_blocks.patch>


IMHO, merging after testing makes sense, otherwise new ones will creep up as you said.


— Daniel

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Miguel Ojeda]

On Fri, Jul 19, 2024 at 6:24 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> For these corner cases, a simple safety comment should suffice. By all means,
> let's strive to push as much of the unsafe bits into the kernel crate. But,
> IMHO, we shouldn’t treat Rust drivers as some unprivileged entity, they’re
> also kernel code, after all.

The intention has never been to forbid unsafe blocks, but rather to
minimize the amount of unsafe code as much as possible.

That is why mixing Rust and C in ways that make the amount of unsafe
blocks increase a lot may not be the best approaches. Sometimes it may
be needed nevertheless, i.e. there is no hard rule here.

Relatedly, on the topic of drivers being "unprivileged" entities:
anything we can do to make any code as "unprivileged" as possible (in
"number of unsafe blocks" or other ways) is likely a good thing, as
long as it does not detract from what actually needs to be done and is
not too onerous.

In other words, part of the idea of using Rust has always been trying
to see how much we could tight things up while still making things
work in practice.

For instance, other ways to tight things up would be disallowing to
call certain APIs/subsystems/... (i.e. visibility, which we will add),
marking certain crates as `#![deny(unsafe_code)]` or other lints, or
verification of certain properties (there are researchers looking into
this).

But, yeah, I agree it will be one of the interesting discussions at
Kangrejos... :)

Cheers,
Miguel

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Benno Lossin]

On 19.07.24 18:24, Daniel Almeida wrote:
> Hi Benno,
> 
> It’s nice to see this shaping up. I do agree that it’s a bit of a wild
> west right now.
> 
> IMHO, we need a lint to enforce compliance, unless we plan to have every patch
> reviewed by the RFL community, which is unrealistic as time goes forward. I
> myself have forgotten to properly document unsafe blocks because it’s easy
> to miss things when submitting more than a thousand LOC.
> 
> A new clippy lint would make sense here, since we already have clippy support
> in the kernel anyways.

I definitely see the potential, but I have no experience writing clippy
lints. I also have no idea if it can detect comments.
I also think that a lint solution will be very difficult, since it will
either have to be a full proof assistant that mathematically checks if
the safety comments are correct, or we still need human review.
I think that if people are more familiar with safety comments, it will
be easier, it's just how one improves at coding.

I don't want to reject formal verification from the get-go; on the
contrary, I would like to see it applied more in the kernel. Rust has
several different implementations, but I haven't yet taken an in-depth
look at them. However, from my past experience with formal proof
assistants, I have my doubts that everyday developers will pick them up
more easily/in favor of just plain safety comments.

I think that we should apply formal verification to those areas that
have been shown to be very difficult to get right. We currently do not
have enough Rust to have such areas, but when we do, it might be a
powerful tool. But I don't see it becoming the norm for Rust code (but
maybe I am wrong, and I would be very happy to be wrong in this
instance).

There are also several clippy lints [1] that we could start using:
- missing_safety_doc
- multiple_unsafe_ops_per_block
- undocumented_unsafe_blocks
- unnecessary_safety_comment
- unnecessary_safety_doc

I personally think we should enable all of them.

[1]: https://rust-lang.github.io/rust-clippy/master/index.html#/safety

What did you expect/wish for with a clippy lint? Is it already present
or did you want something that verifies your safety comments?

>> On 17 Jul 2024, at 19:12, Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> `unsafe` Rust code in the kernel is required to have safety
>> documentation. This is to ensure the correctness of `unsafe` code and is
>> thus very important.
>> However, at this point in time there does not exist a standard way of
>> writing safety documentation. This leads to confusion, as authors
>> struggle to find the right way to convey their desired intentions.
>> Readers struggle with correctly interpreting the existing documentation.
>>
>> Add the safety standard that will document the meaning of safety
>> documentation. This first document gives an overview of the problem and
>> gives general information about the topic.
>>
>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
>> ---
>> Documentation/rust/general-information.rst   |   1 +
>> Documentation/rust/index.rst                 |   1 +
>> Documentation/rust/safety-standard/index.rst | 246 +++++++++++++++++++
>> 3 files changed, 248 insertions(+)
>> create mode 100644 Documentation/rust/safety-standard/index.rst
>>
>> diff --git a/Documentation/rust/general-information.rst b/Documentation/rust/general-information.rst
>> index e3f388ef4ee4..ddfe4e2e5307 100644
>> --- a/Documentation/rust/general-information.rst
>> +++ b/Documentation/rust/general-information.rst
>> @@ -54,6 +54,7 @@ the same invocation used for compilation, e.g.::
>> Please note that Clippy may change code generation, thus it should not be
>> enabled while building a production kernel.
>>
>> +.. _rust-abstractions:
>>
>> Abstractions vs. bindings
>> -------------------------
>> diff --git a/Documentation/rust/index.rst b/Documentation/rust/index.rst
>> index 46d35bd395cf..968e9aace301 100644
>> --- a/Documentation/rust/index.rst
>> +++ b/Documentation/rust/index.rst
>> @@ -39,6 +39,7 @@ configurations.
>>     quick-start
>>     general-information
>>     coding-guidelines
>> +    safety-standard/index
>>     arch-support
>>     testing
>>
>> diff --git a/Documentation/rust/safety-standard/index.rst b/Documentation/rust/safety-standard/index.rst
>> new file mode 100644
>> index 000000000000..1cbc8d3dea04
>> --- /dev/null
>> +++ b/Documentation/rust/safety-standard/index.rst
>> @@ -0,0 +1,246 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +.. highlight:: rust
>> +
>> +====================
>> +Rust Safety Standard
>> +====================
>> +
>> +Safe Rust code cannot have memory related bugs. This is a guarantee by the Rust compiler. Of course
>> +it is not without caveats: no compiler bugs, no bugs in the specification etc. But the possibly most
>> +important caveat is that of ``unsafe`` code. ``unsafe`` code needs to follow certain rules in order
>> +for safe code to enjoy the no-memory-bugs privilege. A simple example of such a rule is that
>> +references must be valid for the duration of their lifetime. If any rule is violated, it can lead
>> +to undefined behavior even in safe code! The term undefined behavior in Rust has a lot stricter
>> +meaning than in C or C++: UB in Rust is totally forbidden. In C one might rely on the compiler
>> +implementation to ensure correct code generation, but that is not the case for Rust. You can read
>> +more about UB in Rust
>> +`here <https://doc.rust-lang.org/reference/behavior-considered-undefined.html>`_.
>> +
>> +If ``unsafe`` code makes our life this difficult, one might ask the question "why do we even need
>> +it?" and the answer to that is that it gives users an escape hatch to do things that the compiler
>> +normally forbids. ``unsafe`` code is a tool that enables programmers to write more performant code,
>> +or code that interacts with hardware or C. These things are particularly important in kernel
>> +development.
>> +
>> +The most effective way to prevent issues in ``unsafe`` code is to just not write ``unsafe`` code in
>> +the first place. That is why minimizing the amount of ``unsafe`` code is very important. For
>> +example, drivers are not allowed to directly interface with the C side. Instead of directly
>> +communicating with C functions, they interact with Rust abstractions. This concentrates the usage
>> +of ``unsafe`` code, making it easy to fix issues, since only the abstraction needs to be fixed.
>> +Abstractions also allow taking advantage of other Rust language features. Read more in
>> +:ref:`rust-abstractions`.
> 
> This is something that I think we should discuss at Kangrejos. I do not think
> that we should set in stone that the kernel crate is the only place where
> unsafe code is acceptable.

Oh then I need to rephrase the above paragraph, since I don't meant to
say that. What I want to say is this:
 (1) concentrate as much `unsafe` code as possible, and put it somewhere
     where everyone can use it (ie the `kernel` crate)
 (2) abstract over common use-patterns of `unsafe` code via safe
     abstractions
 (3) disallow access to *raw* `bindings::` function calls from drivers.

From what you write below, I think that we are on the same page for (1)
and (2). What I want to accomplish with (3) is that we don't have hacky
drivers that are just like a C driver with `unsafe` sprinkled
throughout. If you want to do that, just write a C driver instead.

As Alice already replied, there should be no issue with having an
`unsafe` function in an Abstraction. But we should strive for them to be
as few as possible.

> I am in no way disagreeing with the use of safe abstractions, but I think we
> should have abstractions where they make sense. This is the case in the vast
> majority of times, but not in *all* of them.
> 
> A simple example is a MMIO read or write. Should a driver be forbidden to call
> readX/writeX for an address it knows to be valid? How can you possibly write an
> abstraction for this, when the driver is the only one aware of the actual
> device addresses, and when the driver author is the person with actual access
> to the HW docs?

One idea that I have in this concrete example would be to make the
driver specify in exactly one place what the addresses are that are
read/writeable. If there are devices with dynamic addresses, then we
could additionally provide an `unsafe` API, but link to the safe one for
people to prefer.

> If a driver is written partially in Rust, and partially in C, and it gets a
> pointer to some kcalloc’d memory in C, should It be forbidden to use unsafe
> in order to build a slice from that pointer? How can you possibly design a
> general abstraction for something that is, essentially, a driver-internal API?

This also would be a good example for an exception of (3). In this case,
you could still write a driver-specific abstraction that does everything
under the hood and then every place in the driver can use the safe
abstraction.

> For these corner cases, a simple safety comment should suffice. By all means,
> let's strive to push as much of the unsafe bits into the kernel crate. But,
> IMHO, we shouldn’t treat Rust drivers as some unprivileged entity, they’re
> also kernel code, after all.

That is true, but I want to prevent that we just "ship it" and then a
couple of days later it turns out that there was a good abstraction
after all. I personally like to spend a lot of time thinking about safe
abstractions before giving in to `unsafe`, but I understand that we need
to find a balance. In the end, we can also always change things. But
when something lands, it most of the time won't get people thinking
about whether there is a better way of doing things. Not unless the
status quo is annoying/burdensome, at which point it already "was too
late", ie there could have been more thought at the beginning.

---
Cheers,
Benno

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Miguel Ojeda]

On Wed, Jul 24, 2024 at 10:32 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> There are also several clippy lints [1] that we could start using:
> - missing_safety_doc
> - multiple_unsafe_ops_per_block
> - undocumented_unsafe_blocks
> - unnecessary_safety_comment
> - unnecessary_safety_doc
>
> I personally think we should enable all of them.

We briefly talked about it today -- others agreed on going ahead with
something like the diff I sent the other day, so I will send a formal
patch -- it has been a while since we wanted to do this (long enough
that we were the ones requesting one of those lints, and it got
implemented since then... :)

And we can keep the `TODO`s as "good first issue"s (I already updated
some days ago our good first issue about it:
https://github.com/Rust-for-Linux/linux/issues/351).

We can also enable the others easily, most are essentially clean
already anyway, so I will send that as well.

The only one that may be more "annoying" is
`multiple_unsafe_ops_per_block`. On the other hand, it could in fact
force people to think about every "bullet point" of the requirements
(the lint highlights nicely the different operations).

Cheers,
Miguel

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Benno Lossin]

On 24.07.24 23:20, Miguel Ojeda wrote:
> On Wed, Jul 24, 2024 at 10:32 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> There are also several clippy lints [1] that we could start using:
>> - missing_safety_doc
>> - multiple_unsafe_ops_per_block
>> - undocumented_unsafe_blocks
>> - unnecessary_safety_comment
>> - unnecessary_safety_doc
>>
>> I personally think we should enable all of them.
> 
> We briefly talked about it today -- others agreed on going ahead with
> something like the diff I sent the other day, so I will send a formal
> patch -- it has been a while since we wanted to do this (long enough
> that we were the ones requesting one of those lints, and it got
> implemented since then... :)

Perfect :)

> And we can keep the `TODO`s as "good first issue"s (I already updated
> some days ago our good first issue about it:
> https://github.com/Rust-for-Linux/linux/issues/351).

That sounds like a good idea.

> We can also enable the others easily, most are essentially clean
> already anyway, so I will send that as well.

Sounds good.

> The only one that may be more "annoying" is
> `multiple_unsafe_ops_per_block`. On the other hand, it could in fact
> force people to think about every "bullet point" of the requirements
> (the lint highlights nicely the different operations).

Oh yeah, that might be annoying if we have

    unsafe {
        (*ptr).a = 0;
        (*ptr).b = 0;
    }

So it probably is better to leave that one disabled.

---
Cheers,
Benno

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Daniel Almeida]

Hi Benno,


> On 24 Jul 2024, at 17:31, Benno Lossin <benno.lossin@proton.me> wrote:
> 
> On 19.07.24 18:24, Daniel Almeida wrote:
>> Hi Benno,
>> 
>> It’s nice to see this shaping up. I do agree that it’s a bit of a wild
>> west right now.
>> 
>> IMHO, we need a lint to enforce compliance, unless we plan to have every patch
>> reviewed by the RFL community, which is unrealistic as time goes forward. I
>> myself have forgotten to properly document unsafe blocks because it’s easy
>> to miss things when submitting more than a thousand LOC.
>> 
>> A new clippy lint would make sense here, since we already have clippy support
>> in the kernel anyways.
> 
> I definitely see the potential, but I have no experience writing clippy
> lints. I also have no idea if it can detect comments.
> I also think that a lint solution will be very difficult, since it will
> either have to be a full proof assistant that mathematically checks if
> the safety comments are correct, or we still need human review.
> I think that if people are more familiar with safety comments, it will
> be easier, it's just how one improves at coding.
> 
> I don't want to reject formal verification from the get-go; on the
> contrary, I would like to see it applied more in the kernel. Rust has
> several different implementations, but I haven't yet taken an in-depth
> look at them. However, from my past experience with formal proof
> assistants, I have my doubts that everyday developers will pick them up
> more easily/in favor of just plain safety comments.
> 
> I think that we should apply formal verification to those areas that
> have been shown to be very difficult to get right. We currently do not
> have enough Rust to have such areas, but when we do, it might be a
> powerful tool. But I don't see it becoming the norm for Rust code (but
> maybe I am wrong, and I would be very happy to be wrong in this
> instance).
> 
> There are also several clippy lints [1] that we could start using:
> - missing_safety_doc
> - multiple_unsafe_ops_per_block
> - undocumented_unsafe_blocks
> - unnecessary_safety_comment
> - unnecessary_safety_doc
> 
> I personally think we should enable all of them.
> 
> [1]: https://rust-lang.github.io/rust-clippy/master/index.html#/safety
> 
> What did you expect/wish for with a clippy lint? Is it already present
> or did you want something that verifies your safety comments?


Yeah, I wasn’t referring to formal verification, just a lint that will complain when
it finds an unsafe block that has no safety comments at all. The clippy lints you
listed should work fine and, IIUC, Miguel already has a patch to enable (some of) them,
so I don’t think any further action is needed.

> 
>>> On 17 Jul 2024, at 19:12, Benno Lossin <benno.lossin@proton.me> wrote:
>>> 
>>> `unsafe` Rust code in the kernel is required to have safety
>>> documentation. This is to ensure the correctness of `unsafe` code and is
>>> thus very important.
>>> However, at this point in time there does not exist a standard way of
>>> writing safety documentation. This leads to confusion, as authors
>>> struggle to find the right way to convey their desired intentions.
>>> Readers struggle with correctly interpreting the existing documentation.
>>> 
>>> Add the safety standard that will document the meaning of safety
>>> documentation. This first document gives an overview of the problem and
>>> gives general information about the topic.
>>> 
>>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
>>> ---
>>> Documentation/rust/general-information.rst   |   1 +
>>> Documentation/rust/index.rst                 |   1 +
>>> Documentation/rust/safety-standard/index.rst | 246 +++++++++++++++++++
>>> 3 files changed, 248 insertions(+)
>>> create mode 100644 Documentation/rust/safety-standard/index.rst
>>> 
>>> diff --git a/Documentation/rust/general-information.rst b/Documentation/rust/general-information.rst
>>> index e3f388ef4ee4..ddfe4e2e5307 100644
>>> --- a/Documentation/rust/general-information.rst
>>> +++ b/Documentation/rust/general-information.rst
>>> @@ -54,6 +54,7 @@ the same invocation used for compilation, e.g.::
>>> Please note that Clippy may change code generation, thus it should not be
>>> enabled while building a production kernel.
>>> 
>>> +.. _rust-abstractions:
>>> 
>>> Abstractions vs. bindings
>>> -------------------------
>>> diff --git a/Documentation/rust/index.rst b/Documentation/rust/index.rst
>>> index 46d35bd395cf..968e9aace301 100644
>>> --- a/Documentation/rust/index.rst
>>> +++ b/Documentation/rust/index.rst
>>> @@ -39,6 +39,7 @@ configurations.
>>>    quick-start
>>>    general-information
>>>    coding-guidelines
>>> +    safety-standard/index
>>>    arch-support
>>>    testing
>>> 
>>> diff --git a/Documentation/rust/safety-standard/index.rst b/Documentation/rust/safety-standard/index.rst
>>> new file mode 100644
>>> index 000000000000..1cbc8d3dea04
>>> --- /dev/null
>>> +++ b/Documentation/rust/safety-standard/index.rst
>>> @@ -0,0 +1,246 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +.. highlight:: rust
>>> +
>>> +====================
>>> +Rust Safety Standard
>>> +====================
>>> +
>>> +Safe Rust code cannot have memory related bugs. This is a guarantee by the Rust compiler. Of course
>>> +it is not without caveats: no compiler bugs, no bugs in the specification etc. But the possibly most
>>> +important caveat is that of ``unsafe`` code. ``unsafe`` code needs to follow certain rules in order
>>> +for safe code to enjoy the no-memory-bugs privilege. A simple example of such a rule is that
>>> +references must be valid for the duration of their lifetime. If any rule is violated, it can lead
>>> +to undefined behavior even in safe code! The term undefined behavior in Rust has a lot stricter
>>> +meaning than in C or C++: UB in Rust is totally forbidden. In C one might rely on the compiler
>>> +implementation to ensure correct code generation, but that is not the case for Rust. You can read
>>> +more about UB in Rust
>>> +`here <https://doc.rust-lang.org/reference/behavior-considered-undefined.html>`_.
>>> +
>>> +If ``unsafe`` code makes our life this difficult, one might ask the question "why do we even need
>>> +it?" and the answer to that is that it gives users an escape hatch to do things that the compiler
>>> +normally forbids. ``unsafe`` code is a tool that enables programmers to write more performant code,
>>> +or code that interacts with hardware or C. These things are particularly important in kernel
>>> +development.
>>> +
>>> +The most effective way to prevent issues in ``unsafe`` code is to just not write ``unsafe`` code in
>>> +the first place. That is why minimizing the amount of ``unsafe`` code is very important. For
>>> +example, drivers are not allowed to directly interface with the C side. Instead of directly
>>> +communicating with C functions, they interact with Rust abstractions. This concentrates the usage
>>> +of ``unsafe`` code, making it easy to fix issues, since only the abstraction needs to be fixed.
>>> +Abstractions also allow taking advantage of other Rust language features. Read more in
>>> +:ref:`rust-abstractions`.
>> 
>> This is something that I think we should discuss at Kangrejos. I do not think
>> that we should set in stone that the kernel crate is the only place where
>> unsafe code is acceptable.
> 
> Oh then I need to rephrase the above paragraph, since I don't meant to
> say that. What I want to say is this:
> (1) concentrate as much `unsafe` code as possible, and put it somewhere
>     where everyone can use it (ie the `kernel` crate)
> (2) abstract over common use-patterns of `unsafe` code via safe
>     abstractions
> (3) disallow access to *raw* `bindings::` function calls from drivers.
> 
> From what you write below, I think that we are on the same page for (1)
> and (2). What I want to accomplish with (3) is that we don't have hacky
> drivers that are just like a C driver with `unsafe` sprinkled
> throughout. If you want to do that, just write a C driver instead.
> 
> As Alice already replied, there should be no issue with having an
> `unsafe` function in an Abstraction. But we should strive for them to be
> as few as possible.
> 
>> I am in no way disagreeing with the use of safe abstractions, but I think we
>> should have abstractions where they make sense. This is the case in the vast
>> majority of times, but not in *all* of them.
>> 
>> A simple example is a MMIO read or write. Should a driver be forbidden to call
>> readX/writeX for an address it knows to be valid? How can you possibly write an
>> abstraction for this, when the driver is the only one aware of the actual
>> device addresses, and when the driver author is the person with actual access
>> to the HW docs?
> 
> One idea that I have in this concrete example would be to make the
> driver specify in exactly one place what the addresses are that are
> read/writeable. If there are devices with dynamic addresses, then we
> could additionally provide an `unsafe` API, but link to the safe one for
> people to prefer.
> 
>> If a driver is written partially in Rust, and partially in C, and it gets a
>> pointer to some kcalloc’d memory in C, should It be forbidden to use unsafe
>> in order to build a slice from that pointer? How can you possibly design a
>> general abstraction for something that is, essentially, a driver-internal API?
> 
> This also would be a good example for an exception of (3). In this case,
> you could still write a driver-specific abstraction that does everything
> under the hood and then every place in the driver can use the safe
> abstraction.
> 
>> For these corner cases, a simple safety comment should suffice. By all means,
>> let's strive to push as much of the unsafe bits into the kernel crate. But,
>> IMHO, we shouldn’t treat Rust drivers as some unprivileged entity, they’re
>> also kernel code, after all.
> 
> That is true, but I want to prevent that we just "ship it" and then a
> couple of days later it turns out that there was a good abstraction
> after all. I personally like to spend a lot of time thinking about safe
> abstractions before giving in to `unsafe`, but I understand that we need
> to find a balance. In the end, we can also always change things. But
> when something lands, it most of the time won't get people thinking
> about whether there is a better way of doing things. Not unless the
> status quo is annoying/burdensome, at which point it already "was too
> late", ie there could have been more thought at the beginning.
> 
> ---
> Cheers,
> Benno

I see,

There has been extensive discussion about this topic in this series and I no
longer see any problems. Thanks everybody for all the clarification provided.

— Daniel

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Miguel Ojeda]

On Thu, Aug 8, 2024 at 3:02 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Yeah, I wasn’t referring to formal verification, just a lint that will complain when
> it finds an unsafe block that has no safety comments at all. The clippy lints you
> listed should work fine and, IIUC, Miguel already has a patch to enable (some of) them,
> so I don’t think any further action is needed.

+1, sending a series about that soon.

Cheers,
Miguel

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Boqun Feng]

Hi Benno,

On Wed, Jul 17, 2024 at 10:12:29PM +0000, Benno Lossin wrote:
[...]
> @@ -0,0 +1,246 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. highlight:: rust
> +
> +====================
> +Rust Safety Standard
> +====================
> +
> +Safe Rust code cannot have memory related bugs. This is a guarantee by the Rust compiler. Of course
> +it is not without caveats: no compiler bugs, no bugs in the specification etc. But the possibly most
> +important caveat is that of ``unsafe`` code. ``unsafe`` code needs to follow certain rules in order
> +for safe code to enjoy the no-memory-bugs privilege. A simple example of such a rule is that
> +references must be valid for the duration of their lifetime. If any rule is violated, it can lead
> +to undefined behavior even in safe code! The term undefined behavior in Rust has a lot stricter
> +meaning than in C or C++: UB in Rust is totally forbidden. In C one might rely on the compiler
> +implementation to ensure correct code generation, but that is not the case for Rust. You can read

I don't disagree with your intention here (i.e. we should seek for
UB-free program), however, during the discussion on memory model, I got
response like in [1]:

	... they are technically wrong (violating the C standard), but
	practically well-tested. (and then above I added that there's
	good reasons for why they don't go wrong: volatile compilation
	strategies and reordering constraints relating volatile, inline
	asm, and non-atomics make it so that this almost 'has to' work,
	I think.)

which suggests that we should rely on the compiler implementation to
ensure the "correct" code generation.

Basically, since LKMM relies on a few things that C standard dosen't
say, e.g. votatile accesses on certain types are atomic, behaviors of
asm blocks, dependencies. Let alone we have data_race() where for
example, the diagnostic code accesses the shared variable out of the
core synchronization design.

All of the above is difficult to implement purely UB-free in Rust IIUC.
Of course you could argue the ideal way is to teach Rust how to model
these useful operations/patterns as non-UBs, but that's a relatively
high task:

	Or do we want to go well beyond what happens in C, and actually
	define a memory model that both has the performance
	characteristics required by Linux, and can be defined precisely
	as a language-level graph-based (or ideally operational)
	concurrency memory model? This is a monumental task and a large
	open problem, and should not be on the list of blocking issues
	for anything that needs to get done in the next 5 years. ;)

from Ralf [2].

Again, I don't want to rely on compiler's behaviors on UBs, it's just
the langauge is not ready for some jobs and programmers have to be
"creative".

Regards,
Boqun

[1]: https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/.E2.9C.94.20Rust.20and.20the.20Linux.20Kernel.20Memory.20Model/near/422193212	
[2]: https://github.com/rust-lang/unsafe-code-guidelines/issues/348#issuecomment-1221376388

> +more about UB in Rust
> +`here <https://doc.rust-lang.org/reference/behavior-considered-undefined.html>`_.
> +
[...]

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Benno Lossin]

On 20.07.24 00:11, Boqun Feng wrote:
> Hi Benno,
> 
> On Wed, Jul 17, 2024 at 10:12:29PM +0000, Benno Lossin wrote:
> [...]
>> @@ -0,0 +1,246 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +.. highlight:: rust
>> +
>> +====================
>> +Rust Safety Standard
>> +====================
>> +
>> +Safe Rust code cannot have memory related bugs. This is a guarantee by the Rust compiler. Of course
>> +it is not without caveats: no compiler bugs, no bugs in the specification etc. But the possibly most
>> +important caveat is that of ``unsafe`` code. ``unsafe`` code needs to follow certain rules in order
>> +for safe code to enjoy the no-memory-bugs privilege. A simple example of such a rule is that
>> +references must be valid for the duration of their lifetime. If any rule is violated, it can lead
>> +to undefined behavior even in safe code! The term undefined behavior in Rust has a lot stricter
>> +meaning than in C or C++: UB in Rust is totally forbidden. In C one might rely on the compiler
>> +implementation to ensure correct code generation, but that is not the case for Rust. You can read
> 
> I don't disagree with your intention here (i.e. we should seek for
> UB-free program), however, during the discussion on memory model, I got
> response like in [1]:
> 
> 	... they are technically wrong (violating the C standard), but
> 	practically well-tested. (and then above I added that there's
> 	good reasons for why they don't go wrong: volatile compilation
> 	strategies and reordering constraints relating volatile, inline
> 	asm, and non-atomics make it so that this almost 'has to' work,
> 	I think.)
> 
> which suggests that we should rely on the compiler implementation to
> ensure the "correct" code generation.

I disagree, why can't we get the specification to specify what we need?
I would rather have a compiler and standard that are in sync and give us
what we need, than have a standard that says we aren't allowed to do X,
but compilers still allow you to do X. I don't understand why this is
the case for C (I would bet this is is because of history/legacy).

> Basically, since LKMM relies on a few things that C standard dosen't
> say, e.g. votatile accesses on certain types are atomic, behaviors of
> asm blocks, dependencies. Let alone we have data_race() where for
> example, the diagnostic code accesses the shared variable out of the
> core synchronization design.
> 
> All of the above is difficult to implement purely UB-free in Rust IIUC.
> Of course you could argue the ideal way is to teach Rust how to model
> these useful operations/patterns as non-UBs, but that's a relatively
> high task:
> 
> 	Or do we want to go well beyond what happens in C, and actually
> 	define a memory model that both has the performance
> 	characteristics required by Linux, and can be defined precisely
> 	as a language-level graph-based (or ideally operational)
> 	concurrency memory model? This is a monumental task and a large
> 	open problem, and should not be on the list of blocking issues
> 	for anything that needs to get done in the next 5 years. ;)
> 
> from Ralf [2].
> 
> Again, I don't want to rely on compiler's behaviors on UBs, it's just
> the langauge is not ready for some jobs and programmers have to be
> "creative".

I think this is something that we need to very carefully evaluate on a
case-by-case basis. I think that with Rust we have a clean slate and can
try from the beginning to ensure that there is no
compiler-but-not-specification behavior that we rely upon. AFAIK the
Rust standard moves quicker than the C standard, so we might be able to
influence it more easily.

So what I am trying to say is: let UB be an actually useful term of
forbidden behavior. If you want to convince me otherwise, I think we
should talk specifics and not in this general way, since "sometimes UB
is actually ok" is something that I don't want to accept in Rust as a
general statement. If we have an exception, it should have a damn good
reason to be an exception and then still I don't like it one bit. Can't
we just ask the Rust folks to implement some compiler magic for us that
achieves what we need without relying one some weird compiler quirk?

---
Cheers,
Benno

> Regards,
> Boqun
> 
> [1]: https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/.E2.9C.94.20Rust.20and.20the.20Linux.20Kernel.20Memory.20Model/near/422193212
> [2]: https://github.com/rust-lang/unsafe-code-guidelines/issues/348#issuecomment-1221376388
> 
>> +more about UB in Rust
>> +`here <https://doc.rust-lang.org/reference/behavior-considered-undefined.html>`_.
>> +
> [...]

Re: [RFC PATCH 1/5] doc: rust: create safety standard

[Dirk Behme]

Hi Benno,

Am 18.07.24 um 00:12 schrieb Benno Lossin:
...
> +to undefined behavior even in safe code! The term undefined behavior in Rust has a lot stricter
> +meaning than in C or C++: UB in Rust is totally forbidden. In C one might rely on the compiler

Just a minor formal thing: An abbreviation should be introduced (in 
brackets) before using it the first time. So I would propose:

"... undefined behavior (UB)  ..."

Dirk

[RFC PATCH 2/5] doc: rust: safety standard: add examples

[Benno Lossin]

Add examples of good and bad safety documentation.

There aren't many examples at the moment, as I hope to add more during
discussions, since coming up with examples on my own is very difficult.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
 .../rust/safety-standard/examples.rst         | 70 +++++++++++++++++++
 Documentation/rust/safety-standard/index.rst  | 23 ++++--
 2 files changed, 86 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/rust/safety-standard/examples.rst

diff --git a/Documentation/rust/safety-standard/examples.rst b/Documentation/rust/safety-standard/examples.rst
new file mode 100644
index 000000000000..d66ef3f8954a
--- /dev/null
+++ b/Documentation/rust/safety-standard/examples.rst
@@ -0,0 +1,70 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. highlight:: rust
+
+Examples
+========
+
+Unsound APIs
+------------
+
+Simple Unsound Function
+***********************
+::
+
+    struct Data {
+        a: usize,
+    }
+
+    fn access_a(data: *mut Data) -> usize {
+        unsafe { (*data).a }
+    }
+
+One would normally call this function as follows, which does not trigger UB::
+
+    fn main() {
+        let mut d = Data { a: 42 };
+        println!("{}", access_a(&mut d));
+    }
+
+However, a caller could also call it like this, which triggers UB using only safe code::
+
+    fn main() {
+        println!("{}", access_a(core::ptr::null_mut()));
+    }
+
+And this would result in a dereference of a null pointer.
+
+
+Sound ``unsafe`` Code
+---------------------
+
+The Importance of the API Boundary
+**********************************
+
+Is the following API sound?::
+
+    fn foo(r: &mut u32) {
+        let ptr: *mut u32 = r;
+        let val;
+        unsafe {
+            val = *ptr;
+            *ptr = 0;
+        }
+    }
+
+It better be sound, but one could argue that it is unsound, since one could replace the ptr
+initialization by ``ptr = core::ptr::null_mut()``::
+
+    fn foo(r: &mut u32) {
+        let ptr: *mut u32 = core::ptr::null_mut();
+        let val;
+        unsafe {
+            val = *ptr;
+            *ptr = 0;
+        }
+    }
+
+But this modification is not allowed, since it goes beyond the API boundary of ``foo``. This way
+any ``unsafe`` code that relies on surrounding safe code could be shown to be unsound. Instead one
+should only consider safe code using the API, in this case, there is no way to make the code
+incorrect, since a reference is always valid to dereference during its lifetime.
diff --git a/Documentation/rust/safety-standard/index.rst b/Documentation/rust/safety-standard/index.rst
index 1cbc8d3dea04..bebebda06831 100644
--- a/Documentation/rust/safety-standard/index.rst
+++ b/Documentation/rust/safety-standard/index.rst
@@ -92,21 +92,28 @@ And this would result in a dereference of a null pointer.
 In its essence, a sound API means that if someone only writes safe code, they can never encounter UB
 even if they call safe code that calls ``unsafe`` code behind the scenes.
 
+For more examples of unsound code see examples.rst.
+
 Because unsoundness issues have the potential for allowing safe code to experience UB, they are
-treated similarly to actual bugs with UB. Their fixes should also be included in the  stable tree.
+treated similarly to real UB. Their fixes should also be included in the stable tree.
 
 Safety Documentation
 ====================
 
-After trying to minimize and remove as much ``unsafe`` code as possible, there still is some left.
-This is because some things are just not possible in only safe code. This last part of ``unsafe``
-code must still be correct. Helping with that is the safety documentation: it meticulously documents
-the various requirements and justifications for every line of ``unsafe`` code. That way it can be
-ensured that all ``unsafe`` code is sound without anyone needing to know the whole kernel at once.
+No matter how hard one tries to remove ``unsafe`` code, it is impossible to completely get rid of it
+in the Kernel. There are things that are impossible for safe code. For example interacting with the
+C side. So one can never be completely sure that there are no memory issues lurking somewhere.
+
+This is where safety documentation helps: it meticulously documents the various requirements and
+justifications for every line of ``unsafe`` code. That way the risk of writing unsound ``unsafe``
+code is reduced drastically.
+
 The gist of the idea is this: every ``unsafe`` operation documents its requirements and every
 location that uses an ``unsafe`` operation documents for every requirement a justification why they
 are fulfilled. If now all requirements and justifications are correct, then there can only be sound
-``unsafe`` code.
+``unsafe`` code. Reducing the global problem of correctness of the whole kernel to the correctness
+of each and every ``unsafe`` code block makes it a local problem. Local problems are a lot easier to
+handle, since each instance can be fixed/reviewed independently.
 
 The ``unsafe`` keywords has two different meanings depending on the context it is used in:
 
@@ -238,6 +245,8 @@ Further Pages
 .. toctree::
    :maxdepth: 1
 
+   examples
+
 .. only::  subproject and html
 
    Indices
-- 
2.45.1

Re: [RFC PATCH 2/5] doc: rust: safety standard: add examples

[Daniel Almeida]

Hi Benno,

> On 17 Jul 2024, at 19:12, Benno Lossin <benno.lossin@proton.me> wrote:
> 
> Add examples of good and bad safety documentation.
> 
> There aren't many examples at the moment, as I hope to add more during
> discussions, since coming up with examples on my own is very difficult.
> 
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> .../rust/safety-standard/examples.rst         | 70 +++++++++++++++++++
> Documentation/rust/safety-standard/index.rst  | 23 ++++--
> 2 files changed, 86 insertions(+), 7 deletions(-)
> create mode 100644 Documentation/rust/safety-standard/examples.rst
> 
> diff --git a/Documentation/rust/safety-standard/examples.rst b/Documentation/rust/safety-standard/examples.rst
> new file mode 100644
> index 000000000000..d66ef3f8954a
> --- /dev/null
> +++ b/Documentation/rust/safety-standard/examples.rst
> @@ -0,0 +1,70 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. highlight:: rust
> +
> +Examples
> +========
> +
> +Unsound APIs
> +------------
> +
> +Simple Unsound Function
> +***********************
> +::
> +
> +    struct Data {
> +        a: usize,
> +    }
> +
> +    fn access_a(data: *mut Data) -> usize {
> +        unsafe { (*data).a }
> +    }
> +
> +One would normally call this function as follows, which does not trigger UB::
> +
> +    fn main() {
> +        let mut d = Data { a: 42 };
> +        println!("{}", access_a(&mut d));
> +    }
> +
> +However, a caller could also call it like this, which triggers UB using only safe code::
> +
> +    fn main() {
> +        println!("{}", access_a(core::ptr::null_mut()));
> +    }
> +
> +And this would result in a dereference of a null pointer.
> +
> +
> +Sound ``unsafe`` Code
> +---------------------
> +
> +The Importance of the API Boundary
> +**********************************
> +
> +Is the following API sound?::
> +
> +    fn foo(r: &mut u32) {
> +        let ptr: *mut u32 = r;
> +        let val;
> +        unsafe {
> +            val = *ptr;
> +            *ptr = 0;
> +        }
> +    }
> +
> +It better be sound, but one could argue that it is unsound, since one could replace the ptr
> +initialization by ``ptr = core::ptr::null_mut()``::
> +
> +    fn foo(r: &mut u32) {
> +        let ptr: *mut u32 = core::ptr::null_mut();
> +        let val;
> +        unsafe {
> +            val = *ptr;
> +            *ptr = 0;
> +        }
> +    }
> +
> +But this modification is not allowed, since it goes beyond the API boundary of ``foo``. This way
> +any ``unsafe`` code that relies on surrounding safe code could be shown to be unsound. Instead one
> +should only consider safe code using the API, in this case, there is no way to make the code
> +incorrect, since a reference is always valid to dereference during its lifetime.

I find this paragraph a bit confusing. Maybe this can be clarified a bit further?

> diff --git a/Documentation/rust/safety-standard/index.rst b/Documentation/rust/safety-standard/index.rst
> index 1cbc8d3dea04..bebebda06831 100644
> --- a/Documentation/rust/safety-standard/index.rst
> +++ b/Documentation/rust/safety-standard/index.rst
> @@ -92,21 +92,28 @@ And this would result in a dereference of a null pointer.
> In its essence, a sound API means that if someone only writes safe code, they can never encounter UB
> even if they call safe code that calls ``unsafe`` code behind the scenes.
> 
> +For more examples of unsound code see examples.rst.
> +
> Because unsoundness issues have the potential for allowing safe code to experience UB, they are
> -treated similarly to actual bugs with UB. Their fixes should also be included in the  stable tree.
> +treated similarly to real UB. Their fixes should also be included in the stable tree.
> 
> Safety Documentation
> ====================
> 
> -After trying to minimize and remove as much ``unsafe`` code as possible, there still is some left.
> -This is because some things are just not possible in only safe code. This last part of ``unsafe``
> -code must still be correct. Helping with that is the safety documentation: it meticulously documents
> -the various requirements and justifications for every line of ``unsafe`` code. That way it can be
> -ensured that all ``unsafe`` code is sound without anyone needing to know the whole kernel at once.
> +No matter how hard one tries to remove ``unsafe`` code, it is impossible to completely get rid of it
> +in the Kernel. There are things that are impossible for safe code. For example interacting with the
> +C side. So one can never be completely sure that there are no memory issues lurking somewhere.
> +
> +This is where safety documentation helps: it meticulously documents the various requirements and
> +justifications for every line of ``unsafe`` code. That way the risk of writing unsound ``unsafe``
> +code is reduced drastically.
> +
> The gist of the idea is this: every ``unsafe`` operation documents its requirements and every
> location that uses an ``unsafe`` operation documents for every requirement a justification why they
> are fulfilled. If now all requirements and justifications are correct, then there can only be sound
> -``unsafe`` code.
> +``unsafe`` code. Reducing the global problem of correctness of the whole kernel to the correctness
> +of each and every ``unsafe`` code block makes it a local problem. Local problems are a lot easier to
> +handle, since each instance can be fixed/reviewed independently.
> 
> The ``unsafe`` keywords has two different meanings depending on the context it is used in:
> 
> @@ -238,6 +245,8 @@ Further Pages
> .. toctree::
>    :maxdepth: 1
> 
> +   examples
> +
> .. only::  subproject and html
> 
>    Indices
> -- 
> 2.45.1
> 
> 

— Daniel

Re: [RFC PATCH 2/5] doc: rust: safety standard: add examples

[Daniel Almeida]

Sorry, ended up replying to this using my personal email.

Sending it again.

—————

Hi Benno,

> On 17 Jul 2024, at 19:12, Benno Lossin <benno.lossin@proton.me> wrote:
> 
> Add examples of good and bad safety documentation.
> 
> There aren't many examples at the moment, as I hope to add more during
> discussions, since coming up with examples on my own is very difficult.
> 
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> .../rust/safety-standard/examples.rst         | 70 +++++++++++++++++++
> Documentation/rust/safety-standard/index.rst  | 23 ++++--
> 2 files changed, 86 insertions(+), 7 deletions(-)
> create mode 100644 Documentation/rust/safety-standard/examples.rst
> 
> diff --git a/Documentation/rust/safety-standard/examples.rst b/Documentation/rust/safety-standard/examples.rst
> new file mode 100644
> index 000000000000..d66ef3f8954a
> --- /dev/null
> +++ b/Documentation/rust/safety-standard/examples.rst
> @@ -0,0 +1,70 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. highlight:: rust
> +
> +Examples
> +========
> +
> +Unsound APIs
> +------------
> +
> +Simple Unsound Function
> +***********************
> +::
> +
> +    struct Data {
> +        a: usize,
> +    }
> +
> +    fn access_a(data: *mut Data) -> usize {
> +        unsafe { (*data).a }
> +    }
> +
> +One would normally call this function as follows, which does not trigger UB::
> +
> +    fn main() {
> +        let mut d = Data { a: 42 };
> +        println!("{}", access_a(&mut d));
> +    }
> +
> +However, a caller could also call it like this, which triggers UB using only safe code::
> +
> +    fn main() {
> +        println!("{}", access_a(core::ptr::null_mut()));
> +    }
> +
> +And this would result in a dereference of a null pointer.
> +
> +
> +Sound ``unsafe`` Code
> +---------------------
> +
> +The Importance of the API Boundary
> +**********************************
> +
> +Is the following API sound?::
> +
> +    fn foo(r: &mut u32) {
> +        let ptr: *mut u32 = r;
> +        let val;
> +        unsafe {
> +            val = *ptr;
> +            *ptr = 0;
> +        }
> +    }
> +
> +It better be sound, but one could argue that it is unsound, since one could replace the ptr
> +initialization by ``ptr = core::ptr::null_mut()``::
> +
> +    fn foo(r: &mut u32) {
> +        let ptr: *mut u32 = core::ptr::null_mut();
> +        let val;
> +        unsafe {
> +            val = *ptr;
> +            *ptr = 0;
> +        }
> +    }
> +
> +But this modification is not allowed, since it goes beyond the API boundary of ``foo``. This way
> +any ``unsafe`` code that relies on surrounding safe code could be shown to be unsound. Instead one
> +should only consider safe code using the API, in this case, there is no way to make the code
> +incorrect, since a reference is always valid to dereference during its lifetime.

I find this paragraph a bit confusing. Maybe this can be clarified a bit further?


— Daniel

Re: [RFC PATCH 2/5] doc: rust: safety standard: add examples

[Benno Lossin]

On 19.07.24 18:36, Daniel Almeida wrote:
>> On 17 Jul 2024, at 19:12, Benno Lossin <benno.lossin@proton.me> wrote:
>> +Sound ``unsafe`` Code
>> +---------------------
>> +
>> +The Importance of the API Boundary
>> +**********************************
>> +
>> +Is the following API sound?::
>> +
>> +    fn foo(r: &mut u32) {
>> +        let ptr: *mut u32 = r;
>> +        let val;
>> +        unsafe {
>> +            val = *ptr;
>> +            *ptr = 0;
>> +        }
>> +    }
>> +
>> +It better be sound, but one could argue that it is unsound, since one could replace the ptr
>> +initialization by ``ptr = core::ptr::null_mut()``::
>> +
>> +    fn foo(r: &mut u32) {
>> +        let ptr: *mut u32 = core::ptr::null_mut();
>> +        let val;
>> +        unsafe {
>> +            val = *ptr;
>> +            *ptr = 0;
>> +        }
>> +    }
>> +
>> +But this modification is not allowed, since it goes beyond the API boundary of ``foo``. This way
>> +any ``unsafe`` code that relies on surrounding safe code could be shown to be unsound. Instead one
>> +should only consider safe code using the API, in this case, there is no way to make the code
>> +incorrect, since a reference is always valid to dereference during its lifetime.
> 
> I find this paragraph a bit confusing. Maybe this can be clarified a bit further?

I will try to rephrase this, tell me if it helps. When checking if an
API is sound, you are not allowed to change the code behind the API.
That is because `unsafe` code often relies on the surrounding safe code
to work properly. In the example above, safe code ensures that the raw
pointer `ptr` is valid. This is OK (and also very necessary), since we
expect people to be *aware* of the `unsafe` block and thus more
carefully review the changes in surrounding safe code. If you have safe
code that only interfaces with other safe code you don't need to be this
careful.

Note that this heavily depends on where you put the API boundary. In our
case, we generally have this boundary: driver code <-> `kernel` crate.
But if your driver requires very specific helper code that does not fit
into the `kernel` crate, then you might also have an API boundary there.

If it doesn't help, then it would great to get some more detailed
questions which part(s) you need help with.

---
Cheers,
Benno

Re: [RFC PATCH 2/5] doc: rust: safety standard: add examples

[Daniel Almeida]

Hi Benno,

> 
> I will try to rephrase this, tell me if it helps. When checking if an
> API is sound, you are not allowed to change the code behind the API.
> That is because `unsafe` code often relies on the surrounding safe code
> to work properly. In the example above, safe code ensures that the raw
> pointer `ptr` is valid. This is OK (and also very necessary), since we
> expect people to be *aware* of the `unsafe` block and thus more
> carefully review the changes in surrounding safe code. If you have safe
> code that only interfaces with other safe code you don't need to be this
> careful.
> 
> Note that this heavily depends on where you put the API boundary. In our
> case, we generally have this boundary: driver code <-> `kernel` crate.
> But if your driver requires very specific helper code that does not fit
> into the `kernel` crate, then you might also have an API boundary there.
> 
> If it doesn't help, then it would great to get some more detailed
> questions which part(s) you need help with.
> 
> ---
> Cheers,
> Benno
> 
> 

Yes, I think this is more clear, but note that this explanation is more thorough
than the actual example.

My point being, maybe you should take some of what you just wrote and put it
into the actual docs.

— Daniel

Re: [RFC PATCH 2/5] doc: rust: safety standard: add examples

[Benno Lossin]

On 08.08.24 15:10, Daniel Almeida wrote:
> Hi Benno,
> 
>>
>> I will try to rephrase this, tell me if it helps. When checking if an
>> API is sound, you are not allowed to change the code behind the API.
>> That is because `unsafe` code often relies on the surrounding safe code
>> to work properly. In the example above, safe code ensures that the raw
>> pointer `ptr` is valid. This is OK (and also very necessary), since we
>> expect people to be *aware* of the `unsafe` block and thus more
>> carefully review the changes in surrounding safe code. If you have safe
>> code that only interfaces with other safe code you don't need to be this
>> careful.
>>
>> Note that this heavily depends on where you put the API boundary. In our
>> case, we generally have this boundary: driver code <-> `kernel` crate.
>> But if your driver requires very specific helper code that does not fit
>> into the `kernel` crate, then you might also have an API boundary there.
>>
>> If it doesn't help, then it would great to get some more detailed
>> questions which part(s) you need help with.
>>
>> ---
>> Cheers,
>> Benno
>>
>>
> 
> Yes, I think this is more clear, but note that this explanation is more thorough
> than the actual example.
> 
> My point being, maybe you should take some of what you just wrote and put it
> into the actual docs.

Yeah that was part of my plan :)

Thanks for taking a look.

---
Cheers,
Benno

[RFC PATCH 3/5] doc: rust: safety standard: add guarantees and type invariants

[Benno Lossin]

Add documents about `# Guarantees` and `# Invariants` sections.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
 .../rust/safety-standard/guarantee.rst        |  9 +++++++++
 Documentation/rust/safety-standard/index.rst  | 16 +++++++++++++++
 .../rust/safety-standard/type-invariants.rst  | 20 +++++++++++++++++++
 3 files changed, 45 insertions(+)
 create mode 100644 Documentation/rust/safety-standard/guarantee.rst
 create mode 100644 Documentation/rust/safety-standard/type-invariants.rst

diff --git a/Documentation/rust/safety-standard/guarantee.rst b/Documentation/rust/safety-standard/guarantee.rst
new file mode 100644
index 000000000000..4d8c811c2bed
--- /dev/null
+++ b/Documentation/rust/safety-standard/guarantee.rst
@@ -0,0 +1,9 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Guarantees
+==========
+
+Functions and ``struct``, ``enum`` and ``union`` types can list guarantees that ``unsafe`` code can
+rely upon in the ``# Guarantees`` section. Guarantees are listed in an unordered markdown list.
+The wording of guarantees is identical to the wording of requirements.rst. To refer to the return
+value of the function, use ``$ret``.
diff --git a/Documentation/rust/safety-standard/index.rst b/Documentation/rust/safety-standard/index.rst
index bebebda06831..40b17f59709c 100644
--- a/Documentation/rust/safety-standard/index.rst
+++ b/Documentation/rust/safety-standard/index.rst
@@ -221,6 +221,20 @@ similarly to :ref:`unsafe-Blocks`::
     // SAFETY: <justification>
     unsafe impl Foo for Bar {}
 
+Guarantees
+==========
+
+Functions are also allowed to declare certain guarantees that ``unsafe`` code is able to rely upon.
+For example when returning a raw pointer, a common guarantee would be to state that it is valid. See
+guarantee.rst for more info. Importantly, guarantees can also be given by safe functions.
+
+Type Invariants
+---------------
+
+Type invariants are a kind of guarantee. Like their name suggests, they always hold (invariant --
+never changing). They can only be specified on ``struct``, ``enum`` or ``union`` types. See
+type-invariants.rst for more info.
+
 General Rules
 =============
 
@@ -246,6 +260,8 @@ Further Pages
    :maxdepth: 1
 
    examples
+   guarantee
+   type-invariants
 
 .. only::  subproject and html
 
diff --git a/Documentation/rust/safety-standard/type-invariants.rst b/Documentation/rust/safety-standard/type-invariants.rst
new file mode 100644
index 000000000000..dd7e9bda80e5
--- /dev/null
+++ b/Documentation/rust/safety-standard/type-invariants.rst
@@ -0,0 +1,20 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Type Invariants
+===============
+
+``struct``, ``enum`` and ``union`` types can list type invariants. They are a kind of
+:doc:`guarantee <guarantee>` that ``unsafe`` code can rely upon. They are listed in an unordered
+list in the ``## Invariants`` subsection of the ``# Guarantees`` section. The wording of invariants
+is identical to the wording of requirements.rst. Invariants hold for the entire lifetime of an
+object. During the call of the ``drop`` function these invariants may be violated, since objects
+that are being dropped can never be observed.
+
+Objects with invariants need ``INVARIANT`` comments whenever they are constructed or a field with an
+invariant is modified. The comment is similar to ``SAFETY`` comments and needs to justify that the
+invariant holds. See justifications.rst for how to justify requirements.
+
+Sometimes it is needed to violate an invariant temporarily. For example when inside of a function
+one can temporarily violate the invariant, as long as it is later reestablished and no external code
+can observe the violation. These violations must also be documented using the ``INVARIANT``
+comments.
-- 
2.45.1

[RFC PATCH 4/5] doc: rust: safety standard: add safety requirements

[Benno Lossin]

Add standardized safety requirements.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
 Documentation/rust/safety-standard/index.rst  |  5 ++
 .../rust/safety-standard/requirements.rst     | 80 +++++++++++++++++++
 2 files changed, 85 insertions(+)
 create mode 100644 Documentation/rust/safety-standard/requirements.rst

diff --git a/Documentation/rust/safety-standard/index.rst b/Documentation/rust/safety-standard/index.rst
index 40b17f59709c..2ef82d7dfbd8 100644
--- a/Documentation/rust/safety-standard/index.rst
+++ b/Documentation/rust/safety-standard/index.rst
@@ -157,6 +157,8 @@ The safety requirements have to be documented in the so called safety section::
         // ...
     }
 
+See requirements.rst for a full list of standardized safety requirements.
+
 .. _unsafe-Blocks:
 
 ``unsafe`` Blocks
@@ -208,6 +210,8 @@ are called safety requirements and need to be documented in the same way::
     /// <safety requirements>
     unsafe trait Foo {}
 
+See requirements.rst for a full list of standardized safety requirements.
+
 ``unsafe`` Impls
 ----------------
 
@@ -262,6 +266,7 @@ Further Pages
    examples
    guarantee
    type-invariants
+   requirements
 
 .. only::  subproject and html
 
diff --git a/Documentation/rust/safety-standard/requirements.rst b/Documentation/rust/safety-standard/requirements.rst
new file mode 100644
index 000000000000..b86bfb98179e
--- /dev/null
+++ b/Documentation/rust/safety-standard/requirements.rst
@@ -0,0 +1,80 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. highlight:: rust
+
+===================
+Safety Requirements
+===================
+
+There are many different kinds of safety requirements. The simplest example is the validity of raw
+pointers. But there is no limit to what they may require.
+
+Safety requirements are listed in an unordered markdown list in the ``# Safety`` section on
+``unsafe fn`` and ``unsafe trait`` items. Each list item should only contain a single requirement.
+The items should not specify the same requirement multiple times, especially not by expressing it in
+different terms.  The ``# Safety`` section should only consist of the list of safety requirements,
+if there is additional information, it should be documented in a different section.
+
+Common Safety Requirements
+==========================
+
++------------------------+---------------------+---------------------------------------------------+
+| Syntax                 | Meta Variables      | Meaning                                           |
+|                        |                     |                                                   |
++========================+=====================+===================================================+
+| ``ptr`` is valid for   |                     | Abbreviation for:                                 |
+| reads and writes.      |                     |                                                   |
+|                        | * ``ptr: *mut T``   | * ``ptr`` is valid for reads.                     |
+|                        |                     | * ``ptr`` is valid for writes.                    |
++------------------------+---------------------+---------------------------------------------------+
+| ``ptr`` is valid for   |                     | Abbreviation for:                                 |
+| reads.                 |                     |                                                   |
+|                        | * ``ptr: *const T`` | * ``ptr`` is valid for reads up to                |
+|                        |                     |   ``size_of::<T>()`` bytes for the duration of    |
+|                        |                     |   this function call.                             |
++------------------------+---------------------+---------------------------------------------------+
+| ``ptr`` is valid for   |                     | Abbreviation for:                                 |
+| writes.                |                     |                                                   |
+|                        | * ``ptr: *mut T``   | * ``ptr`` is valid for writes up to               |
+|                        |                     |   ``size_of::<T>()`` bytes for the duration of    |
+|                        |                     |   this function call.                             |
++------------------------+---------------------+---------------------------------------------------+
+| ``ptr`` is valid for   |                     | For the duration of ``'a``:                       |
+| reads up to ``size``   |                     |                                                   |
+| bytes for the duration | * ``ptr: *const T`` | * The pointer ``ptr`` is dereferenceable for      |
+| of ``'a``.             | * ``size: usize``   |   ``size`` bytes: all bytes with offset           |
+|                        |                     |   ``0..size`` have to be part of the same         |
+|                        |                     |   allocated object and it has to be alive.        |
+|                        |                     | * No concurrent write operation may occur to      |
+|                        |                     |   ``ptr`` at any offset between ``0..size``.      |
+|                        |                     | * The value at ``ptr`` is a valid instance of     |
+|                        |                     |   the type ``T``.                                 |
+|                        |                     |                                                   |
+|                        |                     | Additionally ``ptr`` must be:                     |
+|                        |                     |                                                   |
+|                        |                     | * non-null,                                       |
+|                        |                     | * aligned to ``align_of::<T>()`` i.e.             |
+|                        |                     |   ``ptr.addr() % align_of::<T>() == 0``.          |
++------------------------+---------------------+---------------------------------------------------+
+| ``ptr`` is valid for   |                     | For the duration of ``'a``:                       |
+| writes up to ``size``  |                     |                                                   |
+| bytes for the duration | * ``ptr: *mut T``   | * The pointer ``ptr`` is dereferenceable for      |
+| of ``'a``.             | * ``size: usize``   |   ``size`` bytes: all bytes with offset           |
+|                        |                     |   ``0..size`` have to be part of the same         |
+|                        |                     |   allocated object and it has to be alive.        |
+|                        |                     | * No concurrent read or write operation may occur |
+|                        |                     |   to ``ptr`` at any offset between ``0..size``.   |
+|                        |                     |                                                   |
+|                        |                     | Additionally ``ptr`` must be:                     |
+|                        |                     |                                                   |
+|                        |                     | * non-null,                                       |
+|                        |                     | * aligned to ``align_of::<T>()`` i.e.             |
+|                        |                     |   ``ptr.addr() % align_of::<T>() == 0``.          |
++------------------------+---------------------+---------------------------------------------------+
+
+
+Custom Safety Requirements
+==========================
+
+There are of course situations where the safety requirements listed above are insufficient. In that
+case the author can try to come up with their own safety requirement wording and ask the reviewers
+what they think. If the requirement is common enough, it should be added to the list above.
-- 
2.45.1

[RFC PATCH 5/5] doc: rust: safety standard: add justifications

[Benno Lossin]

Add standardized justifications that are used to justify the safety
requirements.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
 Documentation/rust/safety-standard/index.rst  |  5 +++
 .../rust/safety-standard/justifications.rst   | 40 +++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 Documentation/rust/safety-standard/justifications.rst

diff --git a/Documentation/rust/safety-standard/index.rst b/Documentation/rust/safety-standard/index.rst
index 2ef82d7dfbd8..db62ad01ebe0 100644
--- a/Documentation/rust/safety-standard/index.rst
+++ b/Documentation/rust/safety-standard/index.rst
@@ -191,6 +191,8 @@ block. For example::
 
 In this case it is more readable to not split the block into multiple parts.
 
+See justifications.rst for a full list of standardized justifications.
+
 ``unsafe`` Traits
 -----------------
 
@@ -225,6 +227,8 @@ similarly to :ref:`unsafe-Blocks`::
     // SAFETY: <justification>
     unsafe impl Foo for Bar {}
 
+See justifications.rst for a full list of standardized justifications.
+
 Guarantees
 ==========
 
@@ -267,6 +271,7 @@ Further Pages
    guarantee
    type-invariants
    requirements
+   justifications
 
 .. only::  subproject and html
 
diff --git a/Documentation/rust/safety-standard/justifications.rst b/Documentation/rust/safety-standard/justifications.rst
new file mode 100644
index 000000000000..72b6943f3d40
--- /dev/null
+++ b/Documentation/rust/safety-standard/justifications.rst
@@ -0,0 +1,40 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. highlight:: rust
+
+==============
+Justifications
+==============
+
+Since there are so many different safety-requirements.rst, there are also many different ways to
+justify them.
+
+Justifications are listed in an unordered markdown list in the ``SAFETY`` comments on ``unsafe``
+blocks and ``unsafe impl``. The order and elements of the list must match the list present in the
+``# Safety`` section on the respective ``unsafe`` item (e.g. the function or the trait).
+
+Common Justifications
+=====================
+
+In order to use the justifications from the table below, first repeat the safety requirement and
+then write ``by <justification>`` where justification is from the table below.
+If you need the conjunction of multiple justifications, then you just intersperse them with "and".
+
+The term "goal safety requirement" is referring to the requirement that you are trying to justify.
+
++---------------------------+----------------------------------------------------------------------+
+| Syntax                    | Meaning/Justified Safety Requirement                                 |
++===========================+======================================================================+
+| function requirements     | The goal safety requirement is provided by the surrounding function, |
+|                           | as it also has it (or a stronger statement) as a safety requirement. |
++---------------------------+----------------------------------------------------------------------+
+| type invariant of ``T``   | The given safety requirement is provided by the type invariant of    |
+|                           | ``T``, as it also has it (or a stronger statement) as a type         |
+|                           | invariant.                                                           |
++---------------------------+----------------------------------------------------------------------+
+| reference validity        | When turning a (mutable) reference into a pointer, that pointer will |
+|                           | be valid for reads (and writes).                                     |
++---------------------------+----------------------------------------------------------------------+
+| function guarantee of     | The goal safety requirement is provided by the called function       |
+| ``$function``             | ``$function``, as it has it (or a stronger statement) listed as a    |
+|                           | :doc:`guarantee <guarantee>`.                                        |
++---------------------------+----------------------------------------------------------------------+
-- 
2.45.1