Hi Thomas & Miguel,
These are two tiny fixes/cleanup for the kernel::time, the first one is
adding missing links to C header files, and the second one is switching
to use wrapping_sub() for `Ktime::sub()`. The series is based on today's
tip/timers/core.
Thanks!
Boqun Feng (2):
rust: time: doc: Add missing C header links
rust: time: Use wrapping_sub() for Ktime::sub()
rust/kernel/time.rs | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--
2.44.0
The definitions related to jiffies are at linux/jiffies.h, and the
definitions related to ktime_t are at linux/ktime.h, since
`kernel::time` provides the functionality dealing with jiffies and
ktime_t, it makes sense to add links to them from Rust's time module.
Signed-off-by: Boqun Feng <[email protected]>
---
rust/kernel/time.rs | 3 +++
1 file changed, 3 insertions(+)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 6811d5cadbd4..e3bb5e89f88d 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -4,6 +4,9 @@
//!
//! This module contains the kernel APIs related to time and timers that
//! have been ported or wrapped for usage by Rust code in the kernel.
+//!
+//! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
+//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
/// The number of nanoseconds per millisecond.
pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
--
2.44.0
Currently since Rust code is compiled with "-Coverflow-checks=y", so a
normal substraction may be compiled as an overflow checking and panic
if overflow happens:
subq %rsi, %rdi
jo .LBB0_2
movq %rdi, %rax
retq
LBB0_2:
pushq %rax
leaq str.0(%rip), %rdi
leaq .L__unnamed_1(%rip), %rdx
movl $33, %esi
callq *core::panicking::panic::h59297120e85ea178@GOTPCREL(%rip)
although overflow detection is nice to have, however this makes
`Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
clear that the overflow checking is helpful, since for example, the
current binder usage[1] doesn't have the checking.
Therefore make `Ktime::sub()` have the same semantics as `ktime_sub()`:
overflow behaves like 2s-complement wrapping sub.
Link: https://lore.kernel.org/lkml/5ac8c0d09392290be789423f0dd78a520b830fab.1682333709.git.zhangchuang3@xiaomi.com/ [1]
Signed-off-by: Boqun Feng <[email protected]>
---
rust/kernel/time.rs | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index e3bb5e89f88d..3cb15d3079f4 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -77,7 +77,9 @@ impl core::ops::Sub for Ktime {
#[inline]
fn sub(self, other: Ktime) -> Ktime {
Self {
- inner: self.inner - other.inner,
+ // Mirrors `ktime_sub()`, kernel defines signed overflow to behave like 2s-complement,
+ // hence `wrapping_sub()` is used.
+ inner: self.inner.wrapping_sub(other.inner),
}
}
}
--
2.44.0
On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]> wrote:
>
> Currently since Rust code is compiled with "-Coverflow-checks=y", so a
Nit: it is enabled by default, but configurable (`CONFIG_RUST_OVERFLOW_CHECKS`).
> although overflow detection is nice to have, however this makes
> `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> clear that the overflow checking is helpful, since for example, the
> current binder usage[1] doesn't have the checking.
>
> Therefore make `Ktime::sub()` have the same semantics as `ktime_sub()`:
> overflow behaves like 2s-complement wrapping sub.
If `ktime_sub()`'s callers rely on wrapping in some cases, then an
alternative we should consider is having a method for explicitly
wrapping, like the integers. This would allow callers to decide and it
would make the expected semantics clear since the beginning (which is
the easiest time to add this kind of thing) for Rust code.
Otherwise, I agree we should at least document the preconditions clearly.
Having said that, I see a `ktime_add_unsafe()` too, which was added
due to a UBSAN report for `ktime_add()` in commit 979515c56458 ("time:
Avoid undefined behaviour in ktime_add_safe()"). There is also a
private `ktime_add_safe()` too, which is a saturating one.
So, given that, can callers actually rely on wrapping for these
functions, or not? The documentation on the C side could perhaps be
clarified here (including the mention of UB in `ktime_add_unsafe()` --
we use `-fno-strict-overflow`) and perhaps using the `wrapping_*()` C
functions too.
In addition, Binder calls `ktime_ms_delta()`, not `ktime_sub()`,
right? In that case the arguments are called `later` and `earlier`,
perhaps those have a different expectation even if `ktime_sub()` is
allowed to overflow and thus it would make sense to check in that
function only instead? (and document accordingly)
Thanks!
Cheers,
Miguel
On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]> wrote:
>
> The definitions related to jiffies are at linux/jiffies.h, and the
> definitions related to ktime_t are at linux/ktime.h, since
> `kernel::time` provides the functionality dealing with jiffies and
> ktime_t, it makes sense to add links to them from Rust's time module.
>
> Signed-off-by: Boqun Feng <[email protected]>
Acked-by: Miguel Ojeda <[email protected]>
Thanks Boqun!
Cheers,
Miguel
On Fri, 2024-04-12 at 09:14 +0200, Miguel Ojeda wrote:
> On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]>
> wrote:
> >
> > Currently since Rust code is compiled with "-Coverflow-checks=y",
> > so a
>
> Nit: it is enabled by default, but configurable
> (`CONFIG_RUST_OVERFLOW_CHECKS`).
Is that going to remain enabled by default or what was the plan here?
P.
>
> > although overflow detection is nice to have, however this makes
> > `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's
> > not
> > clear that the overflow checking is helpful, since for example, the
> > current binder usage[1] doesn't have the checking.
> >
> > Therefore make `Ktime::sub()` have the same semantics as
> > `ktime_sub()`:
> > overflow behaves like 2s-complement wrapping sub.
>
> If `ktime_sub()`'s callers rely on wrapping in some cases, then an
> alternative we should consider is having a method for explicitly
> wrapping, like the integers. This would allow callers to decide and
> it
> would make the expected semantics clear since the beginning (which is
> the easiest time to add this kind of thing) for Rust code.
>
> Otherwise, I agree we should at least document the preconditions
> clearly.
>
> Having said that, I see a `ktime_add_unsafe()` too, which was added
> due to a UBSAN report for `ktime_add()` in commit 979515c56458
> ("time:
> Avoid undefined behaviour in ktime_add_safe()"). There is also a
> private `ktime_add_safe()` too, which is a saturating one.
>
> So, given that, can callers actually rely on wrapping for these
> functions, or not? The documentation on the C side could perhaps be
> clarified here (including the mention of UB in `ktime_add_unsafe()` -
> -
> we use `-fno-strict-overflow`) and perhaps using the `wrapping_*()` C
> functions too.
>
> In addition, Binder calls `ktime_ms_delta()`, not `ktime_sub()`,
> right? In that case the arguments are called `later` and `earlier`,
> perhaps those have a different expectation even if `ktime_sub()` is
> allowed to overflow and thus it would make sense to check in that
> function only instead? (and document accordingly)
>
> Thanks!
>
> Cheers,
> Miguel
>
On Fri, Apr 12, 2024 at 9:43 AM Philipp Stanner <[email protected]> wrote:
>
> Is that going to remain enabled by default or what was the plan here?
The plan is to ideally keep it enabled by default, but I defer to Kees
with whom we discussed this back then (Cc'd).
The goal is that Rust code, since the beginning, has all wrapping
operations marked explicitly as such.
Cheers,
Miguel
On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]> wrote:
>
> Currently since Rust code is compiled with "-Coverflow-checks=y", so a
> normal substraction may be compiled as an overflow checking and panic
> if overflow happens:
>
> subq %rsi, %rdi
> jo .LBB0_2
> movq %rdi, %rax
> retq
> .LBB0_2:
> pushq %rax
> leaq str.0(%rip), %rdi
> leaq .L__unnamed_1(%rip), %rdx
> movl $33, %esi
> callq *core::panicking::panic::h59297120e85ea178@GOTPCREL(%rip)
>
> although overflow detection is nice to have, however this makes
> `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> clear that the overflow checking is helpful, since for example, the
> current binder usage[1] doesn't have the checking.
I don't think this is a good idea at all. Any code that triggers an
overflow in Ktime::sub is wrong, and anyone who enables
CONFIG_RUST_OVERFLOW_CHECKS does so because they want such bugs to be
caught. You may have been able to find one example of a subtraction
that doesn't have a risk of overflow, but overflow bugs really do
happen in the real world. I have seen real examples of bugs in Rust
code, where overflow checks were the reason the bug was not a security
vulnerability.
> Therefore make `Ktime::sub()` have the same semantics as `ktime_sub()`:
> overflow behaves like 2s-complement wrapping sub.
From Miguel's reply, it sounds like 2s-complement wrapping is not even
the semantics of ktime_sub. The semantics are just UB.
Alice
On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]> wrote:
>
> The definitions related to jiffies are at linux/jiffies.h, and the
> definitions related to ktime_t are at linux/ktime.h, since
> `kernel::time` provides the functionality dealing with jiffies and
> ktime_t, it makes sense to add links to them from Rust's time module.
>
> Signed-off-by: Boqun Feng <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
On Fri, Apr 12, 2024 at 10:36:05AM +0200, Alice Ryhl wrote:
> On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]> wrote:
> >
> > Currently since Rust code is compiled with "-Coverflow-checks=y", so a
> > normal substraction may be compiled as an overflow checking and panic
> > if overflow happens:
> >
> > subq %rsi, %rdi
> > jo .LBB0_2
> > movq %rdi, %rax
> > retq
> > .LBB0_2:
> > pushq %rax
> > leaq str.0(%rip), %rdi
> > leaq .L__unnamed_1(%rip), %rdx
> > movl $33, %esi
> > callq *core::panicking::panic::h59297120e85ea178@GOTPCREL(%rip)
> >
> > although overflow detection is nice to have, however this makes
> > `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> > clear that the overflow checking is helpful, since for example, the
> > current binder usage[1] doesn't have the checking.
>
> I don't think this is a good idea at all. Any code that triggers an
> overflow in Ktime::sub is wrong, and anyone who enables
> CONFIG_RUST_OVERFLOW_CHECKS does so because they want such bugs to be
> caught. You may have been able to find one example of a subtraction
> that doesn't have a risk of overflow, but overflow bugs really do
The point is you won't panic the kernel because of an overflow. I
agree that overflow is something we want to catch, but currently
ktime_t doesn't panic if overflow happens.
Regards,
Boqun
> happen in the real world. I have seen real examples of bugs in Rust
> code, where overflow checks were the reason the bug was not a security
> vulnerability.
>
> > Therefore make `Ktime::sub()` have the same semantics as `ktime_sub()`:
> > overflow behaves like 2s-complement wrapping sub.
>
> From Miguel's reply, it sounds like 2s-complement wrapping is not even
> the semantics of ktime_sub. The semantics are just UB.
>
> Alice
On Fri, Apr 12, 2024 at 09:14:03AM +0200, Miguel Ojeda wrote:
> On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]> wrote:
> >
> > Currently since Rust code is compiled with "-Coverflow-checks=y", so a
>
> Nit: it is enabled by default, but configurable (`CONFIG_RUST_OVERFLOW_CHECKS`).
>
Ok, I will change it accordingly.
> > although overflow detection is nice to have, however this makes
> > `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> > clear that the overflow checking is helpful, since for example, the
> > current binder usage[1] doesn't have the checking.
> >
> > Therefore make `Ktime::sub()` have the same semantics as `ktime_sub()`:
> > overflow behaves like 2s-complement wrapping sub.
>
> If `ktime_sub()`'s callers rely on wrapping in some cases, then an
> alternative we should consider is having a method for explicitly
> wrapping, like the integers. This would allow callers to decide and it
That works for me, although I would prefer `Ktime::sub()` is wrapping
sub and we have another function doing a safe version of sub.
> would make the expected semantics clear since the beginning (which is
> the easiest time to add this kind of thing) for Rust code.
>
> Otherwise, I agree we should at least document the preconditions clearly.
>
> Having said that, I see a `ktime_add_unsafe()` too, which was added
> due to a UBSAN report for `ktime_add()` in commit 979515c56458 ("time:
> Avoid undefined behaviour in ktime_add_safe()"). There is also a
> private `ktime_add_safe()` too, which is a saturating one.
>
Exactly, ktime_add_safe() doesn't panic if overflow happens, right?
I think that's pretty clear on how time subsystem wants to handle
overflow (saturating it, or zeroing it instead of panicing).
> So, given that, can callers actually rely on wrapping for these
> functions, or not? The documentation on the C side could perhaps be
> clarified here (including the mention of UB in `ktime_add_unsafe()` --
> we use `-fno-strict-overflow`) and perhaps using the `wrapping_*()` C
> functions too.
>
I must defer this to Thomas.
> In addition, Binder calls `ktime_ms_delta()`, not `ktime_sub()`,
> right? In that case the arguments are called `later` and `earlier`,
> perhaps those have a different expectation even if `ktime_sub()` is
> allowed to overflow and thus it would make sense to check in that
> function only instead? (and document accordingly)
>
Maybe, however neither of this function probably shouldn't have the
panic-on-overflow behavior. So I agree that overflow checking is not a
bad thing, but when to check and how to handle overflow should be
controlled by the users, and making the default behavior
panic-on-overflow doesn't look reasonable to me.
Regards,
Boqun
> Thanks!
>
> Cheers,
> Miguel
On Fri, Apr 12, 2024 at 3:18 PM Boqun Feng <[email protected]> wrote:
>
> On Fri, Apr 12, 2024 at 10:36:05AM +0200, Alice Ryhl wrote:
> > On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]> wrote:
> > >
> > > Currently since Rust code is compiled with "-Coverflow-checks=y", so a
> > > normal substraction may be compiled as an overflow checking and panic
> > > if overflow happens:
> > >
> > > subq %rsi, %rdi
> > > jo .LBB0_2
> > > movq %rdi, %rax
> > > retq
> > > .LBB0_2:
> > > pushq %rax
> > > leaq str.0(%rip), %rdi
> > > leaq .L__unnamed_1(%rip), %rdx
> > > movl $33, %esi
> > > callq *core::panicking::panic::h59297120e85ea178@GOTPCREL(%rip)
> > >
> > > although overflow detection is nice to have, however this makes
> > > `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> > > clear that the overflow checking is helpful, since for example, the
> > > current binder usage[1] doesn't have the checking.
> >
> > I don't think this is a good idea at all. Any code that triggers an
> > overflow in Ktime::sub is wrong, and anyone who enables
> > CONFIG_RUST_OVERFLOW_CHECKS does so because they want such bugs to be
> > caught. You may have been able to find one example of a subtraction
> > that doesn't have a risk of overflow, but overflow bugs really do
>
> The point is you won't panic the kernel because of an overflow. I
> agree that overflow is something we want to catch, but currently
> ktime_t doesn't panic if overflow happens.
What the CONFIG_RUST_OVERFLOW_CHECKS option does is enable panics on
overflow. So I don't understand how "it panics on overflow" is an
argument for removing the overflow check. That's what you asked for!
One could perhaps argue about whether CONFIG_RUST_OVERFLOW_CHECKS is a
good idea (I think it is), but that is orthogonal. When
CONFIG_RUST_OVERFLOW_CHECKS is enabled, you should respect the flag.
Alice
On Fri, Apr 12, 2024 at 3:34 PM Boqun Feng <[email protected]> wrote:
>
> That works for me, although I would prefer `Ktime::sub()` is wrapping
> sub and we have another function doing a safe version of sub.
Why? It goes against the "normal" case in integers. It is also not
what `ktime_sub()` does, which is the "normal" case here, vs.
`_unsafe()` and `_safe()` ones.
> Exactly, ktime_add_safe() doesn't panic if overflow happens, right?
> I think that's pretty clear on how time subsystem wants to handle
> overflow (saturating it, or zeroing it instead of panicing).
There are three variants in C (for addition) that I can see:
- No suffix: not supposed to wrap.
- `_unsafe()`: wraps.
- `_safe()`: saturates.
The first one, in normal C, would be UB. In kernel C, it wraps but may
be detected by UBSAN (this is what Kees is re-introducing very
recently with 557f8c582a9b ("ubsan: Reintroduce signed overflow
sanitizer")).
So, in Rust terms, the three options above would map to:
- Raw operators.
- `wrapping_`.
- `saturating_`.
Because the raw operators are what we use for arithmetic that is "not
supposed to wrap" too. That is, they wrap, but may be checked by the
Kconfig option. Of course, it may be worth having an intermediate
option that does not actually go for a full-blown Rust-panic for that,
but the point is that the current "not supposed to wrap" methods are
the raw operators.
All three, in fact, are "safe" in Rust terms, since none can actually
trigger UB (in kernel C at least -- it would be different in normal C:
the first one would map to an unsafe Rust method, i.e. `unchecked_`).
Instead, in the C side, `_unsafe()` seems to be used to mean instead
"you should be checking for overflow if needed, because it will never
be reported by UBSAN unlike the raw one". Again, this is based on my
reading of that commit and the docs on `_unsafe()`. It may be wrong,
or maybe the subtraction is supposed to be different. It should
probably be clarified in the C side anyway.
And, relatedly, I see that when the `union` was removed in commit
2456e8553544 ("ktime: Get rid of the union"), `ktime_add_unsafe()`
stopped returning a `ktime_t` even when both inputs are `ktime_t`s
themselves:
static_assert(_Generic(ktime_add(a, b), ktime_t: true, default:
false)); // OK
static_assert(_Generic(ktime_add_unsafe(a, b), ktime_t: true,
default: false)); // Bad
It returns an `u64` now, which could surprise users, and probably
should be fixed. The only user just puts the result into a `ktime_t`,
so there is no actual issue today.
> I must defer this to Thomas.
Yeah, the question on the C API was meant for Thomas et al.
> Maybe, however neither of this function probably shouldn't have the
> panic-on-overflow behavior. So I agree that overflow checking is not a
> bad thing, but when to check and how to handle overflow should be
> controlled by the users, and making the default behavior
> panic-on-overflow doesn't look reasonable to me.
Yes, it should be controlled by callers, but the point above is that,
from the looks of it, these interfaces are not meant to overflow to
begin with.
Cheers,
Miguel
On Fri, Apr 12, 2024 at 04:41:26PM +0200, Miguel Ojeda wrote:
> On Fri, Apr 12, 2024 at 3:34 PM Boqun Feng <[email protected]> wrote:
> >
> > That works for me, although I would prefer `Ktime::sub()` is wrapping
> > sub and we have another function doing a safe version of sub.
>
> Why? It goes against the "normal" case in integers. It is also not
> what `ktime_sub()` does, which is the "normal" case here, vs.
Seems we have a different reading of `ktime_sub()` ;-)
Based on your reply to Philipp, I take it that
CONFIG_RUST_CHECK_OVERFLOWS can be enabled in a production kernel,
right? IOW, it's not a debug-only feature like UBSAN (or maybe I'm way
wrong, that UBSAN is also a feature that production kernel can or
already use?). If so, then the current `Ktime::sub()` has a different
behavior compared to `ktime_sub()`: it will perform overflow checks and
panic (which is BUG()) in production kernels.
Now I wasn't trying to say substraction overflows shouldn't be checked
(by default), the thing is that `Ktime` is just a `ktime_t` wrapper, so
it's natural that it provides as least difference as possible. If it was
a standalone abstraction, then by all means let's add different APIs for
different purpose.
If you look at ktime API, ktime_sub() is the only one doing
substraction between two ktime_t, there is no raw or unsafe or safe API,
So as a minimal abstraction, it's natural for a user to expect
`Ktime::sub()` behaves like `ktime_sub()`.
That's my reasoning, but it depends one a few "if"s and what time
subsystem wants to do.
> `_unsafe()` and `_safe()` ones.
>
> > Exactly, ktime_add_safe() doesn't panic if overflow happens, right?
> > I think that's pretty clear on how time subsystem wants to handle
> > overflow (saturating it, or zeroing it instead of panicing).
>
> There are three variants in C (for addition) that I can see:
>
> - No suffix: not supposed to wrap.
> - `_unsafe()`: wraps.
> - `_safe()`: saturates.
>
> The first one, in normal C, would be UB. In kernel C, it wraps but may
> be detected by UBSAN (this is what Kees is re-introducing very
> recently with 557f8c582a9b ("ubsan: Reintroduce signed overflow
> sanitizer")).
>
> So, in Rust terms, the three options above would map to:
>
> - Raw operators.
> - `wrapping_`.
> - `saturating_`.
>
> Because the raw operators are what we use for arithmetic that is "not
> supposed to wrap" too. That is, they wrap, but may be checked by the
> Kconfig option. Of course, it may be worth having an intermediate
> option that does not actually go for a full-blown Rust-panic for that,
> but the point is that the current "not supposed to wrap" methods are
> the raw operators.
>
> All three, in fact, are "safe" in Rust terms, since none can actually
> trigger UB (in kernel C at least -- it would be different in normal C:
> the first one would map to an unsafe Rust method, i.e. `unchecked_`).
>
> Instead, in the C side, `_unsafe()` seems to be used to mean instead
> "you should be checking for overflow if needed, because it will never
> be reported by UBSAN unlike the raw one". Again, this is based on my
> reading of that commit and the docs on `_unsafe()`. It may be wrong,
> or maybe the subtraction is supposed to be different. It should
> probably be clarified in the C side anyway.
>
> And, relatedly, I see that when the `union` was removed in commit
> 2456e8553544 ("ktime: Get rid of the union"), `ktime_add_unsafe()`
> stopped returning a `ktime_t` even when both inputs are `ktime_t`s
> themselves:
>
> static_assert(_Generic(ktime_add(a, b), ktime_t: true, default:
> false)); // OK
> static_assert(_Generic(ktime_add_unsafe(a, b), ktime_t: true,
> default: false)); // Bad
>
> It returns an `u64` now, which could surprise users, and probably
> should be fixed. The only user just puts the result into a `ktime_t`,
> so there is no actual issue today.
>
> > I must defer this to Thomas.
>
> Yeah, the question on the C API was meant for Thomas et al.
>
Maybe it's wise to just wait for them to reply, I don't think you and I
have much disagree other than ktime_t API semantics ;-)
Regards,
Boqun
> > Maybe, however neither of this function probably shouldn't have the
> > panic-on-overflow behavior. So I agree that overflow checking is not a
> > bad thing, but when to check and how to handle overflow should be
> > controlled by the users, and making the default behavior
> > panic-on-overflow doesn't look reasonable to me.
>
> Yes, it should be controlled by callers, but the point above is that,
> from the looks of it, these interfaces are not meant to overflow to
> begin with.
>
> Cheers,
> Miguel
On Sat, Apr 13, 2024 at 3:30 AM Boqun Feng <[email protected]> wrote:
>
> Based on your reply to Philipp, I take it that
> CONFIG_RUST_CHECK_OVERFLOWS can be enabled in a production kernel,
> right? IOW, it's not a debug-only feature like UBSAN (or maybe I'm way
Yeah, it is intended to be used in production (for those that want it,
i.e. not everybody will want it; and probably we will want to provide
other modes of operation when the check fails as I was mentioning,
e.g. report and continue with wrap).
> wrong, that UBSAN is also a feature that production kernel can or
> already use?). If so, then the current `Ktime::sub()` has a different
Generally userspace sanitizers aren't, but they may be (like the
minimal runtime in Clang).
Not sure about the "status" of each kernel one, Cc'ing Kees (sorry,
you got dropped), but I imagine things like the integer overflow ones
with `UBSAN_TRAP=y` may be fine in production.
> Now I wasn't trying to say substraction overflows shouldn't be checked
> (by default), the thing is that `Ktime` is just a `ktime_t` wrapper, so
> it's natural that it provides as least difference as possible. If it was
> a standalone abstraction, then by all means let's add different APIs for
> different purpose.
Agreed that we should generally avoid surprises, but here the C side
may be actually expecting the same (i.e. no overflows).
But if that is not the case, and then you think we should call this a
different name than `Ktime` to avoid confusion, that is fair.
> If you look at ktime API, ktime_sub() is the only one doing
> substraction between two ktime_t, there is no raw or unsafe or safe API,
> So as a minimal abstraction, it's natural for a user to expect
> `Ktime::sub()` behaves like `ktime_sub()`.
Yeah, but as I was mentioning, in the `add` case, it seems like it is
not intended to overflow. Thus one could assume (perhaps naively) the
subtraction isn't, either.
> Maybe it's wise to just wait for them to reply, I don't think you and I
> have much disagree other than ktime_t API semantics ;-)
Indeed :)
Cheers,
Miguel
On Fri, Apr 12, 2024 at 09:58:57AM +0200, Miguel Ojeda wrote:
> On Fri, Apr 12, 2024 at 9:43 AM Philipp Stanner <[email protected]> wrote:
> >
> > Is that going to remain enabled by default or what was the plan here?
>
> The plan is to ideally keep it enabled by default, but I defer to Kees
> with whom we discussed this back then (Cc'd).
Yeah, we want to keep "trap on overflow" the default for Rust. We're
slowly making our way there[1] for C in Linux, so I don't want to
regress the Rust code.
> The goal is that Rust code, since the beginning, has all wrapping
> operations marked explicitly as such.
Exactly. We have to not perpetuate the ambiguity of arithmetic
operations. It should be clear from the operator or the type what the
expected bounds are for a calculation.
-Kees
[1] https://lore.kernel.org/lkml/[email protected]/
--
Kees Cook
On Thu, Apr 11, 2024 at 04:08:01PM -0700, Boqun Feng wrote:
> Currently since Rust code is compiled with "-Coverflow-checks=y", so a
> normal substraction may be compiled as an overflow checking and panic
> if overflow happens:
>
> subq %rsi, %rdi
> jo .LBB0_2
> movq %rdi, %rax
> retq
> .LBB0_2:
> pushq %rax
> leaq str.0(%rip), %rdi
> leaq .L__unnamed_1(%rip), %rdx
> movl $33, %esi
> callq *core::panicking::panic::h59297120e85ea178@GOTPCREL(%rip)
>
> although overflow detection is nice to have, however this makes
> `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> clear that the overflow checking is helpful, since for example, the
> current binder usage[1] doesn't have the checking.
>
Ping. Thomas, John and Stepthen. Could you take a look at this, and the
discussion between Miguel and me? The key question is the behavior when
ktime_sub() hits a overflow, I think. Thanks!
(Cc Kees as well)
Regards,
Boqun
> Therefore make `Ktime::sub()` have the same semantics as `ktime_sub()`:
> overflow behaves like 2s-complement wrapping sub.
>
> Link: https://lore.kernel.org/lkml/5ac8c0d09392290be789423f0dd78a520b830fab.1682333709.git.zhangchuang3@xiaomi.com/ [1]
> Signed-off-by: Boqun Feng <[email protected]>
> ---
> rust/kernel/time.rs | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index e3bb5e89f88d..3cb15d3079f4 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -77,7 +77,9 @@ impl core::ops::Sub for Ktime {
> #[inline]
> fn sub(self, other: Ktime) -> Ktime {
> Self {
> - inner: self.inner - other.inner,
> + // Mirrors `ktime_sub()`, kernel defines signed overflow to behave like 2s-complement,
> + // hence `wrapping_sub()` is used.
> + inner: self.inner.wrapping_sub(other.inner),
> }
> }
> }
> --
> 2.44.0
>
On Tue, Apr 23, 2024 at 02:11:22PM -0700, Boqun Feng wrote:
> On Thu, Apr 11, 2024 at 04:08:01PM -0700, Boqun Feng wrote:
> > Currently since Rust code is compiled with "-Coverflow-checks=y", so a
> > normal substraction may be compiled as an overflow checking and panic
> > if overflow happens:
> >
> > subq %rsi, %rdi
> > jo .LBB0_2
> > movq %rdi, %rax
> > retq
> > .LBB0_2:
> > pushq %rax
> > leaq str.0(%rip), %rdi
> > leaq .L__unnamed_1(%rip), %rdx
> > movl $33, %esi
> > callq *core::panicking::panic::h59297120e85ea178@GOTPCREL(%rip)
> >
> > although overflow detection is nice to have, however this makes
> > `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> > clear that the overflow checking is helpful, since for example, the
> > current binder usage[1] doesn't have the checking.
> >
>
> Ping. Thomas, John and Stepthen. Could you take a look at this, and the
> discussion between Miguel and me? The key question is the behavior when
> ktime_sub() hits a overflow, I think. Thanks!
>
> (Cc Kees as well)
While working on the signed (and unsigned) integer overflow sanitizer
support on the C side for the kernel, I've also run into timekeeping
being a questionable area[1]. I *think* from what I can tell, it's always
expected to have wrapping behavior.
Can we define the type itself to be wrapping? (This has been my plan on
the C side, but we're still waiting on a finalized implementation of the
"wraps" attribute.[2])
-Kees
[1] This is strictly WIP, as I think fixing the _types_ is going to be
the more sustainable solution, but you can see some of what I was
poking at:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=devel/overflow/enable-unsigned-sanitizer&id=284464817a59b14f00d397bfbf1bf05683ed2f58
[2] https://github.com/llvm/llvm-project/pull/86618
--
Kees Cook
On Wed, Apr 24, 2024 at 1:37 AM Kees Cook <[email protected]> wrote:
>
> While working on the signed (and unsigned) integer overflow sanitizer
> support on the C side for the kernel, I've also run into timekeeping
> being a questionable area[1]. I *think* from what I can tell, it's always
> expected to have wrapping behavior.
Thanks, that is useful. In that branch you link, since it is about
unsigned, I imagine you could have hit issues with
`ktime_add_unsafe()` -- after all, callers are expecting overflow
there. But there is a single user (which is `ktime_add_safe()`), and I
don't see that one annotated in your branch.
In any case, if `ktime_add()` is supposed to always be wrapping like
the other functions in the area as you mention, then I think
`ktime_add_unsafe()` (i.e. wrapping one) should be renamed into
`ktime_add()` and the existing one removed. And then we can discuss
whether to do (or not) that in Rust too (see below).
However, if the `ktime_add()` / `ktime_add_unsafe()` /
`ktime_add_safe()` split is there for a good reason (see [1] -- sorry,
you were not in Cc in that particular one), and callers are already
expected to respect it, then I think we should document it in the C
side better and we should just start with that approach in the Rust
side too.
> Can we define the type itself to be wrapping? (This has been my plan on
> the C side, but we're still waiting on a finalized implementation of the
> "wraps" attribute.[2])
Yeah, we can make that the "default", so to speak (i.e. what the
operators will do). But we can also have different methods with
different expectations too if needed (i.e. the usual "access" vs.
"type" discussion).
And given the different variations that exist in C (see [1]), it
seemed to me that `ktime_t` operations there may not be expected to
actually wrap, and thus that would be an argument for trying to be
explicit in Rust.
So for the Rust side, what we need here is the expectation of how
`ktime_t` is supposed to be used. Or, even better, how one would
ideally design `ktime_t` today if there were no worry about callers,
because we have the chance to improve here over the C side before we
have those callers.
If the answer is "everyone assumes those to be wrapping, and there are
almost no use cases where the callers know it is not supposed to wrap,
and we don't care about whether they wrap" etc., then yeah, we should
go with wrapping default semantics and accept that we will not gain
that information and thus not catch mistakes easily in the future.
But if the answer is "we would have liked that `ktime_t` was more
explicit, and that is why we have the `ktime_add{,_safe,_unsafe}()`
variations, but nobody uses them because everybody assumes wrapping"
or "we actually assume no wrapping in `ktime_t`, which is why
`ktime_add_*()` exist", then I think we should definitely try to be
explicit on the Rust side from the onset so that we can catch more
bugs in the future.
(I know you know all this, but I was trying to summarize and clarify
the discussion :)
[1] https://lore.kernel.org/rust-for-linux/CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com/
Cheers,
Miguel
Alice Ryhl <[email protected]> writes:
> On Fri, Apr 12, 2024 at 3:18 PM Boqun Feng <[email protected]> wrote:
>>
>> On Fri, Apr 12, 2024 at 10:36:05AM +0200, Alice Ryhl wrote:
>> > On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]> wrote:
>> > >
>> > > Currently since Rust code is compiled with "-Coverflow-checks=y", so a
>> > > normal substraction may be compiled as an overflow checking and panic
>> > > if overflow happens:
>> > >
>> > > subq %rsi, %rdi
>> > > jo .LBB0_2
>> > > movq %rdi, %rax
>> > > retq
>> > > .LBB0_2:
>> > > pushq %rax
>> > > leaq str.0(%rip), %rdi
>> > > leaq .L__unnamed_1(%rip), %rdx
>> > > movl $33, %esi
>> > > callq *core::panicking::panic::h59297120e85ea178@GOTPCREL(%rip)
>> > >
>> > > although overflow detection is nice to have, however this makes
>> > > `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
>> > > clear that the overflow checking is helpful, since for example, the
>> > > current binder usage[1] doesn't have the checking.
>> >
>> > I don't think this is a good idea at all. Any code that triggers an
>> > overflow in Ktime::sub is wrong, and anyone who enables
>> > CONFIG_RUST_OVERFLOW_CHECKS does so because they want such bugs to be
>> > caught. You may have been able to find one example of a subtraction
>> > that doesn't have a risk of overflow, but overflow bugs really do
>>
>> The point is you won't panic the kernel because of an overflow. I
>> agree that overflow is something we want to catch, but currently
>> ktime_t doesn't panic if overflow happens.
>
> What the CONFIG_RUST_OVERFLOW_CHECKS option does is enable panics on
> overflow. So I don't understand how "it panics on overflow" is an
> argument for removing the overflow check. That's what you asked for!
> One could perhaps argue about whether CONFIG_RUST_OVERFLOW_CHECKS is a
> good idea (I think it is), but that is orthogonal. When
> CONFIG_RUST_OVERFLOW_CHECKS is enabled, you should respect the flag.
I would agree. If users do not want panics on overflow, they disable
RUST_OVERFLOW_CHECKS. If the config is enabled, overflows in ktime sub
should panic, even if it does not do so in equivalent C code.
BR Andreas
On Thu, Apr 25, 2024 at 11:00:31AM +0200, Andreas Hindborg wrote:
> Alice Ryhl <[email protected]> writes:
>
> > On Fri, Apr 12, 2024 at 3:18 PM Boqun Feng <[email protected]> wrote:
> >>
> >> On Fri, Apr 12, 2024 at 10:36:05AM +0200, Alice Ryhl wrote:
> >> > On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <[email protected]> wrote:
> >> > >
> >> > > Currently since Rust code is compiled with "-Coverflow-checks=y", so a
> >> > > normal substraction may be compiled as an overflow checking and panic
> >> > > if overflow happens:
> >> > >
> >> > > subq %rsi, %rdi
> >> > > jo .LBB0_2
> >> > > movq %rdi, %rax
> >> > > retq
> >> > > .LBB0_2:
> >> > > pushq %rax
> >> > > leaq str.0(%rip), %rdi
> >> > > leaq .L__unnamed_1(%rip), %rdx
> >> > > movl $33, %esi
> >> > > callq *core::panicking::panic::h59297120e85ea178@GOTPCREL(%rip)
> >> > >
> >> > > although overflow detection is nice to have, however this makes
> >> > > `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> >> > > clear that the overflow checking is helpful, since for example, the
> >> > > current binder usage[1] doesn't have the checking.
> >> >
> >> > I don't think this is a good idea at all. Any code that triggers an
> >> > overflow in Ktime::sub is wrong, and anyone who enables
> >> > CONFIG_RUST_OVERFLOW_CHECKS does so because they want such bugs to be
> >> > caught. You may have been able to find one example of a subtraction
> >> > that doesn't have a risk of overflow, but overflow bugs really do
> >>
> >> The point is you won't panic the kernel because of an overflow. I
> >> agree that overflow is something we want to catch, but currently
> >> ktime_t doesn't panic if overflow happens.
> >
> > What the CONFIG_RUST_OVERFLOW_CHECKS option does is enable panics on
> > overflow. So I don't understand how "it panics on overflow" is an
> > argument for removing the overflow check. That's what you asked for!
> > One could perhaps argue about whether CONFIG_RUST_OVERFLOW_CHECKS is a
> > good idea (I think it is), but that is orthogonal. When
> > CONFIG_RUST_OVERFLOW_CHECKS is enabled, you should respect the flag.
>
> I would agree. If users do not want panics on overflow, they disable
> RUST_OVERFLOW_CHECKS. If the config is enabled, overflows in ktime sub
> should panic, even if it does not do so in equivalent C code.
>
What's reason then? If we think a piece of API should have a different
semantics than its C counterpart, that suggests we also need to change
the C side for the same reason. Don't keep good stuffs only to ourselves
;-)
Plus, what if users don't want to panic on ktime API but still want
overflow checks for other parts? Using RUST_OVERFLOW_CHECKS to determine
whether ktime should perform overflow checkings seems a bad to me
regardless of what semantics we end up with. For reference, the Rust std
`Duration` performs always overflow checking:
https://doc.rust-lang.org/src/std/time.rs.html#429-435
We shouldn't define that overflow checking of ktime follows the general
rule of overflow checking of any i64, instead, we should have a clear
rule for it.
Regards,
Boqun
> BR Andreas
The following commit has been merged into the timers/core branch of tip:
Commit-ID: ddd9120983c3efbcaa3a4c7777da1440f8ce27d8
Gitweb: https://git.kernel.org/tip/ddd9120983c3efbcaa3a4c7777da1440f8ce27d8
Author: Boqun Feng <[email protected]>
AuthorDate: Thu, 11 Apr 2024 16:08:00 -07:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 01 May 2024 00:04:47 +02:00
rust: time: doc: Add missing C header links
The definitions related to jiffies are at linux/jiffies.h, and the
definitions related to ktime_t are at linux/ktime.h, since
`kernel::time` provides the functionality dealing with jiffies and
ktime_t, it makes sense to add links to them from Rust's time module.
Signed-off-by: Boqun Feng <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
Acked-by: Miguel Ojeda <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
rust/kernel/time.rs | 3 +++
1 file changed, 3 insertions(+)
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 6811d5c..e3bb5e8 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -4,6 +4,9 @@
//!
//! This module contains the kernel APIs related to time and timers that
//! have been ported or wrapped for usage by Rust code in the kernel.
+//!
+//! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
+//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
/// The number of nanoseconds per millisecond.
pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;