rust: kernel: Mark rust_fmt_argument as extern "C" #967

sulix · 2023-02-14T14:55:04Z

(As noted in #966. Let me know if you'd rather this go out on the mailing lists, rather than as a PR for rust-fixes.)

The rust_fmt_argument function is called from printk() to handle the %pA format specifier.

Since it's called from C, we should mark it extern "C" to make sure it's ABI compatible.

Fixes: 247b365 ("rust: add kernel crate")
Signed-off-by: David Gow [email protected]

bjorn3

Reviewed-by: Björn Roy Baron [email protected]

nbdd0121 · 2023-02-14T16:32:47Z

Good catch!

bjorn3 · 2023-02-14T16:45:03Z

Suggested a clippy lint: rust-lang/rust-clippy#10347

ojeda · 2023-02-14T16:46:10Z

(As noted in #966. Let me know if you'd rather this go out on the mailing lists, rather than as a PR for rust-fixes.)

Yeah, please send it as a patch too (the PR is nice though, it saves me from creating it myself later on :)

I think scripts/rustdoc_test_gen.rs also suffers from the same issue from a quick grep (not upstreamed yet, though).

Klint idea: search for #[no_mangle] + non-extern "C" cases (skipping those we know are expected like __rust_*).

ojeda · 2023-02-14T16:46:33Z

Suggested a clippy lint: rust-lang/rust-clippy#10347

Or that :)

ojeda · 2023-02-14T18:23:53Z

as a PR for rust-fixes.

Ah, wait, sorry, I didn't notice you did create it pointing to rust-fixes -- yeah, we are not using PRs for the upstream branches (it is easier to only use patches, plus avoids having to rebase + force push to sign and take tags).

nbdd0121 · 2023-02-14T20:25:44Z

Suggested a clippy lint: rust-lang/rust-clippy#10347

There are legit cases where you'd want to use #[no_mangle] between Rust crates though. Probably would have to be an allow-by-default lint.

The rust_fmt_argument function is called from printk() to handle the %pA format specifier. Since it's called from C, we should mark it extern "C" to make sure it's ABI compatible. Cc: [email protected] Fixes: 247b365 ("rust: add `kernel` crate") Signed-off-by: David Gow <[email protected]>

sulix · 2023-02-14T22:59:55Z

Sent this out here:
https://lore.kernel.org/rust-for-linux/[email protected]/T/#u

And rebased the PR on top of the rust branch.

A quick look suggests that this was the only case where we were missing the extern "C" upstream. The __rust_alloc are related functions also are no_mangle without extern "C", but that looks intentional, as far as I can tell (and so would be one of the cases where a lint would need disabling).

bjorn3 · 2023-02-14T23:25:10Z

Yeah, __rust_alloc and friends are indeed meant to be extern "Rust".

ojeda · 2023-02-15T11:03:46Z

Do you want to send a quick PR here for the other one too so that we do not forget before we upstream that? (since it is related to KUnit; otherwise I can do it too)

ojeda · 2023-02-15T19:17:19Z

Submitted at: https://lore.kernel.org/rust-for-linux/[email protected]/

By the way, it appears that the GitHub CI is not triggered by changing the branch, and thus we didn't notice rustfmt.

The rust_fmt_argument function is called from printk() to handle the %pA format specifier. Since it's called from C, we should mark it extern "C" to make sure it's ABI compatible. Cc: [email protected] Fixes: 247b365 ("rust: add `kernel` crate") Signed-off-by: David Gow <[email protected]> Link: #967 Reviewed-by: Björn Roy Baron <[email protected]> Reviewed-by: Gary Guo <[email protected]> [Applied `rustfmt`] Signed-off-by: Miguel Ojeda <[email protected]>

sulix · 2023-02-16T03:05:11Z

Do you want to send a quick PR here for the other one too so that we do not forget before we upstream that? (since it is related to KUnit; otherwise I can do it too)

Sent out #968, thanks.

The rust_fmt_argument function is called from printk() to handle the %pA format specifier. Since it's called from C, we should mark it extern "C" to make sure it's ABI compatible. Cc: [email protected] Fixes: 247b365 ("rust: add `kernel` crate") Signed-off-by: David Gow <[email protected]> Link: Rust-for-Linux#967 Reviewed-by: Björn Roy Baron <[email protected]> Reviewed-by: Gary Guo <[email protected]> [Applied `rustfmt`] Signed-off-by: Miguel Ojeda <[email protected]>

This lockdep splat says it better than I could: ================================ WARNING: inconsistent lock state 6.2.0-rc2-07010-ga9b9500ffaac-dirty Rust-for-Linux#967 Not tainted -------------------------------- inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage. kworker/1:3/179 [HC0[0]:SC0[0]:HE1:SE1] takes: ffff3ec4036ce098 (_xmit_ETHER#2){+.?.}-{3:3}, at: netif_freeze_queues+0x5c/0xc0 {IN-SOFTIRQ-W} state was registered at: _raw_spin_lock+0x5c/0xc0 sch_direct_xmit+0x148/0x37c __dev_queue_xmit+0x528/0x111c ip6_finish_output2+0x5ec/0xb7c ip6_finish_output+0x240/0x3f0 ip6_output+0x78/0x360 ndisc_send_skb+0x33c/0x85c ndisc_send_rs+0x54/0x12c addrconf_rs_timer+0x154/0x260 call_timer_fn+0xb8/0x3a0 __run_timers.part.0+0x214/0x26c run_timer_softirq+0x3c/0x74 __do_softirq+0x14c/0x5d8 ____do_softirq+0x10/0x20 call_on_irq_stack+0x2c/0x5c do_softirq_own_stack+0x1c/0x30 __irq_exit_rcu+0x168/0x1a0 irq_exit_rcu+0x10/0x40 el1_interrupt+0x38/0x64 irq event stamp: 7825 hardirqs last enabled at (7825): [<ffffdf1f7200cae4>] exit_to_kernel_mode+0x34/0x130 hardirqs last disabled at (7823): [<ffffdf1f708105f0>] __do_softirq+0x550/0x5d8 softirqs last enabled at (7824): [<ffffdf1f7081050c>] __do_softirq+0x46c/0x5d8 softirqs last disabled at (7811): [<ffffdf1f708166e0>] ____do_softirq+0x10/0x20 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(_xmit_ETHER#2); <Interrupt> lock(_xmit_ETHER#2); *** DEADLOCK *** 3 locks held by kworker/1:3/179: #0: ffff3ec400004748 ((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c0 #1: ffff80000a0bbdc8 ((work_completion)(&priv->tx_onestep_tstamp)){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c0 #2: ffff3ec4036cd438 (&dev->tx_global_lock){+.+.}-{3:3}, at: netif_tx_lock+0x1c/0x34 Workqueue: events enetc_tx_onestep_tstamp Call trace: print_usage_bug.part.0+0x208/0x22c mark_lock+0x7f0/0x8b0 __lock_acquire+0x7c4/0x1ce0 lock_acquire.part.0+0xe0/0x220 lock_acquire+0x68/0x84 _raw_spin_lock+0x5c/0xc0 netif_freeze_queues+0x5c/0xc0 netif_tx_lock+0x24/0x34 enetc_tx_onestep_tstamp+0x20/0x100 process_one_work+0x28c/0x6c0 worker_thread+0x74/0x450 kthread+0x118/0x11c but I'll say it anyway: the enetc_tx_onestep_tstamp() work item runs in process context, therefore with softirqs enabled (i.o.w., it can be interrupted by a softirq). If we hold the netif_tx_lock() when there is an interrupt, and the NET_TX softirq then gets scheduled, this will take the netif_tx_lock() a second time and deadlock the kernel. To solve this, use netif_tx_lock_bh(), which blocks softirqs from running. Fixes: 7294380 ("enetc: support PTP Sync packet one-step timestamping") Signed-off-by: Vladimir Oltean <[email protected]> Reviewed-by: Alexander Duyck <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jakub Kicinski <[email protected]>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). Thus enable it. In some cases, we need to use the Rust ABI even with `#[no_mangle]`, and for that, one may use `extern "Rust"` explicitly, but `rustfmt` overwrites it (and there is no option to prevent that, not even `force_explicit_abi` [3]). Therefore, use an `allow` instead for the few cases we had. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux#967 [2] Link: https://rust-lang.github.io/rustfmt/?version=v1.5.2&search=force_explicit_abi#force_explicit_abi [3] Signed-off-by: Miguel Ojeda <[email protected]>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In some cases, we need to use the Rust ABI even with `#[no_mangle]`, and for that, one may use `extern "Rust"` explicitly, but `rustfmt` overwrites it (and there is no option to prevent that, not even `force_explicit_abi` [3]). Therefore, use an `allow` instead for the few cases we had. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux#967 [2] Link: https://rust-lang.github.io/rustfmt/?version=v1.5.2&search=force_explicit_abi#force_explicit_abi [3] Signed-off-by: Miguel Ojeda <[email protected]>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In some cases, we need to use the Rust ABI even with `#[no_mangle]`, and for that, one may use `extern "Rust"` explicitly, but `rustfmt` overwrites it (and there does not seem to be an option to prevent that: `force_explicit_abi` does not allow to control that part, and even if it did, or if there was another option, we may not use it, since so far we have been using the defaults). Therefore, use `allow`s instead for the few cases we had. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux#967 [2] Link: https://rust-lang.github.io/rustfmt/?version=v1.5.2&search=force_explicit_abi#force_explicit_abi [3] Signed-off-by: Miguel Ojeda <[email protected]>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <[email protected]>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: #967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <[email protected]> Reviewed-by: Martin Rodriguez Reboredo <[email protected]> Link: https://lore.kernel.org/r/[email protected]

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: #967 [2] Link: rust-lang/rustfmt#5701 [3] Reviewed-by: Trevor Gross <[email protected]> Reviewed-by: Gary Guo <[email protected]> Reviewed-by: Martin Rodriguez Reboredo <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux/linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <[email protected]>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Reviewed-by: Trevor Gross <[email protected]> Reviewed-by: Gary Guo <[email protected]> Reviewed-by: Martin Rodriguez Reboredo <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux/linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <[email protected]>

bjorn3 approved these changes Feb 14, 2023

View reviewed changes

bjorn3 mentioned this pull request Feb 14, 2023

Lint against #[no_mangle] with the rust ABI rust-lang/rust-clippy#10347

Closed

ojeda mentioned this pull request Feb 14, 2023

Clippy wanted features & bugfixes #349

Open

33 tasks

sulix force-pushed the printk-extern-c branch from d686c92 to 175a53c Compare February 14, 2023 22:56

sulix changed the base branch from rust-fixes to rust February 14, 2023 22:56

sulix mentioned this pull request Feb 16, 2023

rust: doctests: Mark generated rustdoc kunit tests extern "C" #968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust: kernel: Mark rust_fmt_argument as extern "C" #967

rust: kernel: Mark rust_fmt_argument as extern "C" #967

sulix commented Feb 14, 2023

bjorn3 left a comment

nbdd0121 commented Feb 14, 2023

bjorn3 commented Feb 14, 2023

ojeda commented Feb 14, 2023

ojeda commented Feb 14, 2023

ojeda commented Feb 14, 2023

nbdd0121 commented Feb 14, 2023

sulix commented Feb 14, 2023

bjorn3 commented Feb 14, 2023

ojeda commented Feb 15, 2023

ojeda commented Feb 15, 2023

sulix commented Feb 16, 2023

rust: kernel: Mark rust_fmt_argument as extern "C" #967

rust: kernel: Mark rust_fmt_argument as extern "C" #967

Conversation

sulix commented Feb 14, 2023

bjorn3 left a comment

Choose a reason for hiding this comment

nbdd0121 commented Feb 14, 2023

bjorn3 commented Feb 14, 2023

ojeda commented Feb 14, 2023

ojeda commented Feb 14, 2023

ojeda commented Feb 14, 2023

nbdd0121 commented Feb 14, 2023

sulix commented Feb 14, 2023

bjorn3 commented Feb 14, 2023

ojeda commented Feb 15, 2023

ojeda commented Feb 15, 2023

sulix commented Feb 16, 2023