8984 fix for 6764 breaks ACL inheritance

[citrus-it]

https://www.illumos.org/issues/8984

As per the documentation, for the aclinherit property:

When the property value is set to passthrough, files are created with a
mode determined by the inheritable ACEs. If no inheritable ACEs exist
that affect the mode, then the mode is set in accordance to the
requested mode from the application.

6764 introduced a bug causing the requested mode to always be set, even if mode-affecting inheritable ACEs are present.

See https://illumos.topicbox.com/groups/zfs/Te5cbb71851130ac1-M486e4 for more discussion and example, including this from Albert Lee:

You are correct, aclinherit=passthrough/passthrough-x should still
ignore the requested mode when an inheritable ACE for owner@ group@,
or everyone@ is present in the parent directory.

It appears there was an oversight in my fix for
https://www.illumos.org/issues/6764 which made calling zfs_acl_chmod
from zfs_acl_inherit unconditional. I think the parent ACL check for
aclinherit=passthrough needs to be reintroduced in zfs_acl_inherit.

This change re-introduces that parent ACL check.
Work by @hadfl

Testing

r151022# zfs create -o aclinherit=passthrough-x rpool/test
r151022# cd /rpool/test
r151022# mkdir dropbox
r151022# chmod A+user:af:full_set:f:allow dropbox
r151022# ls -dV dropbox
drwxr-xr-x+  2 root     root           2 Feb 18 21:37 dropbox/
                user:af:rwxpdDaARWcCos:f------:allow
                 owner@:rwxp-DaARWcCos:-------:allow
                 group@:r-x---a-R-c--s:-------:allow
              everyone@:r-x---a-R-c--s:-------:allow

# no inheritable owner@, group@, everyone@ - so normal behaviour

r151022# touch dropbox/test
r151022# ls -V dropbox/test
-rw-r--r--+  1 root     root           0 Feb 18 21:38 dropbox/test
                user:af:rw-pdDaARWcCos:------I:allow
                 owner@:rw-p--aARWcCos:-------:allow
                 group@:r-----a-R-c--s:-------:allow
              everyone@:r-----a-R-c--s:-------:allow

# Now setting inheritable owner...

r151022# chmod A+owner@:rwx:fi:allow dropbox
r151022# touch dropbox/test2
r151022# ls -V dropbox/test2
-rw-------+  1 root     root           0 Feb 18 21:40 dropbox/test2
                 owner@:rw------------:------I:allow
                user:af:rw-pdDaARWcCos:------I:allow

# and group

r151022# chmod A0=group@:rwx:fi:allow dropbox
r151022# touch dropbox/test3
r151022# ls -V dropbox/test3
----rw----+  1 root     root           0 Feb 18 21:41 dropbox/test3
                 group@:rw------------:------I:allow
                user:af:rw-pdDaARWcCos:------I:allow

# and everyone

r151022# chmod A0=everyone@:rwx:fi:allow dropbox
r151022# touch dropbox/test4
r151022# ls -V dropbox/test4
-rw-rw-rw-+  1 root     root           0 Feb 18 21:42 dropbox/test4
              everyone@:rw------------:------I:allow
                user:af:rw-pdDaARWcCos:------I:allow

[andy-js]

How would you feel about adding a tuneable to make this unconditional? Something like zfs_acl_always_chmod maybe.

[pbhenson]

For what purpose? need_chmod defaults to true, and it's only set to false if aclinherit is passthrough or passthrough-x and there are trivial acl entries being inherited (to match the documented behavior of these modes). Setting that proposed tuning variable would just result in the broken ACL inheritance that this patch fixes.

If you want a behavior like "inherit everything but then chmod anyway" it seems that should be a new type of aclinherit?

[citrus-it]

I agree with @pbhenson here. This change is about reverting behaviour that changed in 6764 so that it again works as documented (and expected).

The chmod step is only skipped if there are inheritable ACLs that directly affect the file mode (i.e. user@, group@, everyone@) and that makes sense since the reason to set these is to override the standard ugo permissions. Other inheritable ACLs (e.g. user: or group:) don't suppress the chmod.

I can't see a scenario where unconditional chmod would be required but it does seem like another value for aclinherit and not part of this change.

[szaydel]

I think part of the problem might be that there are those already relying on supposedly broken functionality. A tunable would allow for a softer landing, at least in some cases. This will certainly be true for customers of RackTop, so I definitely support @andy-js's suggestion.

[pbhenson]

It's not "supposedly" broken, it is broken. The current behavior was not an intention of the patch that created it, and it violates the documentation that defines how it should behave. I'm really curious in what scenario someone would want to define explicit inherited ACE's for the legacy components of the ACL and also have that explicit definition randomly changed based on the whims of the application and the state of the umask?

As I proposed, if this behavior is desirable, it should result from a new aclinherit setting, not a tunable whose name would be more appropriately "zfs_break_acl_passthrough" 8-/.

[szaydel]

There's a long history of things broken that when fixed end-up breaking what was previously working thanks to previously-broken functionality. The point is that in some environments this may be through happenstance actually doing right thing, because the application used maybe singular and may actually be behaving desirably. Albert confirmed that this is a problem, and it is not like anyone disagrees. I think about the only point here is fix may actually cause pain for someone.

[pbhenson]

Yes, any time behavior is changed, either to fix a bug or introduce a new feature, there is the possibility that someone somewhere might have been relying on the previous behavior. But you can't introduce backwards compatible settings for each and every change ever made, that would just be unwieldy and poor design.

In this case, the change is restoring proper behavior for a documented interface that was temporarily doing the wrong thing. Particularly given as of yet there haven't been any concrete examples as to how this might be desirable behavior, this doesn't seem like a scenario where such a kludge would be appropriate. I take it you work for RackTop? Why is it you think your customers would want to continue with the broken behavior rather than the correct behavior for this aclinherit setting?

Further, as I suggested, if this behavior is desirable for someone, it would be better implemented as a separate aclinherit option rather than providing the confusing ability to have an existing documented mode behave differently. The current broken behavior wouldn't be the default, so an admin reading the release notes would have to do something to maintain it, whether it be setting a kernel tunable or changing an aclinherit setting. Plus a new setting would allow it to be set on a per filesystem basis rather than globally; what if you have some customers who want the fixed behavior and others who want the "different in general but broken for this setting" behavior?

[prakashsurya]

@citrus-it can you rebase this onto the lastest master branch? to pick up some fixes I made yesterday to the test infrastructure.

[citrus-it]

Done.

[prakashsurya]

@citrus-it Thanks!

@szaydel @andy-js @pbhenson Can I count you all as having reviewed this?

[szaydel]

@prakashsurya: Yes you count me as having reviewed.

@pbhenson:
All good points, I cannot disagree. By default filesystems created on RackTop's systems will have aclinherit set to passthrough, and while I am not positive that we have any customers who rely on current behavior, chances are we do and we just don't know. I think for us it is going to be a case of figuring out how to best address this internally. Clearly, there is agreement that we are going from broken to not broken, and I suppose we will internally have to investigate impact from this and perhaps figure out how to handle this if we find that someone is actually seriously depending on current behavior.

My sense is, even if there is a dependency on it from some of our paying customers we won't attempt to permanently leave that behavior in place, but may temporarily implement something to make sure that there's a transition period.

[pbhenson]

@prakashsurya yes, for what my opinion is worth it looks good to me :). We've been running it on our dev systems for a week or so and will be dropping the fix onto our prod systems tomorrow evening.

@szaydel it should be pretty trivial to add a new aclinherit setting that provides the current incorrect behavior, named say "passthrough-chmod" or something. I just don't know that such a resultant ACL would be useful?