linux-kernel - Re: [PATCH 06/15] timers: Update function descriptions of sleep/delay related functions

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]

Message-ID: <877cbpq3t7.fsf@somnus>
Date: Thu, 05 Sep 2024 21:49:08 +0200
From: Anna-Maria Behnsen <anna-maria@...utronix.de>
To: Thomas Gleixner <tglx@...utronix.de>, Frederic Weisbecker
 <frederic@...nel.org>, Jonathan Corbet <corbet@....net>
Cc: linux-kernel@...r.kernel.org, Len Brown <len.brown@...el.com>, "Rafael
 J. Wysocki" <rafael@...nel.org>, Arnd Bergmann <arnd@...db.de>,
 linux-arch@...r.kernel.org, Andrew Morton <akpm@...uxfoundation.org>,
 Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers
 <ndesaulniers@...gle.com>
Subject: Re: [PATCH 06/15] timers: Update function descriptions of
 sleep/delay related functions

Thomas Gleixner <tglx@...utronix.de> writes:

> On Thu, Sep 05 2024 at 08:59, Thomas Gleixner wrote:
>> On Wed, Sep 04 2024 at 15:04, Anna-Maria Behnsen wrote:
>> However, instead of proliferating this voodoo can we please convert it
>> into something comprehensible?
>>
>> /*
>>  * The microseconds delay multiplicator is used to convert a constant
>>  * microseconds value to a <INSERT COHERENT EXPLANATION>.
>>  */
>> #define UDELAY_CONST_MULT  ((unsigned long)DIV_ROUND_UP(1ULL << 32, USEC_PER_SEC))
>>
>> /*
>>  * The maximum constant udelay value picked out of thin air
>>  * to avoid <INSERT COHERENT EXPLANATION>.
>>  */
>> #define UDELAY_CONST_MAX   20000
>>
>> /**
>>  * udelay - .....
>>  */
>> static __always_inline void udelay(unsigned long usec)
>> {
>>         /*
>> 	 * <INSERT COHERENT EXPLANATION> for this construct
>>          */
>> 	if (__builtin_constant_p(usec)) {
>> 		if (usec >= UDELAY_CONST_MAX)
>> 			__bad_udelay();
>> 		else
>> 			__const_udelay(usec * UDELAY_CONST_MULT);
>> 	} else {
>> 		__udelay(usec);
>
> And of course a these magic numeric constants have been copied all over
> the place. git grep '__const_udelay(' arch/ .... Just SH managed to use
> 0x10c6 instead of 0x10c7. 
>
> ARM has it's own udelay implementation:
>
> #define udelay(n)							\
> 	(__builtin_constant_p(n) ?					\
> 	  ((n) > (MAX_UDELAY_MS * 1000) ? __bad_udelay() :		\
> 			__const_udelay((n) * UDELAY_MULT)) :		\
> 	  __udelay(n))
>
> Amazingly this uses the same comparison construct which was in the
> generic udelay implementation... Same for arc, m68k and microblaze.
>
> Plus the default implementation for mdelay() in linux/delay.h:
>
> #define mdelay(n) (\
> 	(__builtin_constant_p(n) && (n)<=MAX_UDELAY_MS) ? udelay((n)*1000) : \
> 	({unsigned long __ms=(n); while (__ms--) udelay(1000);}))
>
> Oh well....
>
> What's truly amazing is that all __udelay() implementations, which
> invoke __const_udelay() under the hood, do:
>
>        __const_udelay(usec * 0x10c7);
>
> So we have an arbitrary range limit for constants, which makes the build
> fail. But the variable based udelays can hand in whatever they want and
> __udelay() happily ignores it including the possible multiplication
> overflow.
>
> That's all really consistently copy and pasted voodoo. The other
> architecture implementations are not much better in that regard.  The
> main difference is their cutoff value for __const_udelay() and the
> multiplication factors.
>
> The below uncompiled and untested pile is an attempt to consolidate this
> mess as far as it goes. There is probably more to mop up, but for a
> start this makes already sense.

Thanks for the first step of dissection of the mess! I'll take a closer
look at it soon.

But as it's in tree since some more days than just one, can we please
make this cleanup on top of the original queue and get the fsleep() and
outdated documentation thing fixed soon? You made a proposal in the
previous answer to convert it into something comprehensible. If there
are no concerns, I would integrate it and prepare a v2 for the queue.

Thanks,

	Anna-Maria