Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing // SAFETY comments #351

Open
ojeda opened this issue Jun 4, 2021 · 2 comments
Open

Add missing // SAFETY comments #351

ojeda opened this issue Jun 4, 2021 · 2 comments
Labels
easy Expected to be an easy issue to resolve. good first issue Good for newcomers • lib Related to the `rust/` library. medium Expected to be an issue of medium difficulty to resolve.

Comments

@ojeda
Copy link
Member

ojeda commented Jun 4, 2021

We do not allow new unsafe comments to enter the codebase without a // SAFETY comment. Nowadays, we use Clippy's undocumented_unsafe_blocks to check for that. However, some // SAFETY: TODO. comments remain (please see the rust-next branch). The issue is about cleaning those up.

It is not needed to fix all the comments at once (in fact, a given area/module/file should be cleaned independently), so this is a set of "good first issues". Please note that some of the comments may be easier or harder to figure out than others.


This requires submitting a proper patch to the LKML and the Rust for Linux mailing list. Please recall to test your changes (including generating the documentation if changed, running the Rust doctests if changed, etc.), to use a proper title for the commit, to sign your commit under the Developer's Certificate of Origin and to add a Suggested-by: tag and a Link: tag to this issue. Please see https://rust-for-linux.com/contributing for details.

@ojeda ojeda added • lib Related to the `rust/` library. prio: high good first issue Good for newcomers labels Jun 4, 2021
@nbdd0121
Copy link
Member

nbdd0121 commented Jun 4, 2021

First of all, remove the unsafe keyword and compile again so that the compiler tells you 1) what subexpressions need a block (sometimes there is more than one) and 2) what is the reason unsafe is needed (e.g. the simplest case being a FFI call).

There is an allow-by-default unsafe_code lint which is designed for auditing unsafe usage.

@LeSeulArtichaut
Copy link

Here's a Python script to find undocumented unsafe blocks, borrowing logic from rust-lang/rust's tidy checks:

import os


def check_safety_comments(path):
    with open(path, 'r') as f:
        last_safety_comment = False
        for i, line in enumerate(f):
            if 'unsafe {' in line and not line.lstrip().startswith('//') and not last_safety_comment:
                print(f"{path}:{i + 1}: {line.strip()}")
            if '// SAFETY' in line:
                last_safety_comment = True
            elif not line.lstrip().startswith('//') and line.strip() != '':
                last_safety_comment = False


rust_dir = os.path.dirname(os.path.realpath(__file__))
kernel_dir = os.path.join(rust_dir, 'kernel')
for dirpath, _, filenames in os.walk(kernel_dir):
    for filename in filenames:
        if filename.endswith('.rs'):
            check_safety_comments(os.path.join(dirpath, filename))

If placed in the rust/ folder, it will check every file in the kernel crate.

DjLogozzo added a commit to DjLogozzo/linux-rust that referenced this issue Nov 22, 2021
Contributes to Rust-for-Linux#351

Added `// SAFETY` docs to `fn proc_handler`
`rust/kernel/sysctl.rs:97:1`
`rust/kernel/sysctl.rs:113:5`
`rust/kernel/sysctl.rs:115:9`
`rust/kernel/sysctl.rs:120:5`
`rust/kernel/sysctl.rs:124:5`
`rust/kernel/sysctl.rs:136:5`
`rust/kernel/sysctl.rs:138:5`

Added `// SAFETY` docs to `fn init_lock`
`rust/kernel/sync/mutex.rs:85:9`

Added `// SAFETY` docs to `fn init_lock`
`rust/kernel/sync/spinlock.rs:144:9`

Added `// SAFETY` docs to `fn init`
`rust/kernel/sync/condvar.rs:135:9`

Added `// SAFETY` docs to `fn read`
`rust/kernel/pages.rs:96:9`

Added `// SAFETY` docs to `fn write`
`rust/kernel/pages.rs:125:9`

Added `// SAFETY` docs to `fn rust_fmt_argument`
`rust/kernel/print.rs:17:1`
`rust/kernel/print.rs:62:5`

Added `// SAFETY` docs to `fn set_param`
`rust/kernel/module_param.rs:63:5`
`rust/kernel/module_param.rs:74:13`
`rust/kernel/module_param.rs:80:17`
`rust/kernel/module_param.rs:83:17`

Added `// SAFETY` docs to `fn get_param`
`rust/kernel/module_param.rs:104:9`
`rust/kernel/module_param.rs:108:9

Added `// SAFETY` docs to `fn free`
`rust/kernel/module_param.rs:124:9

Signed-off-by: Dee-Jay Logozzo <git@djl.id.au>
DjLogozzo added a commit to DjLogozzo/linux-rust that referenced this issue Nov 22, 2021
Contributes to Rust-for-Linux#351

Added `// SAFETY` docs to `fn proc_handler`
`rust/kernel/sysctl.rs:97:1`
`rust/kernel/sysctl.rs:113:5`
`rust/kernel/sysctl.rs:115:9`
`rust/kernel/sysctl.rs:120:5`
`rust/kernel/sysctl.rs:124:5`
`rust/kernel/sysctl.rs:136:5`
`rust/kernel/sysctl.rs:138:5`

