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>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <dcd7516b-0a1f-320d-018d-f3990e771f37@rasmusvillemoes.dk>
Date:   Sun, 7 Jun 2020 15:22:13 +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-=wgz68f2u7bFPZCWgbsbEJw+2HWTJFXSg_TguY+xJ8WrNw@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 05/06/2020 22.18, Linus Torvalds wrote:
> On Fri, Jun 5, 2020 at 7:23 AM Rasmus Villemoes
> <linux@rasmusvillemoes.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.

Yes, I did think about that, but I thought this was the more obviously
correct approach, and that in practice one only sees the 0X44 and 0X55
cases.

> 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?

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.

> 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.

Well, I thought this was readable enough with the added comment. There's
already that magic constant 3 in the shifts, so the 7 seemed entirely
sensible, though one could spell it 0007. Whatever.

Perhaps this? As a whole function, I think that's a bit easier for
brain-storming. It's your patch, just with that rwx thing used instead
of mask, except for the call to check_acl().

static int acl_permission_check(struct inode *inode, int mask)
{
	unsigned int mode = inode->i_mode;
	unsigned int rwx = mask & (MAY_READ | MAY_WRITE | MAY_EXEC);

	/* Are we the owner? If so, ACL's don't matter */
	if (likely(uid_eq(current_fsuid(), inode->i_uid))) {
		if ((rwx << 6) & ~mode)
			return -EACCES;
		return 0;
	}

	/* Do we have ACL's? */
	if (IS_POSIXACL(inode) && (mode & S_IRWXG)) {
		int error = check_acl(inode, mask);
		if (error != -EAGAIN)
			return error;
	}

	/*
	 * Are the group permissions different from
	 * the other permissions in the bits we care
	 * about? Need to check group ownership if so.
	 */
	if (rwx & (mode ^ (mode >> 3))) {
		if (in_group_p(inode->i_gid))
			mode >>= 3;
	}

	/* Bits in 'mode' clear that we require? */
	return (rwx & ~mode) ? -EACCES : 0;
}

Rasmus