On 07/06/2020 21.48, Linus Torvalds wrote: > On Sun, Jun 7, 2020 at 9:37 AM Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> 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
