lkml.org 
[lkml]   [2020]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH resend] fs/namei.c: micro-optimize acl_permission_check
From
Date
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

\
 
 \ /
  Last update: 2020-06-08 12:09    [W:1.001 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site