Since user is u64, it is best to have a predictable return value for all
possible values of user. So maybe:
static u64 user2rate_bytes(u64 user)
{
u64 r;
r = user ? U32_MAX / (u32) min(user, U32_MAX) : U32_MAX;
r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT;
return r;
}
-----Original Message-----
From: Vishwanath Pai [mailto:[email protected]]
Sent: Thursday, September 07, 2017 4:17 PM
To: Linus Torvalds <[email protected]>
Cc: Ingo Molnar <[email protected]>; Lubashev, Igor <[email protected]>; Hunt,
Joshua <[email protected]>; Pablo Neira Ayuso <[email protected]>; Borislav
Petkov <[email protected]>; Andy Lutomirski <[email protected]>; the arch/x86
maintainers <[email protected]>; Linux Kernel Mailing List
<[email protected]>; Brian Gerst <[email protected]>; Andrew Cooper
<[email protected]>; Juergen Gross <[email protected]>; Boris Ostrovsky
<[email protected]>; Kees Cook <[email protected]>; Andrew Morton
<[email protected]>; David S. Miller <[email protected]>; Arnd
Bergmann <[email protected]>
Subject: Re: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove
the restore_c_regs_and_iret label)
On 09/07/2017 02:43 PM, Linus Torvalds wrote:
> Note: that patch has *exactly* the issue I was talking about above.
>
> Doing that
>
> if (user > 0xFFFFFFFFULL)
> return 0;
>
> is different from the old code, which used to result in a zero in the
> divide, and then
>
> r = (r - 1) << 4;
>
> would cause it to return a large value.
>
> So the patch in question doesn't just fix the build error, it
> completely changes the semantics of the function too.
>
> I *think* the new behavior is likely what you want, but these kinds of
> things should be _described_.
>
> Also, even with the patch, we have garbage:
>
> 0xFFFFFFFFULL / (u32)user
>
> why is that sub-expression pointlessly doing a 64-bit divide with a
> 32-bit number? The compiler is hopefully smart enough to point things
> out, but that "ULL" really is _wrong_ there, and could cause a stupid
> compiler to still do a 64-bit divide (although hopefully the simpler
> version that is 64/32).
>
> So please clarify both the correct behavior _and_ the actual typing of
> the divide, ok?
>
> Linus
The value of 'user' is sent from userspace, which is the return value of this
function:
static uint64_t bytes_to_cost(uint32_t bytes) {
uint32_t r = bytes >> XT_HASHLIMIT_BYTE_SHIFT;
return UINT32_MAX / (r+1);
}
What user2rate_bytes() is trying to do is the opposite of above. The size of
'user' is 64bit for a different reason altogether, but in this case it is
guaranteed to be always < U32_MAX. And hence using 64bit divide is completely
pointless (which I now realize).
Writing U32INT_MAX as 0xFFFFFFFFULL was a mistake on my part. I could have
avoided all of this by using built-in constants instead of trying to define
them myself. I will rewrite the function as below and send out another patch:
static u64 user2rate_bytes(u64 user)
{
u64 r;
r = user ? U32_MAX / (u32) user : U32_MAX;
r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT;
return r;
}
-Vishwanath
