linux-kernel - Re: [PATCH v2 2/2] iopoll: Do not use timekeeping in read_poll_timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 10 May 2023 15:35:32 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Geert Uytterhoeven" <geert+renesas@...der.be>,
        "Stephen Boyd" <sboyd@...nel.org>,
        "Tomasz Figa" <tomasz.figa@...il.com>,
        "Sylwester Nawrocki" <s.nawrocki@...sung.com>,
        "Will Deacon" <will@...nel.org>,
        "Wolfram Sang" <wsa+renesas@...g-engineering.com>,
        "Dejin Zheng" <zhengdejin5@...il.com>,
        "Kai-Heng Feng" <kai.heng.feng@...onical.com>,
        "Nicholas Piggin" <npiggin@...il.com>,
        "Heiko Carstens" <hca@...ux.ibm.com>,
        "Peter Zijlstra" <peterz@...radead.org>,
        "Russell King" <linux@...linux.org.uk>,
        "John Stultz" <jstultz@...gle.com>,
        "Thomas Gleixner" <tglx@...utronix.de>,
        "Tony Lindgren" <tony@...mide.com>,
        "Krzysztof Kozlowski" <krzk@...nel.org>,
        "Tero Kristo" <tero.kristo@...ux.intel.com>,
        "Ulf Hansson" <ulf.hansson@...aro.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        "Vincent Guittot" <vincent.guittot@...aro.org>
Cc:     linux-arm-kernel@...ts.infradead.org,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iopoll: Do not use timekeeping in
 read_poll_timeout_atomic()

On Wed, May 10, 2023, at 15:23, Geert Uytterhoeven wrote:
> read_poll_timeout_atomic() uses ktime_get() to implement the timeout
> feature, just like its non-atomic counterpart.  However, there are
> several issues with this, due to its use in atomic contexts:
>
>   1. When called in the s2ram path (as typically done by clock or PM
>      domain drivers), timekeeping may be suspended, triggering the
>      WARN_ON(timekeeping_suspended) in ktime_get():
>
> 	WARNING: CPU: 0 PID: 654 at kernel/time/timekeeping.c:843 ktime_get+0x28/0x78
>
>      Calling ktime_get_mono_fast_ns() instead of ktime_get() would get
>      rid of that warning.  However, that would break timeout handling,
>      as (at least on systems with an ARM architectured timer), the time
>      returned by ktime_get_mono_fast_ns() does not advance while
>      timekeeping is suspended.
>      Interestingly, (on the same ARM systems) the time returned by
>      ktime_get() does advance while timekeeping is suspended, despite
>      the warning.
>
>   2. Depending on the actual clock source, and especially before a
>      high-resolution clocksource (e.g. the ARM architectured timer)
>      becomes available, time may not advance in atomic contexts, thus
>      breaking timeout handling.
>
> Fix this by abandoning the idea that one can rely on timekeeping to
> implement timeout handling in all atomic contexts, and switch from a
> global time-based to a locally-estimated timeout handling.  In most
> (all?) cases the timeout condition is exceptional and an error
> condition, hence any additional delays due to underestimating wall clock
> time are irrelevant.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>

This looks reasonable to me,

Acked-by: Arnd Bergmann <arnd@...db.de>

I assume you sent this because you ran into the bug on a
particular driver. It might help to be more specific about
how this can be reproduced.

> ---
> Alternatively, one could use a mixed approach (use both
> ktime_get_mono_fast_ns() and a local (under)estimate, and timeout on the
> earliest occasion), but I think that would complicate things without
> much gain.

Agreed.

     Arnd