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-=wgz68f2u7bFPZCWgbsbEJw+2HWTJFXSg_TguY+xJ8WrNw@mail.gmail.com>
Date:   Fri, 5 Jun 2020 13:18:21 -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 Fri, Jun 5, 2020 at 7:23 AM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
>
> +               /*
> +                * If the "group" and "other" permissions are the same,
> +                * there's no point calling in_group_p() to decide which
> +                * set to use.
> +                */
> +               if ((((mode >> 3) ^ mode) & 7) && in_group_p(inode->i_gid))
>                         mode >>= 3;

Ugh. Not only is this ugly, but it's not even the best optimization.

We don't care that group and other match exactly. We only care that
they match in the low 3 bits of the "mask" bits.

So if we want this optimization - and it sounds worth it - I think we
should do it right. But I also think it should be written more
legibly.

And the "& 7" is the same "& (MAY_READ | MAY_WRITE | MAY_EXEC)" we do later.

In other words, if we do this, I'd like it to be done even more
aggressively, but I'd also like the end result to be a lot more
readable and have more comments about why we do that odd thing.

Something like this *UNTESTED* patch, perhaps?

I might have gotten something wrong, so this would need
double-checking, but if it's right, I find it a _lot_ more easy to
understand than making one expression that is pretty complicated and
opaque.

Hmm?

                 Linus

Download attachment "patch" of type "application/octet-stream" (1447 bytes)