Fwd: posix_acl_permission() and MAY_* flags

Fwd: posix_acl_permission() and MAY_* flags

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

I was looking at POSIX ACL on-disk and in-memory code and it looks like
there is a subtle dependency between the on-disk format and what (IMHO)
would be considered in-memory declarations.

When a POSIX ACL is read from disk, posix_acl_from_mode() copies the file
mode (S_I[RWX][UGO]) into the e_perm fields of the ACL default entries.
Similarly, in posix_acl_equiv_mode() and posix_acl_create_masq() it uses
S_IRWXO to mask the e_perm flags.

However, later on in posix_acl_permission() it directly uses the "want"
flag contains MAY_{READ,WRITE,EXEC} flags and compares those to e_perm of
each ACL entry.

In posix_acl_valid() it compares e_perm with ACL_{READ,WRITE,EXECUTE}.

While the MAY_[RWX] and ACL_[RWX] currently have the same value as
S_I[RWX]OTH, it isn't very clear that these flags MUST all have the same
values or POSIX ACLs will break.

This definitely doesn't seem quite right.  Are the ACL_* constants the
values to be used, with "conversion" in between the flags/modes?  Should
there be a BUILD_BUG_ON() that trips if those values ever differ?

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: posix_acl_permission() and MAY_* flags

[Andreas Grünbacher]

Am Fr., 12. Okt. 2018 um 02:44 Uhr schrieb Andreas Dilger <adilger@dilger.ca>:
> I was looking at POSIX ACL on-disk and in-memory code and it looks like
> there is a subtle dependency between the on-disk format and what (IMHO)
> would be considered in-memory declarations.
>
> When a POSIX ACL is read from disk, posix_acl_from_mode() copies the file
> mode (S_I[RWX][UGO]) into the e_perm fields of the ACL default entries.
> Similarly, in posix_acl_equiv_mode() and posix_acl_create_masq() it uses
> S_IRWXO to mask the e_perm flags.
>
> However, later on in posix_acl_permission() it directly uses the "want"
> flag contains MAY_{READ,WRITE,EXEC} flags and compares those to e_perm of
> each ACL entry.

As far as I can tell, this practice even goes back to before POSIX
ACLs. For example, if you look at function vfs_permission in
fs/namei.c in a v2.4 tree, you'll find something like this:

    if (((mode & mask & (MAY_READ|MAY_WRITE|MAY_EXEC)) == mask))
        return 0;

Here, mode is inode->i_mode shifted so that the bits that matter are
the lowest three (S_IRWXO) and mask is a combination of MAY_ flags.

> In posix_acl_valid() it compares e_perm with ACL_{READ,WRITE,EXECUTE}.
>
> While the MAY_[RWX] and ACL_[RWX] currently have the same value as
> S_I[RWX]OTH, it isn't very clear that these flags MUST all have the same
> values or POSIX ACLs will break.
>
> This definitely doesn't seem quite right.  Are the ACL_* constants the
> values to be used, with "conversion" in between the flags/modes?  Should
> there be a BUILD_BUG_ON() that trips if those values ever differ?

The ACL_{READ,WRITE,EXECUTE} and MAY_{READ,WRITE,EXEC} values must
definitely have the same values. This wouldn't be true for higher
bits, but POSIX ACLs don't support anything beyond rwx.

Andreas

Re: posix_acl_permission() and MAY_* flags

[Al Viro]

On Fri, Oct 12, 2018 at 11:09:38AM +0200, Andreas Gr�nbacher wrote:

> The ACL_{READ,WRITE,EXECUTE} and MAY_{READ,WRITE,EXEC} values must
> definitely have the same values. This wouldn't be true for higher
> bits, but POSIX ACLs don't support anything beyond rwx.

Yes.  What's more, nobody is going to change the values for any of
those - consider them tied to pretty much universal encoding going
through all Unix filesystem layouts and all Unix ABIs.

Not all uses of symbolic constants mean that the values can be
redefined.  In particular, MAY_READ == R_OK, etc. - the names
are not directly exposed to userland, but attempt to change the
values will immediately break access(2) or demand remapping in
it.  They are also tied to on-disk layouts.

