Received-SPF: pass (google.com: domain of linux-kernel+bounces-202737-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Phil Chang <phil.chang@mediatek.com>
To: <anna-maria@linutronix.de>
CC: <alix.wu@mediatek.com>, <angelogioacchino.delregno@collabora.com>,
	<frederic@kernel.org>, <jy.ho@mediatek.com>,
	<linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<matthias.bgg@gmail.com>, <phil.chang@mediatek.com>, <tglx@linutronix.de>
Subject: Re: [PATCH] hrtimer: check hrtimer with a NULL function
Date: Wed, 5 Jun 2024 21:51:06 +0800
Message-ID: <20240605135106.16065-1-phil.chang@mediatek.com>
In-Reply-To: <87plsvvgpn.fsf@somnus>
References: <87plsvvgpn.fsf@somnus>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

>> 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