Added `// SAFETY` docs to `fn init_lock`
`rust/kernel/sync/mutex.rs:85:9`

Added `// SAFETY` docs to `fn init_lock`
`rust/kernel/sync/spinlock.rs:144:9`

Added `// SAFETY` docs to `fn init`
`rust/kernel/sync/condvar.rs:135:9`

Added `// SAFETY` docs to `fn read`
`rust/kernel/pages.rs:96:9`

Added `// SAFETY` docs to `fn write`
`rust/kernel/pages.rs:125:9`

Added `// SAFETY` docs to `fn rust_fmt_argument`
`rust/kernel/print.rs:17:1`
`rust/kernel/print.rs:62:5`

Added `// SAFETY` docs to `fn set_param`
`rust/kernel/module_param.rs:63:5`
`rust/kernel/module_param.rs:74:13`
`rust/kernel/module_param.rs:80:17`
`rust/kernel/module_param.rs:83:17`

Added `// SAFETY` docs to `fn get_param`
`rust/kernel/module_param.rs:104:9`
`rust/kernel/module_param.rs:108:9`

Added `// SAFETY` docs to `fn free`
`rust/kernel/module_param.rs:124:9`

Signed-off-by: Dee-Jay Logozzo <git@djl.id.au>
DjLogozzo added a commit to DjLogozzo/linux-rust that referenced this issue Nov 22, 2021
Contributes to Rust-for-Linux#351

Added `// SAFETY` docs to `fn proc_handler`
`rust/kernel/sysctl.rs:97:1`
`rust/kernel/sysctl.rs:113:5`
`rust/kernel/sysctl.rs:115:9`
`rust/kernel/sysctl.rs:120:5`
`rust/kernel/sysctl.rs:124:5`
`rust/kernel/sysctl.rs:136:5`
`rust/kernel/sysctl.rs:138:5`

Added `// SAFETY` docs to `fn init_lock`
`rust/kernel/sync/mutex.rs:85:9`

Added `// SAFETY` docs to `fn init_lock`
`rust/kernel/sync/spinlock.rs:144:9`

Added `// SAFETY` docs to `fn init`
`rust/kernel/sync/condvar.rs:135:9`

Added `// SAFETY` docs to `fn read`
`rust/kernel/pages.rs:96:9`

Added `// SAFETY` docs to `fn write`
`rust/kernel/pages.rs:125:9`

Added `// SAFETY` docs to `fn rust_fmt_argument`
`rust/kernel/print.rs:17:1`
`rust/kernel/print.rs:62:5`

Added `// SAFETY` docs to `fn set_param`
`rust/kernel/module_param.rs:63:5`
`rust/kernel/module_param.rs:74:13`
`rust/kernel/module_param.rs:80:17`
`rust/kernel/module_param.rs:83:17`

Added `// SAFETY` docs to `fn get_param`
`rust/kernel/module_param.rs:104:9`
`rust/kernel/module_param.rs:108:9`

Added `// SAFETY` docs to `fn free`
`rust/kernel/module_param.rs:124:9`

Signed-off-by: Dee-Jay Logozzo <git@djl.id.au>
DjLogozzo added a commit to DjLogozzo/linux-rust that referenced this issue Nov 29, 2021
Contributes to Rust-for-Linux#351

Added `// SAFETY` docs to `fn proc_handler`
`rust/kernel/sysctl.rs:97:1`
`rust/kernel/sysctl.rs:113:5`
`rust/kernel/sysctl.rs:115:9`
`rust/kernel/sysctl.rs:120:5`
`rust/kernel/sysctl.rs:124:5`
`rust/kernel/sysctl.rs:136:5`
`rust/kernel/sysctl.rs:138:5`

Added `// SAFETY` docs to `fn init_lock`
`rust/kernel/sync/mutex.rs:85:9`

Added `// SAFETY` docs to `fn init_lock`
`rust/kernel/sync/spinlock.rs:144:9`

Added `// SAFETY` docs to `fn init`
`rust/kernel/sync/condvar.rs:135:9`

Added `// SAFETY` docs to `fn read`
`rust/kernel/pages.rs:96:9`

Added `// SAFETY` docs to `fn write`
`rust/kernel/pages.rs:125:9`

Added `// SAFETY` docs to `fn rust_fmt_argument`
`rust/kernel/print.rs:17:1`
`rust/kernel/print.rs:62:5`

Added `// SAFETY` docs to `fn set_param`
`rust/kernel/module_param.rs:63:5`
`rust/kernel/module_param.rs:74:13`
`rust/kernel/module_param.rs:80:17`
`rust/kernel/module_param.rs:83:17`

Added `// SAFETY` docs to `fn get_param`
`rust/kernel/module_param.rs:104:9`
`rust/kernel/module_param.rs:108:9`

Added `// SAFETY` docs to `fn free`
`rust/kernel/module_param.rs:124:9`