If you want BUILD_BUG_ON() on those, we could add such, but I
really don't see the point - anyone changing any of those will
get instant breakage as soon as they try to boot.  Or the patch
will have a very heavy footprint and raise obvious red flags on
review (along the lines of "WTF do you insert that crap on a lot
of hot paths?  You are changing what, again?  What for?")

Re: posix_acl_permission() and MAY_* flags

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

On Oct 12, 2018, at 9:56 PM, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> On Fri, Oct 12, 2018 at 11:09:38AM +0200, Andreas Grünbacher wrote:
> 
>> The ACL_{READ,WRITE,EXECUTE} and MAY_{READ,WRITE,EXEC} values must
>> definitely have the same values. This wouldn't be true for higher
>> bits, but POSIX ACLs don't support anything beyond rwx.
> 
> Yes.  What's more, nobody is going to change the values for any of
> those - consider them tied to pretty much universal encoding going
> through all Unix filesystem layouts and all Unix ABIs.
> 
> Not all uses of symbolic constants mean that the values can be
> redefined.  In particular, MAY_READ == R_OK, etc. - the names
> are not directly exposed to userland, but attempt to change the
> values will immediately break access(2) or demand remapping in
> it.  They are also tied to on-disk layouts.
> 
> If you want BUILD_BUG_ON() on those, we could add such, but I
> really don't see the point - anyone changing any of those will
> get instant breakage as soon as they try to boot.  Or the patch
> will have a very heavy footprint and raise obvious red flags on
> review (along the lines of "WTF do you insert that crap on a lot
> of hot paths?  You are changing what, again?  What for?")

It's not that I'm _so_ worried about the values changing, just
that I was following the ACL code paths around, and the caller
is passing in MAY_* flags on the one side, but then comparing
them to values set from S_I*OTH flags on disk and it made me
wonder if something was broken, or if it might break in the future.

I would definitely agree that S_I* flags are set in stone, but
I've never really thought of MAY_* flags as being directly tied
to on-disk values because there are so many more than just
MAY_{READ,WRITE,EXECUTE} - MAY_APPEND, MAY_OPEN, etc.  I'd always
thought of them like the EXT4_IMMUTABLE_FL on-disk flags vs.
the S_IMMUTABLE inode flags in memory.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: posix_acl_permission() and MAY_* flags

[Al Viro]

On Fri, Oct 12, 2018 at 10:08:57PM -0600, Andreas Dilger wrote:

> It's not that I'm _so_ worried about the values changing, just
> that I was following the ACL code paths around, and the caller
> is passing in MAY_* flags on the one side, but then comparing
> them to values set from S_I*OTH flags on disk and it made me
> wonder if something was broken, or if it might break in the future.
> 
> I would definitely agree that S_I* flags are set in stone, but
> I've never really thought of MAY_* flags as being directly tied
> to on-disk values because there are so many more than just
> MAY_{READ,WRITE,EXECUTE} - MAY_APPEND, MAY_OPEN, etc.  I'd always
> thought of them like the EXT4_IMMUTABLE_FL on-disk flags vs.
> the S_IMMUTABLE inode flags in memory.

In theory - yes, in practice... imm/append-only are nowhere near
as common and didn't have universal values on-disk (ext* ones are
different from ufs ones, for example).

It might be worth a comment (near the definition of MAY_...,
probably mentioning that MAY_READ/MAY_WRITE/MAY_EXEC are
also equal to R_OK/W_OK/X_OK), but that's about it.

Re: Fwd: posix_acl_permission() and MAY_* flags

[Al Viro]

On Thu, Oct 11, 2018 at 06:43:57PM -0600, Andreas Dilger wrote:
> I was looking at POSIX ACL on-disk and in-memory code and it looks like
> there is a subtle dependency between the on-disk format and what (IMHO)
> would be considered in-memory declarations.
> 
> When a POSIX ACL is read from disk, posix_acl_from_mode() copies the file
> mode (S_I[RWX][UGO]) into the e_perm fields of the ACL default entries.
> Similarly, in posix_acl_equiv_mode() and posix_acl_create_masq() it uses
> S_IRWXO to mask the e_perm flags.
> 
> However, later on in posix_acl_permission() it directly uses the "want"
> flag contains MAY_{READ,WRITE,EXEC} flags and compares those to e_perm of
> each ACL entry.
> 
> In posix_acl_valid() it compares e_perm with ACL_{READ,WRITE,EXECUTE}.
> 
> While the MAY_[RWX] and ACL_[RWX] currently have the same value as
> S_I[RWX]OTH, it isn't very clear that these flags MUST all have the same
> values or POSIX ACLs will break.
> 
> This definitely doesn't seem quite right.  Are the ACL_* constants the
> values to be used, with "conversion" in between the flags/modes?  Should
> there be a BUILD_BUG_ON() that trips if those values ever differ?

Encoding of rwx bits is pretty much cast in stone - they go all way back
to v1 (if not to PDP7 times) and I can't imagine any Unix variant that
would have them not in the same order.  MAY_... is tied to those and
so are the bits in ->e_perm.

IOW, all of those are in practice immutable - too closely tied to on-disk
data structures in a lot of filesystems *and* to any number of userland
programs using explicit octal values for mkdir(), etc. arguments.

Symbolic constants != can realistically be redefined...