Re: [PATCH] hrtimer: check hrtimer with a NULL function

Phil Chang
2024-06-05T13:51:15Z

>> 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 <phil.chang@mediatek.com>
>> ---
>> 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