Clean `unsafe` in `unsafe fn`s warnings #348

ojeda · 2021-06-04T17:33:34Z

Requesting a review from our in-house Rust experts since even if this is mechanical, I am not 100% sure I know about all the cases where introducing a block may change behavior (e.g. creating temporaries etc.).

Note that this does not add // SAFETY comments -- that cleanup will come later when we add a tidy script, but this at least cleans up the warnings to avoid missing other warnings in the sea and allows us to deny the warning so that we stop getting new unsafe blocks inside unsafe fns without a proper // SAFETY comment.

Fixes #340.
Fixes #341.

Signed-off-by: Miguel Ojeda <[email protected]>

This prevents new ones from getting in. Signed-off-by: Miguel Ojeda <[email protected]>

ksquirrel · 2021-06-04T17:33:45Z

Review of a0ea20f07a50:

⚠️ This PR changes more than 20 files.
✔️ Commit c606d85: Looks fine!
✔️ Commit f81ade8: Looks fine!
✔️ Commit a0ea20f: Looks fine!

nbdd0121

LGTM, as long as CI passes.

Does the CI cover all Rust files?

ojeda · 2021-06-04T17:45:31Z

LGTM, as long as CI passes.

Well, that was super quick! Please do not rush it! :)

Does the CI cover all Rust files?

Good question, if they are not, well, soon people will notice since it is deny :)

ojeda · 2021-06-04T17:47:59Z

By the way, to simplify, I have not split some of the unsafes into several lines etc. (which should be done).

I will open an issue and explain what should be done to do the tidy cleanup (basically removing the unsafe, checking what the compiler says, then adding as needed).

TheSven73 · 2021-06-04T17:52:40Z

Do we need to switch on some sort of "warnings=errors" in the build? As part of this PR, or a new one?

ojeda · 2021-06-04T17:55:36Z

Do you mean making the warning now an error? It is done in the third commit.

TheSven73 · 2021-06-04T17:56:45Z

Should we go even further, and treat all warnings as errors?

ojeda · 2021-06-04T17:59:04Z

For rust/ code, 100% with being super strict.

For the rest of modules etc., I would also do it -- if we deny instead of forbid, they can still workaround it if needed.

bjorn3

There are no behavior changes I think.

bjorn3 · 2021-06-04T18:34:09Z

rust/kernel/miscdev.rs

