| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b129cddc-862a-472b-b52a-2457b1a02d45@gmail.com>
Date: Thu, 3 Oct 2024 10:14:17 +0200
From: Dirk Behme <dirk.behme@...il.com>
To: Boqun Feng <boqun.feng@...il.com>, Dirk Behme <dirk.behme@...bosch.com>
Cc: Andreas Hindborg <a.hindborg@...nel.org>, Miguel Ojeda
<ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Anna-Maria Behnsen <anna-maria@...utronix.de>,
Frederic Weisbecker <frederic@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>, Alice Ryhl <aliceryhl@...gle.com>,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 00/14] hrtimer Rust API
Am 01.10.24 um 16:42 schrieb Boqun Feng:
> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote:
>> On 18.09.2024 00:27, Andreas Hindborg wrote:
>>> Hi!
>>>
>>> This series adds support for using the `hrtimer` subsystem from Rust code.
>>>
>>> I tried breaking up the code in some smaller patches, hopefully that will
>>> ease the review process a bit.
>>
>> Just fyi, having all 14 patches applied I get [1] on the first (doctest)
>> Example from hrtimer.rs.
>>
>> This is from lockdep:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785
>>
>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in
>> interrupt context? Or a more subtle one?
>
> I think it's calling mutex inside an interrupt context as shown by the
> callstack:
>
> ] __mutex_lock+0xa0/0xa4
> ] ...
> ] hrtimer_interrupt+0x1d4/0x2ac
>
> , it is because:
>
> +//! struct ArcIntrusiveTimer {
> +//! #[pin]
> +//! timer: Timer<Self>,
> +//! #[pin]
> +//! flag: Mutex<bool>,
> +//! #[pin]
> +//! cond: CondVar,
> +//! }
>
> has a Mutex<bool>, which actually should be a SpinLockIrq [1].
Two understanding questions:
1. In the main thread (full example for reference below [2]) where is
the lock released? After the while loop? I.e. is the lock held until
guard reaches 5?
let mut guard = has_timer.flag.lock(); // <= lock taken here?
while *guard != 5 {
has_timer.cond.wait(&mut guard);
} // <= lock
released here?
I wonder what this would mean for the interrupt TimerCallback in case
we would use an irq-off SpinLock instead here?
Or maybe:
2. The only place where the guard is modified (*guard += 1;) is in the
TimerCallback which runs in interrupt context as we learned. With that
writing the guard value can't be interrupted. Couldn't we drop the
whole lock, then?
Best regards
Dirk
[2]
//! #[pin_data]
//! struct ArcIntrusiveTimer {
//! #[pin]
//! timer: Timer<Self>,
//! #[pin]
//! flag: Mutex<u64>,
//! #[pin]
//! cond: CondVar,
//! }
//!
//! impl ArcIntrusiveTimer {
//! fn new() -> impl PinInit<Self, kernel::error::Error> {
//! try_pin_init!(Self {
//! timer <- Timer::new(),
//! flag <- new_mutex!(0),
//! cond <- new_condvar!(),
//! })
//! }
//! }
//!
//! impl TimerCallback for ArcIntrusiveTimer {
//! type CallbackTarget<'a> = Arc<Self>;
//! type CallbackPointer<'a> = Arc<Self>;
//!
//! fn run(this: Self::CallbackTarget<'_>) -> TimerRestart {
//! pr_info!("Timer called\n");
//! let mut guard = this.flag.lock();
//! *guard += 1;
//! this.cond.notify_all();
//! if *guard == 5 {
//! TimerRestart::NoRestart
//! }
//! else {
//! TimerRestart::Restart
//!
//! }
//! }
//! }
//!
//! impl_has_timer! {
//! impl HasTimer<Self> for ArcIntrusiveTimer { self.timer }
//! }
//!
//!
//! let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?;
//! let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000));
//! let mut guard = has_timer.flag.lock();
//!
//! while *guard != 5 {
//! has_timer.cond.wait(&mut guard);
//! }
//!
//! pr_info!("Counted to 5\n");
//! # Ok::<(), kernel::error::Error>(())
> Note that
> irq-off is needed for the lock, because otherwise we will hit a self
> deadlock due to interrupts:
>
> spin_lock(&a);
> > timer interrupt
> spin_lock(&a);
>
> Also notice that the IrqDisabled<'_> token can be simply created by
> ::new(), because irq contexts should guarantee interrupt disabled (i.e.
> we don't support nested interrupts*).
>
> [*]: I vaguely remember we still have some driver code for slow devices
> that will enable interrupts during an irq handler, but these are going
> to be gone, we shouldn't really care about this in Rust code.
>
> Regards,
> Boqun
>
> [1]: https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/
>
>
>>
>> Best regards
>>
>> Dirk
>>
>> [1]
>>
>> # rust_doctest_kernel_hrtimer_rs_0.location: rust/kernel/hrtimer.rs:10
>> rust_doctests_kernel: Timer called
>>
>> =============================
>> [ BUG: Invalid wait context ]
>> 6.11.0-rc1-arm64 #28 Tainted: G N
>> -----------------------------
>> swapper/5/0 is trying to lock:
>> ffff0004409ab900 (rust/doctests_kernel_generated.rs:1238){+.+.}-{3:3}, at:
>> rust_helper_mutex_lock+0x10/0x18
>> other info that might help us debug this:
>> context-{2:2}
>> no locks held by swapper/5/0.
>> stack backtrace:
>> CPU: 5 UID: 0 PID: 0 Comm: swapper/5 Tainted: G N 6.11.0-rc1-arm64 #28
>> Tainted: [N]=TEST
>> Hardware name: ARM64 based board (DT)
>> Call trace:
>> $x.11+0x98/0xb4
>> show_stack+0x14/0x1c
>> $x.3+0x3c/0x94
>> dump_stack+0x14/0x1c
>> $x.205+0x538/0x594
>> $x.179+0xd0/0x18c
>> __mutex_lock+0xa0/0xa4
>> mutex_lock_nested+0x20/0x28
>> rust_helper_mutex_lock+0x10/0x18
>>
>> _RNvXs_NvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_0NtB4_17ArcIntrusiveTimerNtNtCsclYTRz49wqv_6kernel7hrtimer13TimerCallback3run+0x5c/0xd0
>>
>> _RNvXs1_NtNtCsclYTRz49wqv_6kernel7hrtimer3arcINtNtNtB9_4sync3arc3ArcNtNvNvNvCslTRHJHclVGW_25doctests_kernel_generated32rust_doctest_kernel_hrtimer_rs_04main41__doctest_main_rust_kernel_hrtimer_rs_10_017ArcIntrusiveTimerENtB7_16RawTimerCallback3runB1b_+0x20/0x2c
>> $x.90+0x64/0x70
>> hrtimer_interrupt+0x1d4/0x2ac
>> arch_timer_handler_phys+0x34/0x40
>> $x.62+0x50/0x54
>> generic_handle_domain_irq+0x28/0x40
>> $x.154+0x58/0x6c
>> $x.471+0x10/0x20
>> el1_interrupt+0x70/0x94
>> el1h_64_irq_handler+0x14/0x1c
>> el1h_64_irq+0x64/0x68
>> arch_local_irq_enable+0x4/0x8
>> cpuidle_enter+0x34/0x48
>> $x.37+0x58/0xe4
>> cpu_startup_entry+0x30/0x34
>> $x.2+0xf8/0x118
>> $x.13+0x0/0x4
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Counted to 5
>> ok 22 rust_doctest_kernel_hrtimer_rs_0
>> # rust_doctest_kernel_hrtimer_rs_1.location: rust/kernel/hrtimer.rs:137
>> rust_doctests_kernel: Hello from the future
>> rust_doctests_kernel: Flag raised
>> ok 23 rust_doctest_kernel_hrtimer_rs_1
>> # rust_doctest_kernel_hrtimer_rs_2.location: rust/kernel/hrtimer.rs:76
>> rust_doctests_kernel: Timer called
>> rust_doctests_kernel: Flag raised
>> ok 24 rust_doctest_kernel_hrtimer_rs_2
>
Powered by blists - more mailing lists