MIME-Version: 1.0
In-Reply-To: <69b38985-e094-ddcc-6f7e-d6e5cc2c657e@akamai.com>
References: <CA+55aFznorYJd8OyV09eHt4zrRrrv2aEq_D=Q8k2w+CmgJ=+cw@mail.gmail.com>
 <69b38985-e094-ddcc-6f7e-d6e5cc2c657e@akamai.com>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 7 Sep 2017 11:43:06 -0700
Message-ID: <CA+55aFxvD1R0xg8ZEjNyHtvhBZchWBJTU=FQNiY6Mv8E4TqZ=Q@mail.gmail.com>
Subject: Re: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove
 the restore_c_regs_and_iret label)
To: Vishwanath Pai <vpai@akamai.com>
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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1408
Lines: 44

On Thu, Sep 7, 2017 at 11:25 AM, Vishwanath Pai <vpai@akamai.com> wrote:
> On 09/07/2017 01:51 PM, Linus Torvalds wrote:
>>
>> But honestly, that math is odd in other ways too (is that "r-1"
>> _supposed_ to underflow to -1 for large 'user' counts?), so somebody
>> needs to look at that logic.
>
> Sorry about the build failure, we have already queued up a fix for this:
> https://patchwork.ozlabs.org/patch/810772/

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