Using `READ_ONCE` is the correct way to read the `node.expires` field.
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/time/hrtimer.rs | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
// - Timers cannot have negative ktime_t values as their expiration time.
// - There's no actual locking here, a racy read is fine and expected
unsafe {
- Instant::from_ktime(
- // This `read_volatile` is intended to correspond to a READ_ONCE call.
- // FIXME(read_once): Replace with `read_once` when available on the Rust side.
- core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
- )
+ Instant::from_ktime(kernel::sync::READ_ONCE(
+ &raw const (*c_timer_ptr).node.expires,
+ ))
}
}
}
--
2.52.0.351.gbe84eed79e-googOn Wed, 31 Dec 2025 12:22:28 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> Using `READ_ONCE` is the correct way to read the `node.expires` field.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/time/hrtimer.rs | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
> // - Timers cannot have negative ktime_t values as their expiration time.
> // - There's no actual locking here, a racy read is fine and expected
> unsafe {
> - Instant::from_ktime(
> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
> - )
> + Instant::from_ktime(kernel::sync::READ_ONCE(
> + &raw const (*c_timer_ptr).node.expires,
> + ))
> }
Do we actually need READ_ONCE() here? I'm not sure but would it be
better to call the C-side API?
diff --git a/rust/helpers/time.c b/rust/helpers/time.c
index 67a36ccc3ec4..73162dea2a29 100644
--- a/rust/helpers/time.c
+++ b/rust/helpers/time.c
@@ -2,6 +2,7 @@
#include <linux/delay.h>
#include <linux/ktime.h>
+#include <linux/hrtimer.h>
#include <linux/timekeeping.h>
void rust_helper_fsleep(unsigned long usecs)
@@ -38,3 +39,8 @@ void rust_helper_udelay(unsigned long usec)
{
udelay(usec);
}
+
+__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
+{
+ return timer->node.expires;
+}
diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 856d2d929a00..61e656a65216 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -237,14 +237,7 @@ pub fn expires(&self) -> HrTimerInstant<T>
// SAFETY:
// - Timers cannot have negative ktime_t values as their expiration time.
- // - There's no actual locking here, a racy read is fine and expected
- unsafe {
- Instant::from_ktime(
- // This `read_volatile` is intended to correspond to a READ_ONCE call.
- // FIXME(read_once): Replace with `read_once` when available on the Rust side.
- core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
- )
- }
+ unsafe { Instant::from_ktime(bindings::hrtimer_get_expires(c_timer_ptr)) }
}
}
On Thu, 01 Jan 2026 11:11:23 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> On Wed, 31 Dec 2025 12:22:28 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>> Using `READ_ONCE` is the correct way to read the `node.expires` field.
>>
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> ---
>> rust/kernel/time/hrtimer.rs | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
>> // - Timers cannot have negative ktime_t values as their expiration time.
>> // - There's no actual locking here, a racy read is fine and expected
>> unsafe {
>> - Instant::from_ktime(
>> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
>> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
>> - )
>> + Instant::from_ktime(kernel::sync::READ_ONCE(
>> + &raw const (*c_timer_ptr).node.expires,
>> + ))
>> }
>
> Do we actually need READ_ONCE() here? I'm not sure but would it be
> better to call the C-side API?
>
> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
> index 67a36ccc3ec4..73162dea2a29 100644
> --- a/rust/helpers/time.c
> +++ b/rust/helpers/time.c
> @@ -2,6 +2,7 @@
>
> #include <linux/delay.h>
> #include <linux/ktime.h>
> +#include <linux/hrtimer.h>
> #include <linux/timekeeping.h>
>
> void rust_helper_fsleep(unsigned long usecs)
> @@ -38,3 +39,8 @@ void rust_helper_udelay(unsigned long usec)
> {
> udelay(usec);
> }
> +
> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
> +{
> + return timer->node.expires;
> +}
Sorry, of course this should be:
+__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
+{
+ return hrtimer_get_expires(timer);
+}
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index 856d2d929a00..61e656a65216 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -237,14 +237,7 @@ pub fn expires(&self) -> HrTimerInstant<T>
>
> // SAFETY:
> // - Timers cannot have negative ktime_t values as their expiration time.
> - // - There's no actual locking here, a racy read is fine and expected
> - unsafe {
> - Instant::from_ktime(
> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
> - )
> - }
> + unsafe { Instant::from_ktime(bindings::hrtimer_get_expires(c_timer_ptr)) }
> }
> }
>
>
"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> On Thu, 01 Jan 2026 11:11:23 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
>> On Wed, 31 Dec 2025 12:22:28 +0000
>> Alice Ryhl <aliceryhl@google.com> wrote:
>>
>>> Using `READ_ONCE` is the correct way to read the `node.expires` field.
>>>
>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>> ---
>>> rust/kernel/time/hrtimer.rs | 8 +++-----
>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>> index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
>>> --- a/rust/kernel/time/hrtimer.rs
>>> +++ b/rust/kernel/time/hrtimer.rs
>>> @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
>>> // - Timers cannot have negative ktime_t values as their expiration time.
>>> // - There's no actual locking here, a racy read is fine and expected
>>> unsafe {
>>> - Instant::from_ktime(
>>> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
>>> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>>> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
>>> - )
>>> + Instant::from_ktime(kernel::sync::READ_ONCE(
>>> + &raw const (*c_timer_ptr).node.expires,
>>> + ))
>>> }
>>
>> Do we actually need READ_ONCE() here? I'm not sure but would it be
>> better to call the C-side API?
>>
>> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
>> index 67a36ccc3ec4..73162dea2a29 100644
>> --- a/rust/helpers/time.c
>> +++ b/rust/helpers/time.c
>> @@ -2,6 +2,7 @@
>>
>> #include <linux/delay.h>
>> #include <linux/ktime.h>
>> +#include <linux/hrtimer.h>
>> #include <linux/timekeeping.h>
>>
>> void rust_helper_fsleep(unsigned long usecs)
>> @@ -38,3 +39,8 @@ void rust_helper_udelay(unsigned long usec)
>> {
>> udelay(usec);
>> }
>> +
>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>> +{
>> + return timer->node.expires;
>> +}
>
> Sorry, of course this should be:
>
> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
> +{
> + return hrtimer_get_expires(timer);
> +}
>
This is a potentially racy read. As far as I recall, we determined that
using read_once is the proper way to handle the situation.
I do not think it makes a difference that the read is done by C code.
Best regards,
Andreas Hindborg
On Tue, 06 Jan 2026 13:37:34 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:
> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>
>> On Thu, 01 Jan 2026 11:11:23 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>
>>> On Wed, 31 Dec 2025 12:22:28 +0000
>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>
>>>> Using `READ_ONCE` is the correct way to read the `node.expires` field.
>>>>
>>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>>> ---
>>>> rust/kernel/time/hrtimer.rs | 8 +++-----
>>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>>> index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
>>>> --- a/rust/kernel/time/hrtimer.rs
>>>> +++ b/rust/kernel/time/hrtimer.rs
>>>> @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
>>>> // - Timers cannot have negative ktime_t values as their expiration time.
>>>> // - There's no actual locking here, a racy read is fine and expected
>>>> unsafe {
>>>> - Instant::from_ktime(
>>>> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
>>>> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>>>> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
>>>> - )
>>>> + Instant::from_ktime(kernel::sync::READ_ONCE(
>>>> + &raw const (*c_timer_ptr).node.expires,
>>>> + ))
>>>> }
>>>
>>> Do we actually need READ_ONCE() here? I'm not sure but would it be
>>> better to call the C-side API?
>>>
>>> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
>>> index 67a36ccc3ec4..73162dea2a29 100644
>>> --- a/rust/helpers/time.c
>>> +++ b/rust/helpers/time.c
>>> @@ -2,6 +2,7 @@
>>>
>>> #include <linux/delay.h>
>>> #include <linux/ktime.h>
>>> +#include <linux/hrtimer.h>
>>> #include <linux/timekeeping.h>
>>>
>>> void rust_helper_fsleep(unsigned long usecs)
>>> @@ -38,3 +39,8 @@ void rust_helper_udelay(unsigned long usec)
>>> {
>>> udelay(usec);
>>> }
>>> +
>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>> +{
>>> + return timer->node.expires;
>>> +}
>>
>> Sorry, of course this should be:
>>
>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>> +{
>> + return hrtimer_get_expires(timer);
>> +}
>>
>
> This is a potentially racy read. As far as I recall, we determined that
> using read_once is the proper way to handle the situation.
>
> I do not think it makes a difference that the read is done by C code.
What does "racy read" mean here?
The C side doesn't use WRITE_ONCE() or READ_ONCE for node.expires. How
would using READ_ONCE() on the Rust side make a difference?
FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> On Tue, 06 Jan 2026 13:37:34 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>
>>> On Thu, 01 Jan 2026 11:11:23 +0900 (JST)
>>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>>
>>>> On Wed, 31 Dec 2025 12:22:28 +0000
>>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>>
>>>>> Using `READ_ONCE` is the correct way to read the `node.expires` field.
>>>>>
>>>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>>>> ---
>>>>> rust/kernel/time/hrtimer.rs | 8 +++-----
>>>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>>>> index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
>>>>> --- a/rust/kernel/time/hrtimer.rs
>>>>> +++ b/rust/kernel/time/hrtimer.rs
>>>>> @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
>>>>> // - Timers cannot have negative ktime_t values as their expiration time.
>>>>> // - There's no actual locking here, a racy read is fine and expected
>>>>> unsafe {
>>>>> - Instant::from_ktime(
>>>>> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
>>>>> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>>>>> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
>>>>> - )
>>>>> + Instant::from_ktime(kernel::sync::READ_ONCE(
>>>>> + &raw const (*c_timer_ptr).node.expires,
>>>>> + ))
>>>>> }
>>>>
>>>> Do we actually need READ_ONCE() here? I'm not sure but would it be
>>>> better to call the C-side API?
>>>>
>>>> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
>>>> index 67a36ccc3ec4..73162dea2a29 100644
>>>> --- a/rust/helpers/time.c
>>>> +++ b/rust/helpers/time.c
>>>> @@ -2,6 +2,7 @@
>>>>
>>>> #include <linux/delay.h>
>>>> #include <linux/ktime.h>
>>>> +#include <linux/hrtimer.h>
>>>> #include <linux/timekeeping.h>
>>>>
>>>> void rust_helper_fsleep(unsigned long usecs)
>>>> @@ -38,3 +39,8 @@ void rust_helper_udelay(unsigned long usec)
>>>> {
>>>> udelay(usec);
>>>> }
>>>> +
>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>> +{
>>>> + return timer->node.expires;
>>>> +}
>>>
>>> Sorry, of course this should be:
>>>
>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>> +{
>>> + return hrtimer_get_expires(timer);
>>> +}
>>>
>>
>> This is a potentially racy read. As far as I recall, we determined that
>> using read_once is the proper way to handle the situation.
>>
>> I do not think it makes a difference that the read is done by C code.
>
> What does "racy read" mean here?
>
> The C side doesn't use WRITE_ONCE() or READ_ONCE for node.expires. How
> would using READ_ONCE() on the Rust side make a difference?
Data races like this are UB in Rust. As far as I understand, using this
READ_ONCE implementation or a relaxed atomic read would make the read
well defined. I am not aware if this is only the case if all writes to
the location from C also use atomic operations or WRITE_ONCE. @Boqun?
Best regards,
Andreas Hindborg
On Wed, 07 Jan 2026 11:11:43 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:
> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>
>> On Tue, 06 Jan 2026 13:37:34 +0100
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>
>>>> On Thu, 01 Jan 2026 11:11:23 +0900 (JST)
>>>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>>>
>>>>> On Wed, 31 Dec 2025 12:22:28 +0000
>>>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>>>
>>>>>> Using `READ_ONCE` is the correct way to read the `node.expires` field.
>>>>>>
>>>>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>>>>> ---
>>>>>> rust/kernel/time/hrtimer.rs | 8 +++-----
>>>>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>>>>> index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
>>>>>> --- a/rust/kernel/time/hrtimer.rs
>>>>>> +++ b/rust/kernel/time/hrtimer.rs
>>>>>> @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
>>>>>> // - Timers cannot have negative ktime_t values as their expiration time.
>>>>>> // - There's no actual locking here, a racy read is fine and expected
>>>>>> unsafe {
>>>>>> - Instant::from_ktime(
>>>>>> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
>>>>>> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>>>>>> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
>>>>>> - )
>>>>>> + Instant::from_ktime(kernel::sync::READ_ONCE(
>>>>>> + &raw const (*c_timer_ptr).node.expires,
>>>>>> + ))
>>>>>> }
>>>>>
>>>>> Do we actually need READ_ONCE() here? I'm not sure but would it be
>>>>> better to call the C-side API?
>>>>>
>>>>> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
>>>>> index 67a36ccc3ec4..73162dea2a29 100644
>>>>> --- a/rust/helpers/time.c
>>>>> +++ b/rust/helpers/time.c
>>>>> @@ -2,6 +2,7 @@
>>>>>
>>>>> #include <linux/delay.h>
>>>>> #include <linux/ktime.h>
>>>>> +#include <linux/hrtimer.h>
>>>>> #include <linux/timekeeping.h>
>>>>>
>>>>> void rust_helper_fsleep(unsigned long usecs)
>>>>> @@ -38,3 +39,8 @@ void rust_helper_udelay(unsigned long usec)
>>>>> {
>>>>> udelay(usec);
>>>>> }
>>>>> +
>>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>>> +{
>>>>> + return timer->node.expires;
>>>>> +}
>>>>
>>>> Sorry, of course this should be:
>>>>
>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>> +{
>>>> + return hrtimer_get_expires(timer);
>>>> +}
>>>>
>>>
>>> This is a potentially racy read. As far as I recall, we determined that
>>> using read_once is the proper way to handle the situation.
>>>
>>> I do not think it makes a difference that the read is done by C code.
>>
>> What does "racy read" mean here?
>>
>> The C side doesn't use WRITE_ONCE() or READ_ONCE for node.expires. How
>> would using READ_ONCE() on the Rust side make a difference?
>
> Data races like this are UB in Rust. As far as I understand, using this
> READ_ONCE implementation or a relaxed atomic read would make the read
> well defined. I am not aware if this is only the case if all writes to
> the location from C also use atomic operations or WRITE_ONCE. @Boqun?
The C side updates node.expires without WRITE_ONCE()/atomics so a
Rust-side READ_ONCE() can still observe a torn value; I think that
this is still a data race / UB from Rust's perspective.
And since expires is 64-bit, WRITE_ONCE() on 32-bit architectures does
not inherently guarantee tear-free stores either.
I think that the expires() method should follow the same safety
requirements as raw_forward(): it should only be considered safe when
holding exclusive access to hrtimer or within the context of the timer
callback. Under those conditions, it would be fine to call C's
hrtimer_get_expires().
"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> On Wed, 07 Jan 2026 11:11:43 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>>
>>> On Tue, 06 Jan 2026 13:37:34 +0100
>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>
>>>>> On Thu, 01 Jan 2026 11:11:23 +0900 (JST)
>>>>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>>>>
>>>>>> On Wed, 31 Dec 2025 12:22:28 +0000
>>>>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>>>>
>>>>>>> Using `READ_ONCE` is the correct way to read the `node.expires` field.
>>>>>>>
>>>>>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>>>>>> ---
>>>>>>> rust/kernel/time/hrtimer.rs | 8 +++-----
>>>>>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>>>>>> index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
>>>>>>> --- a/rust/kernel/time/hrtimer.rs
>>>>>>> +++ b/rust/kernel/time/hrtimer.rs
>>>>>>> @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
>>>>>>> // - Timers cannot have negative ktime_t values as their expiration time.
>>>>>>> // - There's no actual locking here, a racy read is fine and expected
>>>>>>> unsafe {
>>>>>>> - Instant::from_ktime(
>>>>>>> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
>>>>>>> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>>>>>>> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
>>>>>>> - )
>>>>>>> + Instant::from_ktime(kernel::sync::READ_ONCE(
>>>>>>> + &raw const (*c_timer_ptr).node.expires,
>>>>>>> + ))
>>>>>>> }
>>>>>>
>>>>>> Do we actually need READ_ONCE() here? I'm not sure but would it be
>>>>>> better to call the C-side API?
>>>>>>
>>>>>> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
>>>>>> index 67a36ccc3ec4..73162dea2a29 100644
>>>>>> --- a/rust/helpers/time.c
>>>>>> +++ b/rust/helpers/time.c
>>>>>> @@ -2,6 +2,7 @@
>>>>>>
>>>>>> #include <linux/delay.h>
>>>>>> #include <linux/ktime.h>
>>>>>> +#include <linux/hrtimer.h>
>>>>>> #include <linux/timekeeping.h>
>>>>>>
>>>>>> void rust_helper_fsleep(unsigned long usecs)
>>>>>> @@ -38,3 +39,8 @@ void rust_helper_udelay(unsigned long usec)
>>>>>> {
>>>>>> udelay(usec);
>>>>>> }
>>>>>> +
>>>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>>>> +{
>>>>>> + return timer->node.expires;
>>>>>> +}
>>>>>
>>>>> Sorry, of course this should be:
>>>>>
>>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>>> +{
>>>>> + return hrtimer_get_expires(timer);
>>>>> +}
>>>>>
>>>>
>>>> This is a potentially racy read. As far as I recall, we determined that
>>>> using read_once is the proper way to handle the situation.
>>>>
>>>> I do not think it makes a difference that the read is done by C code.
>>>
>>> What does "racy read" mean here?
>>>
>>> The C side doesn't use WRITE_ONCE() or READ_ONCE for node.expires. How
>>> would using READ_ONCE() on the Rust side make a difference?
>>
>> Data races like this are UB in Rust. As far as I understand, using this
>> READ_ONCE implementation or a relaxed atomic read would make the read
>> well defined. I am not aware if this is only the case if all writes to
>> the location from C also use atomic operations or WRITE_ONCE. @Boqun?
>
> The C side updates node.expires without WRITE_ONCE()/atomics so a
> Rust-side READ_ONCE() can still observe a torn value; I think that
> this is still a data race / UB from Rust's perspective.
>
> And since expires is 64-bit, WRITE_ONCE() on 32-bit architectures does
> not inherently guarantee tear-free stores either.
>
> I think that the expires() method should follow the same safety
> requirements as raw_forward(): it should only be considered safe when
> holding exclusive access to hrtimer or within the context of the timer
> callback. Under those conditions, it would be fine to call C's
> hrtimer_get_expires().
We can make it safe, please see my comment here [1].
Best regards,
Andreas Hindborg
[1] https://lore.kernel.org/r/87v7hdh9m4.fsf@t14s.mail-host-address-is-not-set
On Wed, 07 Jan 2026 19:21:11 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:
> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>
>> On Wed, 07 Jan 2026 11:11:43 +0100
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>>>
>>>> On Tue, 06 Jan 2026 13:37:34 +0100
>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>
>>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>>
>>>>>> On Thu, 01 Jan 2026 11:11:23 +0900 (JST)
>>>>>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>>>>>
>>>>>>> On Wed, 31 Dec 2025 12:22:28 +0000
>>>>>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>>>>>
>>>>>>>> Using `READ_ONCE` is the correct way to read the `node.expires` field.
>>>>>>>>
>>>>>>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>>>>>>> ---
>>>>>>>> rust/kernel/time/hrtimer.rs | 8 +++-----
>>>>>>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>>>>>>> index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
>>>>>>>> --- a/rust/kernel/time/hrtimer.rs
>>>>>>>> +++ b/rust/kernel/time/hrtimer.rs
>>>>>>>> @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
>>>>>>>> // - Timers cannot have negative ktime_t values as their expiration time.
>>>>>>>> // - There's no actual locking here, a racy read is fine and expected
>>>>>>>> unsafe {
>>>>>>>> - Instant::from_ktime(
>>>>>>>> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
>>>>>>>> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>>>>>>>> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
>>>>>>>> - )
>>>>>>>> + Instant::from_ktime(kernel::sync::READ_ONCE(
>>>>>>>> + &raw const (*c_timer_ptr).node.expires,
>>>>>>>> + ))
>>>>>>>> }
>>>>>>>
>>>>>>> Do we actually need READ_ONCE() here? I'm not sure but would it be
>>>>>>> better to call the C-side API?
>>>>>>>
>>>>>>> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
>>>>>>> index 67a36ccc3ec4..73162dea2a29 100644
>>>>>>> --- a/rust/helpers/time.c
>>>>>>> +++ b/rust/helpers/time.c
>>>>>>> @@ -2,6 +2,7 @@
>>>>>>>
>>>>>>> #include <linux/delay.h>
>>>>>>> #include <linux/ktime.h>
>>>>>>> +#include <linux/hrtimer.h>
>>>>>>> #include <linux/timekeeping.h>
>>>>>>>
>>>>>>> void rust_helper_fsleep(unsigned long usecs)
>>>>>>> @@ -38,3 +39,8 @@ void rust_helper_udelay(unsigned long usec)
>>>>>>> {
>>>>>>> udelay(usec);
>>>>>>> }
>>>>>>> +
>>>>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>>>>> +{
>>>>>>> + return timer->node.expires;
>>>>>>> +}
>>>>>>
>>>>>> Sorry, of course this should be:
>>>>>>
>>>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>>>> +{
>>>>>> + return hrtimer_get_expires(timer);
>>>>>> +}
>>>>>>
>>>>>
>>>>> This is a potentially racy read. As far as I recall, we determined that
>>>>> using read_once is the proper way to handle the situation.
>>>>>
>>>>> I do not think it makes a difference that the read is done by C code.
>>>>
>>>> What does "racy read" mean here?
>>>>
>>>> The C side doesn't use WRITE_ONCE() or READ_ONCE for node.expires. How
>>>> would using READ_ONCE() on the Rust side make a difference?
>>>
>>> Data races like this are UB in Rust. As far as I understand, using this
>>> READ_ONCE implementation or a relaxed atomic read would make the read
>>> well defined. I am not aware if this is only the case if all writes to
>>> the location from C also use atomic operations or WRITE_ONCE. @Boqun?
>>
>> The C side updates node.expires without WRITE_ONCE()/atomics so a
>> Rust-side READ_ONCE() can still observe a torn value; I think that
>> this is still a data race / UB from Rust's perspective.
>>
>> And since expires is 64-bit, WRITE_ONCE() on 32-bit architectures does
>> not inherently guarantee tear-free stores either.
>>
>> I think that the expires() method should follow the same safety
>> requirements as raw_forward(): it should only be considered safe when
>> holding exclusive access to hrtimer or within the context of the timer
>> callback. Under those conditions, it would be fine to call C's
>> hrtimer_get_expires().
>
> We can make it safe, please see my comment here [1].
>
> Best regards,
> Andreas Hindborg
>
> [1] https://lore.kernel.org/r/87v7hdh9m4.fsf@t14s.mail-host-address-is-not-set
I agree. My point was that expire() can be safe only under the same
constraints as forward()/forward_now() so the API should require
Pin<&mut Self> and expose it on HrTimerCallbackContext.
"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> On Wed, 07 Jan 2026 19:21:11 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>
>>> On Wed, 07 Jan 2026 11:11:43 +0100
>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>>> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>>>>
>>>>> On Tue, 06 Jan 2026 13:37:34 +0100
>>>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>
>>>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>>>
>>>>>>> On Thu, 01 Jan 2026 11:11:23 +0900 (JST)
>>>>>>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>>>>>>
>>>>>>>> On Wed, 31 Dec 2025 12:22:28 +0000
>>>>>>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>>>>>>
>>>>>>>>> Using `READ_ONCE` is the correct way to read the `node.expires` field.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>>>>>>>> ---
>>>>>>>>> rust/kernel/time/hrtimer.rs | 8 +++-----
>>>>>>>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>>>>>>>> index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644
>>>>>>>>> --- a/rust/kernel/time/hrtimer.rs
>>>>>>>>> +++ b/rust/kernel/time/hrtimer.rs
>>>>>>>>> @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant<T>
>>>>>>>>> // - Timers cannot have negative ktime_t values as their expiration time.
>>>>>>>>> // - There's no actual locking here, a racy read is fine and expected
>>>>>>>>> unsafe {
>>>>>>>>> - Instant::from_ktime(
>>>>>>>>> - // This `read_volatile` is intended to correspond to a READ_ONCE call.
>>>>>>>>> - // FIXME(read_once): Replace with `read_once` when available on the Rust side.
>>>>>>>>> - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)),
>>>>>>>>> - )
>>>>>>>>> + Instant::from_ktime(kernel::sync::READ_ONCE(
>>>>>>>>> + &raw const (*c_timer_ptr).node.expires,
>>>>>>>>> + ))
>>>>>>>>> }
>>>>>>>>
>>>>>>>> Do we actually need READ_ONCE() here? I'm not sure but would it be
>>>>>>>> better to call the C-side API?
>>>>>>>>
>>>>>>>> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
>>>>>>>> index 67a36ccc3ec4..73162dea2a29 100644
>>>>>>>> --- a/rust/helpers/time.c
>>>>>>>> +++ b/rust/helpers/time.c
>>>>>>>> @@ -2,6 +2,7 @@
>>>>>>>>
>>>>>>>> #include <linux/delay.h>
>>>>>>>> #include <linux/ktime.h>
>>>>>>>> +#include <linux/hrtimer.h>
>>>>>>>> #include <linux/timekeeping.h>
>>>>>>>>
>>>>>>>> void rust_helper_fsleep(unsigned long usecs)
>>>>>>>> @@ -38,3 +39,8 @@ void rust_helper_udelay(unsigned long usec)
>>>>>>>> {
>>>>>>>> udelay(usec);
>>>>>>>> }
>>>>>>>> +
>>>>>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>>>>>> +{
>>>>>>>> + return timer->node.expires;
>>>>>>>> +}
>>>>>>>
>>>>>>> Sorry, of course this should be:
>>>>>>>
>>>>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>>>>> +{
>>>>>>> + return hrtimer_get_expires(timer);
>>>>>>> +}
>>>>>>>
>>>>>>
>>>>>> This is a potentially racy read. As far as I recall, we determined that
>>>>>> using read_once is the proper way to handle the situation.
>>>>>>
>>>>>> I do not think it makes a difference that the read is done by C code.
>>>>>
>>>>> What does "racy read" mean here?
>>>>>
>>>>> The C side doesn't use WRITE_ONCE() or READ_ONCE for node.expires. How
>>>>> would using READ_ONCE() on the Rust side make a difference?
>>>>
>>>> Data races like this are UB in Rust. As far as I understand, using this
>>>> READ_ONCE implementation or a relaxed atomic read would make the read
>>>> well defined. I am not aware if this is only the case if all writes to
>>>> the location from C also use atomic operations or WRITE_ONCE. @Boqun?
>>>
>>> The C side updates node.expires without WRITE_ONCE()/atomics so a
>>> Rust-side READ_ONCE() can still observe a torn value; I think that
>>> this is still a data race / UB from Rust's perspective.
>>>
>>> And since expires is 64-bit, WRITE_ONCE() on 32-bit architectures does
>>> not inherently guarantee tear-free stores either.
>>>
>>> I think that the expires() method should follow the same safety
>>> requirements as raw_forward(): it should only be considered safe when
>>> holding exclusive access to hrtimer or within the context of the timer
>>> callback. Under those conditions, it would be fine to call C's
>>> hrtimer_get_expires().
>>
>> We can make it safe, please see my comment here [1].
>>
>> Best regards,
>> Andreas Hindborg
>>
>> [1] https://lore.kernel.org/r/87v7hdh9m4.fsf@t14s.mail-host-address-is-not-set
>
> I agree. My point was that expire() can be safe only under the same
> constraints as forward()/forward_now() so the API should require
> Pin<&mut Self> and expose it on HrTimerCallbackContext.
Do you want to send a patch?
Best regards,
Andreas Hindborg
On Wed, Jan 07, 2026 at 11:11:43AM +0100, Andreas Hindborg wrote: > FUJITA Tomonori <fujita.tomonori@gmail.com> writes: > [...] > >>> > >> > >> This is a potentially racy read. As far as I recall, we determined that > >> using read_once is the proper way to handle the situation. > >> > >> I do not think it makes a difference that the read is done by C code. > > > > What does "racy read" mean here? > > > > The C side doesn't use WRITE_ONCE() or READ_ONCE for node.expires. How > > would using READ_ONCE() on the Rust side make a difference? > > Data races like this are UB in Rust. As far as I understand, using this > READ_ONCE implementation or a relaxed atomic read would make the read > well defined. I am not aware if this is only the case if all writes to > the location from C also use atomic operations or WRITE_ONCE. @Boqun? > I took a look into this, the current C code is probably fine (i.e. without READ_ONCE() or WRITE_ONCE()) because the accesses are 1) protected by timer base locking or 2) in a timer callback which provides exclusive accesses to .expires as well. Note that hrtimer_cancel() doesn't need to access .expires, so a timer callback racing with a hrtimer_cancel() is fine. (I may miss one or two cases, but most of the cases are fine) The problem in Rust code is that HrTimer::expires() is a pub function, so in 2) a HrTimer::expires() can race with hrtimer_forward(), which causes data races. We either change hrtimer C code to support such a usage (against data races) or change the usage of this HrTimer::expires() function. Using READ_ONCE() here won't work. (Yes, we could say assuming all plain writes on .expires in C are atomic as some other code does, but hrtimer doesn't rely on this, so I don't think we should either) Regards, Boqun > > Best regards, > Andreas Hindborg > >
Boqun Feng <boqun.feng@gmail.com> writes: > On Wed, Jan 07, 2026 at 11:11:43AM +0100, Andreas Hindborg wrote: >> FUJITA Tomonori <fujita.tomonori@gmail.com> writes: >> > [...] >> >>> >> >> >> >> This is a potentially racy read. As far as I recall, we determined that >> >> using read_once is the proper way to handle the situation. >> >> >> >> I do not think it makes a difference that the read is done by C code. >> > >> > What does "racy read" mean here? >> > >> > The C side doesn't use WRITE_ONCE() or READ_ONCE for node.expires. How >> > would using READ_ONCE() on the Rust side make a difference? >> >> Data races like this are UB in Rust. As far as I understand, using this >> READ_ONCE implementation or a relaxed atomic read would make the read >> well defined. I am not aware if this is only the case if all writes to >> the location from C also use atomic operations or WRITE_ONCE. @Boqun? >> > > I took a look into this, the current C code is probably fine (i.e. > without READ_ONCE() or WRITE_ONCE()) because the accesses are > > 1) protected by timer base locking or > 2) in a timer callback which provides exclusive accesses to .expires as > well. Note that hrtimer_cancel() doesn't need to access .expires, so > a timer callback racing with a hrtimer_cancel() is fine. > > (I may miss one or two cases, but most of the cases are fine) > > The problem in Rust code is that HrTimer::expires() is a pub function, > so in 2) a HrTimer::expires() can race with hrtimer_forward(), which > causes data races. > > We either change hrtimer C code to support such a usage (against data > races) or change the usage of this HrTimer::expires() function. Using > READ_ONCE() here won't work. (Yes, we could say assuming all plain > writes on .expires in C are atomic as some other code does, but hrtimer > doesn't rely on this, so I don't think we should either) I don't think we should change the C code, I think the Rust API is simply wrong. The function should have same constraints as `forward_now`, ie. call while having exclusive access to the timer (during setup for instance), or in callback context. We should change it to take `self: Pin<&mut Self>` and add it on `HrTimerCallbackContext` as well. @Tomo, do you know of any users of this function? Best regards, Andreas Hindborg
On Tue, 06 Jan 2026 13:37:34 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:
> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> >
> > Sorry, of course this should be:
> >
> > +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
> > +{
> > + return hrtimer_get_expires(timer);
> > +}
> >
>
> This is a potentially racy read. As far as I recall, we determined that
> using read_once is the proper way to handle the situation.
>
> I do not think it makes a difference that the read is done by C code.
If that's the case I think the C code should be fixed by inserting the
READ_ONCE?
Best,
Gary
On Tue, Jan 06, 2026 at 03:23:00PM +0000, Gary Guo wrote:
> On Tue, 06 Jan 2026 13:37:34 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> > >
> > > Sorry, of course this should be:
> > >
> > > +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
> > > +{
> > > + return hrtimer_get_expires(timer);
> > > +}
> > >
> >
> > This is a potentially racy read. As far as I recall, we determined that
> > using read_once is the proper way to handle the situation.
> >
> > I do not think it makes a difference that the read is done by C code.
>
> If that's the case I think the C code should be fixed by inserting the
> READ_ONCE?
I maintain my position that if this is what you recommend C code does,
it's confusing to not make the same recommendation for Rust abstractions
to the same thing.
After all, nothing is stopping you from calling atomic_read() in C too.
Alice
On Tue, Jan 06, 2026 at 06:43:17PM +0000, Alice Ryhl wrote:
> On Tue, Jan 06, 2026 at 03:23:00PM +0000, Gary Guo wrote:
> > On Tue, 06 Jan 2026 13:37:34 +0100
> > Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >
> > > "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> > > >
> > > > Sorry, of course this should be:
> > > >
> > > > +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
> > > > +{
> > > > + return hrtimer_get_expires(timer);
> > > > +}
> > > >
> > >
> > > This is a potentially racy read. As far as I recall, we determined that
> > > using read_once is the proper way to handle the situation.
> > >
> > > I do not think it makes a difference that the read is done by C code.
> >
> > If that's the case I think the C code should be fixed by inserting the
> > READ_ONCE?
>
> I maintain my position that if this is what you recommend C code does,
> it's confusing to not make the same recommendation for Rust abstractions
> to the same thing.
The problem here is that C code should use atomic operation here, and
it can be done via READ_ONCE() in C, and in Rust, it should be done by
Atomic::from_ptr().load().
The recommendation is not "using READ_ONCE()" for C, it should be "using
reads that are atomic here", and that's why introducing READ_ONCE() in
Rust is a bad idea, because what we need here is an atomic operation not
a "magical thing that C relies on so we are fine".
>
> After all, nothing is stopping you from calling atomic_read() in C too.
>
Actually using atomic_read() in C should also work, it'll be technically
correct as well.
Regards,
Boqun
> Alice
On 1/6/26 10:43 AM, Alice Ryhl wrote:
> On Tue, Jan 06, 2026 at 03:23:00PM +0000, Gary Guo wrote:
>> On Tue, 06 Jan 2026 13:37:34 +0100
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>>>
>>>> Sorry, of course this should be:
>>>>
>>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
>>>> +{
>>>> + return hrtimer_get_expires(timer);
>>>> +}
>>>>
>>>
>>> This is a potentially racy read. As far as I recall, we determined that
>>> using read_once is the proper way to handle the situation.
>>>
>>> I do not think it makes a difference that the read is done by C code.
>>
>> If that's the case I think the C code should be fixed by inserting the
>> READ_ONCE?
>
> I maintain my position that if this is what you recommend C code does,
> it's confusing to not make the same recommendation for Rust abstractions
> to the same thing.
>
> After all, nothing is stopping you from calling atomic_read() in C too.
>
Hi Alice and everyone!
I'm having trouble fully understanding the latest reply, so maybe what
I'm saying is actually what you just said.
Anyway, we should use READ_ONCE in both the C and Rust code. Relying
on the compiler for that is no longer OK. We shouldn't be shy about
fixing the C side (not that I think you have been, so far!).
thanks,
--
John Hubbard
On Tue, Jan 06, 2026 at 04:47:35PM -0800, John Hubbard wrote:
> On 1/6/26 10:43 AM, Alice Ryhl wrote:
> > On Tue, Jan 06, 2026 at 03:23:00PM +0000, Gary Guo wrote:
> >> On Tue, 06 Jan 2026 13:37:34 +0100
> >> Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> >>>>
> >>>> Sorry, of course this should be:
> >>>>
> >>>> +__rust_helper ktime_t rust_helper_hrtimer_get_expires(const struct hrtimer *timer)
> >>>> +{
> >>>> + return hrtimer_get_expires(timer);
> >>>> +}
> >>>>
> >>>
> >>> This is a potentially racy read. As far as I recall, we determined that
> >>> using read_once is the proper way to handle the situation.
> >>>
> >>> I do not think it makes a difference that the read is done by C code.
> >>
> >> If that's the case I think the C code should be fixed by inserting the
> >> READ_ONCE?
> >
> > I maintain my position that if this is what you recommend C code does,
> > it's confusing to not make the same recommendation for Rust abstractions
> > to the same thing.
> >
> > After all, nothing is stopping you from calling atomic_read() in C too.
> >
>
> Hi Alice and everyone!
>
> I'm having trouble fully understanding the latest reply, so maybe what
> I'm saying is actually what you just said.
>
> Anyway, we should use READ_ONCE in both the C and Rust code. Relying
> on the compiler for that is no longer OK. We shouldn't be shy about
> fixing the C side (not that I think you have been, so far!).
>
Agreed on most of it, except that we should be more explicit in Rust,
by using atomic_load[1] instead of READ_ONCE().
[1]: https://lore.kernel.org/rust-for-linux/aV0FxCRzXFrNLZik@tardis-2.local/
Regards,
Boqun
> thanks,
> --
> John Hubbard
>
On 1/6/26 5:08 PM, Boqun Feng wrote: > On Tue, Jan 06, 2026 at 04:47:35PM -0800, John Hubbard wrote: >> On 1/6/26 10:43 AM, Alice Ryhl wrote: >>> On Tue, Jan 06, 2026 at 03:23:00PM +0000, Gary Guo wrote: >>>> On Tue, 06 Jan 2026 13:37:34 +0100 >>>> Andreas Hindborg <a.hindborg@kernel.org> wrote: >>>>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: ... >>>>> This is a potentially racy read. As far as I recall, we determined that >>>>> using read_once is the proper way to handle the situation. >>>>> >>>>> I do not think it makes a difference that the read is done by C code. >>>> >>>> If that's the case I think the C code should be fixed by inserting the >>>> READ_ONCE? >>> >>> I maintain my position that if this is what you recommend C code does, >>> it's confusing to not make the same recommendation for Rust abstractions >>> to the same thing. >>> >>> After all, nothing is stopping you from calling atomic_read() in C too. >>> >> >> Hi Alice and everyone! >> >> I'm having trouble fully understanding the latest reply, so maybe what >> I'm saying is actually what you just said. >> >> Anyway, we should use READ_ONCE in both the C and Rust code. Relying >> on the compiler for that is no longer OK. We shouldn't be shy about >> fixing the C side (not that I think you have been, so far!). >> > > Agreed on most of it, except that we should be more explicit in Rust, > by using atomic_load[1] instead of READ_ONCE(). > > [1]: https://lore.kernel.org/rust-for-linux/aV0FxCRzXFrNLZik@tardis-2.local/ > I see. That does put things in a much clearer state, yes. thanks, -- John Hubbard
© 2016 - 2026 Red Hat, Inc.