Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH resend] fs/namei.c: micro-optimize acl_permission_check
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20200605142300.14591-1-linux@rasmusvillemoes.dk>
 <CAHk-=wgz68f2u7bFPZCWgbsbEJw+2HWTJFXSg_TguY+xJ8WrNw@mail.gmail.com>
 <dcd7516b-0a1f-320d-018d-f3990e771f37@rasmusvillemoes.dk>
 <CAHk-=wixdSUWFf6BoT7rJUVRmjUv+Lir_Rnh81xx7e2wnzgKbg@mail.gmail.com>
 <CAHk-=widT2tV+sVPzNQWijtUz4JA=CS=EaJRfC3_9ymuQXQS8Q@mail.gmail.com>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <934d7665-3358-576e-8434-82b16e3a1bf1@rasmusvillemoes.dk>
Date:   Mon, 8 Jun 2020 12:08:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <CAHk-=widT2tV+sVPzNQWijtUz4JA=CS=EaJRfC3_9ymuQXQS8Q@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
Precedence: bulk

On 07/06/2020 21.48, Linus Torvalds wrote:
> On Sun, Jun 7, 2020 at 9:37 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>>> That will kinda work, except you do that mask &= MAY_RWX before
>>> check_acl(), which cares about MAY_NOT_BLOCK and who knows what other bits.
>>
>> Good catch.
> 
> With the change to not clear the non-rwx bits in general, the owner
> case now wants to do the masking, and then the "shift left by 6"
> modification makes no sense since it only makes for a bigger constant
> (the only reason to do the shift-right was so that the bitwise not of
> the i_mode could be done in parallel with the shift, but with the
> masking that instruction scheduling optimization becomes kind of
> immaterial too). So I modified that patch to not bother, and add a
> comment about MAY_NOT_BLOCK.
> 
> And since I was looking at the MAY_NOT_BLOCK logic, it was not only
> not mentioned in comments, it also had some really confusing code
> around it.
> 
> The posix_acl_permission() looked like it tried to conserve that bit,
> which is completely wrong. It wasn't a bug only for the simple reason
> that the only two call-sites had either explicitly cleared the bit
> when calling, or had tested that the bit wasn't set in the first
> place.
> 
> So as a result, I wrote a second patch to clear that confusion up.
> 
> Rasmus, say the word and I'll mark you for authorship on the first one.

It might be a bit confusing with me mentioned in the third person and
then also author, and it's really mostly your patch, so reported-by is
fine with me. But it's up to you.

> Comments? Can you find something else wrong here, or some other fixup to do?

No, I think it's ok. I took a look at the disassembly and it looks fine.
There's an extra push/pop of %r14 [that's where gcc computes mode>>3,
then CSE allows it to do cmovne %r14d,%ebx after in_group_p), so the
owner case gets slightly penalized. I think/hope the savings from
avoiding the in_group_p should compensate for that - any absolute path
open() by non-root saves at least two in_group_p. YMMV.

Rasmus