@@ -85,8 +85,11 @@ impl<T: Sync> FileOpenAdapter for Registration<T> {
    type Arg = T;

    unsafe fn convert(_inode: *mut bindings::inode, file: *mut bindings::file) -> *const Self::Arg {
+        // TODO: `SAFETY` comment required here even if `unsafe` is not present,
+        // because `container_of!` hides it. Ideally we would not allow
+        // `unsafe` code as parameters to macros.
        let reg = crate::container_of!((*file).private_data, Self, mdev);


The macro could introduce a temp outside of the unsafe block that stores the value casted to a raw ptr. Ideally no $foo:expr happen inside an unsafe block for macros.

+1, opening an issue.

I guess this could also be a lint with some logic like:

If inside an unsafe fn...

And there is a macro call outside an unsafe block...

And unsafe_op_in_unsafe_fn is enabled in this context...

And if the node/AST/tree of a parameter appears as-is...

Then check that node/AST/tree is not inside an introduced unsafe block.

Easier said than done, and I guess I am forgetting a few things, but... :^)

If you think it makes sense, I can open an issue too.

Maybe. I believe there has been discussion previously regarding "unsafe" hygiene, but I can't remember what came of it. I think it was on internals.rust-lang.org.

I guess it does not hurt to open it to keep track.

rust-lang/rust-clippy#7323

bjorn3 · 2021-06-04T18:35:46Z

rust/kernel/module_param.rs

@@ -111,7 +111,7 @@ pub trait ModuleParam: core::fmt::Display + core::marker::Sized {
    ///
    /// The `arg` field of `param` must be an instance of `Self`.
    unsafe extern "C" fn free(arg: *mut crate::c_types::c_void) {
-        core::ptr::drop_in_place(arg as *mut Self);
+        unsafe { core::ptr::drop_in_place(arg as *mut Self) };


Here and elsewhere you may want to move the ; inside the unsafe block. While this doesn't change behavior as all cases are for () returning expressions I think, it does look nicer IMHO.

Yeah, now that you mention it, I agree.

Does anyone know if there a way to tell rustfmt to look for this? Or in Clippy perhaps?

I can't find a clippy lint for this. Makes sense to have as a clippy lint though IMO.

+1 Opened rust-lang/rust-clippy#7322.

ojeda · 2021-06-04T19:12:31Z

Thanks a lot @bjorn3 and @nbdd0121!

Let me merge as is, since you both reviewed it -- we can change style later (e.g. in the tidy cleanup round).

nbdd0121 · 2021-06-04T20:42:21Z

Should we go even further, and treat all warnings as errors?

#![deny(warnings)] is generally considered as an anti-pattern, see here. We could use RUSTFLAGS="-D warnings" (we currently call it RUSTCFLAGS) in the CI, but it shouldn't be in the Makefile.

TheSven73 · 2021-06-04T21:03:08Z

(edited out a wordy reply that basically just agrees with Gary, lol)

So yes, I would be on board with switching on "deny warnings" in the CI only, not in the Makefile.

ojeda · 2021-06-04T21:50:00Z

I was referring more to deny individually as many warnings as possible, like this one, i.e. not every possible future warning. So agreed!

Add a test case to assert that the skb->pkt_type which was set from the BPF program is retained from the netkit xmit side to the peer's device at tcx ingress location. # ./vmtest.sh -- ./test_progs -t netkit [...] ./test_progs -t netkit [ 1.140780] bpf_testmod: loading out-of-tree module taints kernel. [ 1.141127] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel [ 1.284601] tsc: Refined TSC clocksource calibration: 3408.006 MHz [ 1.286672] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x311fd9b189d, max_idle_ns: 440795225691 ns [ 1.290384] clocksource: Switched to clocksource tsc #345 tc_netkit_basic:OK #346 tc_netkit_device:OK #347 tc_netkit_multi_links:OK #348 tc_netkit_multi_opts:OK #349 tc_netkit_neigh_links:OK #350 tc_netkit_pkt_type:OK Summary: 6/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Alexei Starovoitov <[email protected]>

ojeda added 3 commits June 4, 2021 19:23

rust: kernel: clean unsafe_op_in_unsafe_fn warnings

c606d85

Signed-off-by: Miguel Ojeda <[email protected]>

drivers: binder: clean unsafe_op_in_unsafe_fn warning

f81ade8

Signed-off-by: Miguel Ojeda <[email protected]>

rust: globally deny unsafe_op_in_unsafe_fn

a0ea20f

This prevents new ones from getting in. Signed-off-by: Miguel Ojeda <[email protected]>

ojeda requested review from alex, nbdd0121 and bjorn3 June 4, 2021 17:33

nbdd0121 approved these changes Jun 4, 2021

View reviewed changes

bjorn3 approved these changes Jun 4, 2021

View reviewed changes

ojeda merged commit cae4454 into Rust-for-Linux:rust Jun 4, 2021

ojeda deleted the warnings branch June 4, 2021 19:12

This was referenced Jun 4, 2021

Fix warnings in str.rs and binding_generated.rs #343

Merged

Add missing // SAFETY comments #351

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean `unsafe` in `unsafe fn`s warnings #348

Clean `unsafe` in `unsafe fn`s warnings #348

ojeda commented Jun 4, 2021 •

edited

Loading

ksquirrel commented Jun 4, 2021

nbdd0121 left a comment

ojeda commented Jun 4, 2021

ojeda commented Jun 4, 2021

TheSven73 commented Jun 4, 2021

ojeda commented Jun 4, 2021

TheSven73 commented Jun 4, 2021 •

edited

Loading

ojeda commented Jun 4, 2021

bjorn3 left a comment

bjorn3 Jun 4, 2021

ojeda Jun 4, 2021

ojeda Jun 4, 2021 •

edited

Loading

bjorn3 Jun 4, 2021

ojeda Jun 4, 2021

bjorn3 Jun 4, 2021

ojeda Jun 4, 2021

bjorn3 Jun 4, 2021

ojeda Jun 4, 2021

bjorn3 Jun 4, 2021

ojeda Jun 4, 2021

ojeda commented Jun 4, 2021

nbdd0121 commented Jun 4, 2021

TheSven73 commented Jun 4, 2021 •

edited

Loading

ojeda commented Jun 4, 2021

Clean unsafe in unsafe fns warnings #348

Clean unsafe in unsafe fns warnings #348

Conversation

ojeda commented Jun 4, 2021 • edited Loading

ksquirrel commented Jun 4, 2021

nbdd0121 left a comment

Choose a reason for hiding this comment

ojeda commented Jun 4, 2021

ojeda commented Jun 4, 2021

TheSven73 commented Jun 4, 2021

ojeda commented Jun 4, 2021

TheSven73 commented Jun 4, 2021 • edited Loading

ojeda commented Jun 4, 2021

bjorn3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda commented Jun 4, 2021

nbdd0121 commented Jun 4, 2021

TheSven73 commented Jun 4, 2021 • edited Loading

ojeda commented Jun 4, 2021

Clean `unsafe` in `unsafe fn`s warnings #348

Clean `unsafe` in `unsafe fn`s warnings #348

ojeda commented Jun 4, 2021 •

edited

Loading

TheSven73 commented Jun 4, 2021 •

edited

Loading

ojeda Jun 4, 2021 •

edited

Loading

TheSven73 commented Jun 4, 2021 •

edited

Loading