linux-kernel - Re: [PATCH] x86/math64: handle #DE in mul_u64_u64_div

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: <0818676F-ED90-44B1-AB10-42DDB7F1B139@zytor.com>
Date: Tue, 22 Jul 2025 09:50:54 -0700
From: "H. Peter Anvin" <hpa@...or.com>
To: Oleg Nesterov <oleg@...hat.com>,
        David Laight <david.laight.linux@...il.com>
CC: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "Li,Rongqing" <lirongqing@...du.com>,
        Steven Rostedt <rostedt@...dmis.org>, linux-kernel@...r.kernel.org,
        x86@...nel.org
Subject: Re: [PATCH] x86/math64: handle #DE in mul_u64_u64_div_u64()

On July 22, 2025 3:50:35 AM PDT, Oleg Nesterov <oleg@...hat.com> wrote:
>On 07/21, David Laight wrote:
>>
>> On Mon, 21 Jul 2025 15:04:22 +0200
>> Oleg Nesterov <oleg@...hat.com> wrote:
>>
>> > Change mul_u64_u64_div_u64() to return ULONG_MAX if the result doesn't
>> > fit u64, this matches the generic implementation in lib/math/div64.c.
>>
>> Not quite, the generic version is likely to trap on divide by zero.
>
>I meant that the generic implementation returns -1ul too if the result
>doesn't fit into u64.
>
>> I think it would be better to always trap (eg BUG_ON(!div)).
>
>Well, I don't like adding a BUG_ON(), but OK.
>
>> The trouble there is that (an ignored) ~(u64)0 is likely to cause another
>> arithmetic overflow with even more consequences.
>>
>> So I'm not at all sure what it should look like or whether 0 is a better
>> error return (esp for div == 0).
>
>I'm not sure either but x86/generic versions should be consistent. Let's
>discuss this and possibly change both implementations later?
>
>> >  static inline u64 mul_u64_u64_div_u64(u64 a, u64 mul, u64 div)
>> >  {
>> > +	int ok = 0;
>> >  	u64 q;
>> >
>> > -	asm ("mulq %2; divq %3" : "=a" (q)
>> > -				: "a" (a), "rm" (mul), "rm" (div)
>> > -				: "rdx");
>> > +	asm ("mulq %3; 1: divq %4; movl $1,%1; 2:\n"
>>
>> The "movl $1,%1" is a 5 byte instruction.
>> Better to use either 'incl' or get the constraints right for 'movb'
>
>Agreed, thanks,
>
>> > +	if (ok)
>> > +		return q;
>> > +	WARN_ON_ONCE(!div);
>>
>> I think you need to WARN for overflow as well as divide by zero.
>
>The generic implementation doesn't WARN... OK, I won't argue.
>How about
>
>	static inline u64 mul_u64_u64_div_u64(u64 a, u64 mul, u64 div)
>	{
>		char ok = 0;
>		u64 q;
>
>		asm ("mulq %3; 1: divq %4; movb $1,%1; 2:\n"
>			_ASM_EXTABLE(1b, 2b)
>			: "=a" (q), "+r" (ok)
>			: "a" (a), "rm" (mul), "rm" (div)
>			: "rdx");
>
>		if (ok)
>			return q;
>		BUG_ON(!div);
>		WARN_ON_ONCE(1);
>		return ~(u64)0;
>	}
>
>?
>
>Oleg.
>

Maybe the generic version *should* warn?

As far as the ok output, the Right Way™ to do it is with an asm goto instead of a status variable; the second best tends to be to use the flags output.