simillar with timers, check for timer->function == NULL.
If the pointer is NULL, discard the request silently.
Signed-off-by: Phil Chang <[email protected]>
---
kernel/time/hrtimer.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 492c14aac642..72d6e7bc9cd9 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1297,9 +1297,13 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
base = lock_hrtimer_base(timer, &flags);
+ if (!timer->function)
+ goto out;
+
if (__hrtimer_start_range_ns(timer, tim, delta_ns, mode, base))
hrtimer_reprogram(timer, true);
+out:
unlock_hrtimer_base(timer, &flags);
}
EXPORT_SYMBOL_GPL(hrtimer_start_range_ns);
@@ -1667,6 +1671,11 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
__remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
fn = timer->function;
+ if (WARN_ON_ONCE(!fn)) {
+ /* Should never happen. */
+ goto out;
+ }
+
/*
* Clear the 'is relative' flag for the TIME_LOW_RES case. If the
* timer is restarted with a period then it becomes an absolute
@@ -1710,6 +1719,7 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
* hrtimer_active() cannot observe base->running.timer == NULL &&
* timer->state == INACTIVE.
*/
+out:
raw_write_seqcount_barrier(&base->seq);
WARN_ON_ONCE(base->running != timer);
--
2.18.0
Hi,
Phil Chang <[email protected]> writes:
> simillar with timers, check for timer->function == NULL.
> If the pointer is NULL, discard the request silently.
Can you please explain, why this change is required?
The statement "similar to timers" is not a valid explaination as timer
list timers and hrtimers are two different things. The function pointer
for timer list timers is explicitly set to NULL in shutdown path to
prevent unwanted rearming of the timer. For hrtimers there is no
shutdown function implemented and function is never set to NULL by
hrtimer code.
> Signed-off-by: Phil Chang <[email protected]>
> ---
> kernel/time/hrtimer.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 492c14aac642..72d6e7bc9cd9 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1297,9 +1297,13 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
>
> base = lock_hrtimer_base(timer, &flags);
>
> + if (!timer->function)
> + goto out;
When this happens, user of hrtimers do not follow the semantics of
hrtimers which means this is a bug.
> +
> if (__hrtimer_start_range_ns(timer, tim, delta_ns, mode, base))
> hrtimer_reprogram(timer, true);
>
> +out:
> unlock_hrtimer_base(timer, &flags);
> }
> EXPORT_SYMBOL_GPL(hrtimer_start_range_ns);
> @@ -1667,6 +1671,11 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
> __remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
> fn = timer->function;
>
> + if (WARN_ON_ONCE(!fn)) {
> + /* Should never happen. */
...same as above...
> + goto out;
> + }
> +
> /*
> * Clear the 'is relative' flag for the TIME_LOW_RES case. If the
> * timer is restarted with a period then it becomes an absolute
> @@ -1710,6 +1719,7 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
> * hrtimer_active() cannot observe base->running.timer == NULL &&
> * timer->state == INACTIVE.
> */
> +out:
> raw_write_seqcount_barrier(&base->seq);
>
> WARN_ON_ONCE(base->running != timer);
Thanks,
Anna-Maria
>> simillar with timers, check for timer->function == NULL.
>> If the pointer is NULL, discard the request silently.
> Can you please explain, why this change is required?
> The statement "similar to timers" is not a valid explaination as timer
> list timers and hrtimers are two different things. The function pointer
> for timer list timers is explicitly set to NULL in shutdown path to
> prevent unwanted rearming of the timer. For hrtimers there is no
> shutdown function implemented and function is never set to NULL by
> hrtimer code.
>
The timer->function is provided by caller, which is invaild if fuction is NULL,
and currently, the hrtime code does not perform any checks to validate this.
Passing a NULL function can lead to a system panic, with a backtrace likes:
```
__hrtimer_run_queues+0x1d8/0x3b8
hrtimer_interrupt+0xdc/0x3a0
arch_timer_handler_phys+0x54/0x94
handle_percpu_devid_irq+0xb8/0x308
handle_domain_irq+0x78/0xec
gic_handle_irq+0x50/0x10c
call_on_irq_stack+0x38/0x54
do_interrupt_handler+0x40/0x98
```
This backtrace does not clearly indicate the source of the invalid usage of hrtimer.
>> Signed-off-by: Phil Chang <[email protected]>
>> ---
>> kernel/time/hrtimer.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
>> index 492c14aac642..72d6e7bc9cd9 100644
>> --- a/kernel/time/hrtimer.c
>> +++ b/kernel/time/hrtimer.c
>> @@ -1297,9 +1297,13 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
>>
>> base = lock_hrtimer_base(timer, &flags);
>>
>> + if (!timer->function)
>> + goto out;
>
> When this happens, user of hrtimers do not follow the semantics of
> hrtimers which means this is a bug.
>
Agree, how about BUG_ON here ?
>> +
>> if (__hrtimer_start_range_ns(timer, tim, delta_ns, mode, base))
>> hrtimer_reprogram(timer, true);
>>
>> +out:
>> unlock_hrtimer_base(timer, &flags);
>> }
>> EXPORT_SYMBOL_GPL(hrtimer_start_range_ns);
>> @@ -1667,6 +1671,11 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
>> __remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
>> fn = timer->function;
>>
>> + if (WARN_ON_ONCE(!fn)) {
>> + /* Should never happen. */
>
> ...same as above...
>
>> + goto out;
>> + }
>> +
>> /*
>> * Clear the 'is relative' flag for the TIME_LOW_RES case. If the
>> * timer is restarted with a period then it becomes an absolute
>> @@ -1710,6 +1719,7 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
>> * hrtimer_active() cannot observe base->running.timer == NULL &&
>> * timer->state == INACTIVE.
>> */
>> +out:
>> raw_write_seqcount_barrier(&base->seq);
>>
>> WARN_ON_ONCE(base->running != timer);
>
>
> Thanks,
>
> Anna-Maria
Thank You
Phil Chang <[email protected]> writes:
>>> simillar with timers, check for timer->function == NULL.
>>> If the pointer is NULL, discard the request silently.
>
>> Can you please explain, why this change is required?
>
>> The statement "similar to timers" is not a valid explaination as timer
>> list timers and hrtimers are two different things. The function pointer
>> for timer list timers is explicitly set to NULL in shutdown path to
>> prevent unwanted rearming of the timer. For hrtimers there is no
>> shutdown function implemented and function is never set to NULL by
>> hrtimer code.
>>
> The timer->function is provided by caller, which is invaild if fuction is NULL,
> and currently, the hrtime code does not perform any checks to validate this.
> Passing a NULL function can lead to a system panic, with a backtrace likes:
> ```
> __hrtimer_run_queues+0x1d8/0x3b8
> hrtimer_interrupt+0xdc/0x3a0
> arch_timer_handler_phys+0x54/0x94
> handle_percpu_devid_irq+0xb8/0x308
> handle_domain_irq+0x78/0xec
> gic_handle_irq+0x50/0x10c
> call_on_irq_stack+0x38/0x54
> do_interrupt_handler+0x40/0x98
> ```
> This backtrace does not clearly indicate the source of the invalid usage of hrtimer.
To make it more clear to the inexperienced hrtimer user that it is
mandatory to initialize timer->function to be able to use hrtimers, but
to prevent the kernel to crash, please add a warning in
hrtimer_start_range_ns(). The check can be done without locking the
hrtimer base. When the function pointer is not set, return directly
without doing anything. So this above mentioned backtrace will no longer
appear, as the timer is not added to a queue.
Thanks,
Anna-Maria
Since hrtimers do not allow a NULL function to be passed,
to prevent a kernel crash, return before adding the timer to a queue.
Signed-off-by: Phil Chang <[email protected]>
---
kernel/time/hrtimer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 492c14aac642..d32c1afe59b3 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1285,6 +1285,9 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
struct hrtimer_clock_base *base;
unsigned long flags;
+ if (WARN_ON(!timer->function))
+ return;
+
/*
* Check whether the HRTIMER_MODE_SOFT bit and hrtimer.is_soft
* match on CONFIG_PREEMPT_RT = n. With PREEMPT_RT check the hard
--
2.18.0
Phil Chang <[email protected]> writes:
> Since hrtimers do not allow a NULL function to be passed,
> to prevent a kernel crash, return before adding the timer to a queue.
You could point out in the commit message, where the kernel would crash
when using a hrtimer with a NULL pointer as function pointer. And it
shouldn't be a problem to use more than a single sentence for a commit
message :)
> Signed-off-by: Phil Chang <[email protected]>
> ---
> kernel/time/hrtimer.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 492c14aac642..d32c1afe59b3 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1285,6 +1285,9 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
> struct hrtimer_clock_base *base;
> unsigned long flags;
>
> + if (WARN_ON(!timer->function))
> + return;
> +
Please use a WARN_ON_ONCE() (as documented in
Documentation/process/coding-style.rst)
> /*
> * Check whether the HRTIMER_MODE_SOFT bit and hrtimer.is_soft
> * match on CONFIG_PREEMPT_RT = n. With PREEMPT_RT check the hard
Thanks,
Anna-Maria
To prevent improper usage of hrtimers and avoid potential kernel crashes,
this commit introduces a validation check for hrtimers with a valid function callback,
discard the hrtimers that have a NULL callback.
The `run_hrtimer` executes callbacks for every hrtimer,
and these callbacks must not be NULL. A NULL callback can lead to a kernel crash.
This update ensures that all hrtimers have properly initialized callbacks
before execution.
Signed-off-by: Phil Chang <[email protected]>
---
kernel/time/hrtimer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 492c14aac642..b8ee320208d4 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1285,6 +1285,8 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
struct hrtimer_clock_base *base;
unsigned long flags;
+ if (WARN_ON_ONCE(!timer->function))
+ return;
/*
* Check whether the HRTIMER_MODE_SOFT bit and hrtimer.is_soft
* match on CONFIG_PREEMPT_RT = n. With PREEMPT_RT check the hard
--
2.18.0