`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.1Hi 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
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>`_. > + [...]
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>`_. >> + > [...]
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
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
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
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
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
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
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
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
> 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
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.
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
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
>
>
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/
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
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
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
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
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.
© 2016 - 2025 Red Hat, Inc.