linux-kernel - Re: [PATCH resend] fs/namei.c: micro-optimize acl_permission

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: <CAHk-=widT2tV+sVPzNQWijtUz4JA=CS=EaJRfC3_9ymuQXQS8Q@mail.gmail.com>
Date:   Sun, 7 Jun 2020 12:48:53 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH resend] fs/namei.c: micro-optimize acl_permission_check

On Sun, Jun 7, 2020 at 9:37 AM Linus Torvalds
<torvalds@...ux-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.

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

Al, any reaction?

               Linus

View attachment "0001-vfs-do-not-do-group-lookup-when-not-necessary.patch" of type "text/x-patch" (3726 bytes)

View attachment "0002-vfs-clean-up-posix_acl_permission-logic-aroudn-MAY_N.patch" of type "text/x-patch" (2119 bytes)