Subject: Re: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove
 the restore_c_regs_and_iret label)
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>, Igor Lubashev <ilubashe@akamai.com>,
        Josh Hunt <johunt@akamai.com>, Pablo Neira Ayuso <pablo@netfilter.org>,
        Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>,
        the arch/x86 maintainers <x86@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Brian Gerst <brgerst@gmail.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        Juergen Gross <jgross@suse.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Kees Cook <keescook@chromium.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        "David S. Miller" <davem@davemloft.net>, Arnd Bergmann <arnd@arndb.de>
References: <CA+55aFznorYJd8OyV09eHt4zrRrrv2aEq_D=Q8k2w+CmgJ=+cw@mail.gmail.com>
 <69b38985-e094-ddcc-6f7e-d6e5cc2c657e@akamai.com>
 <CA+55aFxvD1R0xg8ZEjNyHtvhBZchWBJTU=FQNiY6Mv8E4TqZ=Q@mail.gmail.com>
From: Vishwanath Pai <vpai@akamai.com>
Message-ID: <6667f710-68f3-b97e-b0eb-d9879476831e@akamai.com>
Date: Thu, 7 Sep 2017 16:16:47 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.1
MIME-Version: 1.0
In-Reply-To: <CA+55aFxvD1R0xg8ZEjNyHtvhBZchWBJTU=FQNiY6Mv8E4TqZ=Q@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2015
Lines: 65

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