Signed-off-by: Dee-Jay Logozzo <git@djl.id.au>
@ojeda ojeda removed the prio: high label Feb 17, 2023
@ojeda ojeda changed the title rust: kernel: add missing // SAFETY comments rust: add missing // SAFETY comments Jul 19, 2024
@ojeda ojeda changed the title rust: add missing // SAFETY comments Add missing // SAFETY comments Jul 19, 2024
ojeda added a commit to ojeda/linux that referenced this issue Sep 3, 2024
Checking that we are not missing any `// SAFETY` comments in our `unsafe`
blocks is something we have wanted to do for a long time, as well as
cleaning up the remaining cases that were not documented [1].

Back when Rust for Linux started, this was something that could have
been done via a script, like Rust's `tidy`. Soon after, in Rust 1.58.0,
Clippy implemented the `undocumented_unsafe_blocks` lint [2].

Even though the lint has a few false positives, e.g. in some cases where
attributes appear between the comment and the `unsafe` block [3], there
are workarounds and the lint seems quite usable already.

Thus enable the lint now.

We still have a few cases to clean up, so just allow those for the moment
by writing a `TODO` comment -- some of those may be good candidates for
new contributors.

Link: Rust-for-Linux#351 [1]
Link: https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks [2]
Link: rust-lang/rust-clippy#13189 [3]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit to ojeda/linux that referenced this issue Sep 4, 2024
Checking that we are not missing any `// SAFETY` comments in our `unsafe`
blocks is something we have wanted to do for a long time, as well as
cleaning up the remaining cases that were not documented [1].

Back when Rust for Linux started, this was something that could have
been done via a script, like Rust's `tidy`. Soon after, in Rust 1.58.0,
Clippy implemented the `undocumented_unsafe_blocks` lint [2].

Even though the lint has a few false positives, e.g. in some cases where
attributes appear between the comment and the `unsafe` block [3], there
are workarounds and the lint seems quite usable already.

Thus enable the lint now.

We still have a few cases to clean up, so just allow those for the moment
by writing a `TODO` comment -- some of those may be good candidates for
new contributors.

Link: Rust-for-Linux#351 [1]
Link: https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks [2]
Link: rust-lang/rust-clippy#13189 [3]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@ojeda ojeda added medium Expected to be an issue of medium difficulty to resolve. easy Expected to be an easy issue to resolve. labels Oct 3, 2024
ojeda added a commit to ojeda/linux that referenced this issue Oct 3, 2024
Checking that we are not missing any `// SAFETY` comments in our `unsafe`
blocks is something we have wanted to do for a long time, as well as
cleaning up the remaining cases that were not documented [1].

Back when Rust for Linux started, this was something that could have
been done via a script, like Rust's `tidy`. Soon after, in Rust 1.58.0,
Clippy implemented the `undocumented_unsafe_blocks` lint [2].

Even though the lint has a few false positives, e.g. in some cases where
attributes appear between the comment and the `unsafe` block [3], there
are workarounds and the lint seems quite usable already.

Thus enable the lint now.

We still have a few cases to clean up, so just allow those for the moment
by writing a `TODO` comment -- some of those may be good candidates for
new contributors.

Link: Rust-for-Linux#351 [1]
Link: https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks [2]
Link: rust-lang/rust-clippy#13189 [3]
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-5-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit that referenced this issue Oct 7, 2024
Checking that we are not missing any `// SAFETY` comments in our `unsafe`
blocks is something we have wanted to do for a long time, as well as
cleaning up the remaining cases that were not documented [1].

Back when Rust for Linux started, this was something that could have
been done via a script, like Rust's `tidy`. Soon after, in Rust 1.58.0,
Clippy implemented the `undocumented_unsafe_blocks` lint [2].

Even though the lint has a few false positives, e.g. in some cases where
attributes appear between the comment and the `unsafe` block [3], there
are workarounds and the lint seems quite usable already.

Thus enable the lint now.

We still have a few cases to clean up, so just allow those for the moment
by writing a `TODO` comment -- some of those may be good candidates for
new contributors.

Link: #351 [1]
Link: https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks [2]
Link: rust-lang/rust-clippy#13189 [3]
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-5-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit that referenced this issue Oct 7, 2024
Checking that we are not missing any `// SAFETY` comments in our `unsafe`
blocks is something we have wanted to do for a long time, as well as
cleaning up the remaining cases that were not documented [1].

Back when Rust for Linux started, this was something that could have
been done via a script, like Rust's `tidy`. Soon after, in Rust 1.58.0,
Clippy implemented the `undocumented_unsafe_blocks` lint [2].

Even though the lint has a few false positives, e.g. in some cases where
attributes appear between the comment and the `unsafe` block [3], there
are workarounds and the lint seems quite usable already.

Thus enable the lint now.

We still have a few cases to clean up, so just allow those for the moment
by writing a `TODO` comment -- some of those may be good candidates for
new contributors.

Link: #351 [1]
Link: https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks [2]
Link: rust-lang/rust-clippy#13189 [3]
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-5-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy Expected to be an easy issue to resolve. good first issue Good for newcomers • lib Related to the `rust/` library. medium Expected to be an issue of medium difficulty to resolve.
Development

No branches or pull requests

3 participants