Received-SPF: pass (google.com: domain of linux-kernel+bounces-204336-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Anna-Maria Behnsen <anna-maria@linutronix.de>
To: Phil Chang <phil.chang@mediatek.com>
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
In-Reply-To: <20240605135106.16065-1-phil.chang@mediatek.com>
References: <87plsvvgpn.fsf@somnus>
 <20240605135106.16065-1-phil.chang@mediatek.com>
Date: Thu, 06 Jun 2024 15:14:37 +0200
Message-ID: <877cf2i56q.fsf@somnus>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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