netdev - Re: [PATCH v3 1/2] timer: add a function to adjust timeouts to be upper bound

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, 30 Mar 2022 15:40:55 +0200 (CEST)
From:   Anna-Maria Behnsen <anna-maria@...utronix.de>
To:     Artem Savkov <asavkov@...hat.com>
cc:     netdev@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Josh Poimboeuf <jpoimboe@...hat.com>, davem@...emloft.net,
        yoshfuji@...ux-ipv6.org, dsahern@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] timer: add a function to adjust timeouts to be
 upper bound

On Wed, 30 Mar 2022, Artem Savkov wrote:

> Current timer wheel implementation is optimized for performance and
> energy usage but lacks in precision. This, normally, is not a problem as
> most timers that use timer wheel are used for timeouts and thus rarely
> expire, instead they often get canceled or modified before expiration.
> Even when they don't, expiring a bit late is not an issue for timeout
> timers.
> 
> TCP keepalive timer is a special case, it's aim is to prevent timeouts,
> so triggering earlier rather than later is desired behavior. In a
> reported case the user had a 3600s keepalive timer for preventing firewall
> disconnects (on a 3650s interval). They observed keepalive timers coming
> in up to four minutes late, causing unexpected disconnects.
> 
> This commit adds upper_bound_timeout() function that takes a relative
> timeout and adjusts it based on timer wheel granularity so that supplied
> value effectively becomes an upper bound for the timer.
> 

I think there is a problem with this approach. Please correct me, if I'm
wrong. The timer wheel index and level calculation depends on
timer_base::clk. The timeout/delta which is used for this calculation is
relative to timer_base::clk (delta = expires - base::clk). timer_base::clk
is not updated in sync with jiffies. It is forwarded before a new timer is
queued. It is possible, that timer_base::clk is behind jiffies after
forwarding because of a not yet expired timer.

When calculating the level/index with a relative timeout, there is no
guarantee that the result is the same when actual enqueueing the timer with
expiry = jiffies + timeout .

Thanks,

	Anna-Maria