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.1Sorry, 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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.