Bug 216886 - ZFS with aclinherit and aclmode set to passthrough adds an extra default ACLs
Summary: ZFS with aclinherit and aclmode set to passthrough adds an extra default ACLs
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Andriy Gapon
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-07 14:34 UTC by Andrey Orlov
Modified: 2018-04-21 19:12 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Orlov 2017-02-07 14:34:37 UTC
This behavior starts from 11.0 and lasts till now (revision HEAD 313385)
1. ZFS "aclinherit" and "aclmode" both set to "passthrough".
2. every syscall to create FileSystem object (dir or file) adds extra default ACls.
How to reproduce:
[~](16:49:59)# uname -a
FreeBSD  12.0-CURRENT FreeBSD 12.0-CURRENT #0 r313385M: Fri Feb  7 11:18:43 UTC 2017     root@msk:/usr/obj/data/src_head/sys/KERNEL_HEAD  amd64
[/](17:11:57)# zpool create test ada1
[/](17:12:02)# zfs list
NAME   USED  AVAIL  REFER  MOUNTPOINT
test   292K   899G    88K  /test
[/](17:12:15)# zfs set aclinherit=passthrough test && zfs set aclmode=passthrough test
[/](17:12:40)# zfs get all test | grep acl
test  aclmode               passthrough            local
test  aclinherit            passthrough            local
[/](17:12:45)# cd /test
[/test](17:13:38)# getfacl /test
# file: /test
# owner: root
# group: wheel
            owner@:rwxp--aARWcCos:-------:allow
            group@:r-x---a-R-c--s:-------:allow
         everyone@:r-x---a-R-c--s:-------:allow
[/test](17:15:04)# umask
0022
[/test](17:15:06)# setfacl -m owner@:rwxp--aARWcCos:fd-----:allow,group@:a-R-c--s:fd-----:allow,everyone@:a-R-c--s:fd-----:allow /test
[/test](17:16:13)# getfacl /test
# file: /test
# owner: root
# group: wheel
            owner@:rwxp--aARWcCos:fd-----:allow
            group@:------a-R-c--s:fd-----:allow
         everyone@:------a-R-c--s:fd-----:allow
[/test](17:16:28)# 
[/test](17:16:28)# touch test_file_1
[/test](17:16:50)# getfacl test_file_1 
# file: test_file_1
# owner: root
# group: wheel
            owner@:rwxp--aARWcCos:------I:allow
            group@:------a-R-c--s:------I:allow
         everyone@:------a-R-c--s:------I:allow
            owner@:rw-p--aARWcCos:-------:allow
            group@:r-----a-R-c--s:-------:allow
         everyone@:r-----a-R-c--s:-------:allow
[/test](17:17:00)# mkdir test_dir_1
[/test](17:18:28)# getfacl test_dir_1
# file: test_dir_1
# owner: root
# group: wheel
            owner@:rwxp--aARWcCos:fd----I:allow
            group@:------a-R-c--s:fd----I:allow
         everyone@:------a-R-c--s:fd----I:allow
            owner@:rwxp--aARWcCos:-------:allow
            group@:r-x---a-R-c--s:-------:allow
         everyone@:r-x---a-R-c--s:-------:allow
[/test](17:18:35)#
Comment 1 Harald Schmalzbauer 2018-02-21 14:12:32 UTC
I was about to report the same, already asked in freebsd-fs@ with the topic "New in 11? ZFS ACL -> aclinherit stacks synthesized mode ACEs".
Can't add anything useful, only confirm that it was introduced with 11.0, unfortunately without knowing the corresponding head revision.

Hope someone can find some time to bisect/analyze/fix this glitch before 11.2.

Thanks,

-harry
Comment 2 Andriy Gapon freebsd_committer 2018-02-21 15:38:24 UTC
Do these look relevant / related to the problem?

https://www.illumos.org/issues/8984
https://github.com/openzfs/openzfs/pull/557
Comment 3 commit-hook freebsd_committer 2018-03-07 13:50:26 UTC
A commit references this bug:

Author: avg
Date: Wed Mar  7 13:49:27 UTC 2018
New revision: 330592
URL: https://svnweb.freebsd.org/changeset/base/330592

Log:
  MFV r330591: 8984 fix for 6764 breaks ACL inheritance

  illumos/illumos-gate@e9bacc6d1a71ea3f7082038b2868de8c4dd98bdc
  https://github.com/illumos/illumos-gate/commit/e9bacc6d1a71ea3f7082038b2868de8c4dd98bdc

  https://www.illumos.org/issues/8984
    Consider a directory configured as:
    drwx-ws---+ 2 henson cpp 3 Jan 23 12:35 dropbox/
    user:henson:rwxpdDaARWcC--:f-i----:allow
    owner@:--------------:f-i----:allow
    group@:--------------:f-i----:allow
    everyone@:--------------:f-i----:allow
    owner@:rwxpdDaARWcC--:-di----:allow
    group:cpp:-wx-----------:-------:allow
    owner@:rwxpdDaARWcC--:-------:allow
    A new file created in this directory ends up looking like:
    rw-r--r-+ 1 astudent cpp 0 Jan 23 12:39 testfile
    user:henson:rw-pdDaARWcC--:------I:allow
    owner@:--------------:------I:allow
    group@:--------------:------I:allow
    everyone@:--------------:------I:allow
    owner@:rw-p--aARWcCos:-------:allow
    group@:r-----a-R-c--s:-------:allow
    everyone@:r-----a-R-c--s:-------:allow
    with extraneous group@ and everyone@ entries allowing read access that
    shouldn't exist.
    Per Albert Lee on the zfs mailing list:
    "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."
    We have a large number of faculty who use dropbox directories like the example
    to have students submit projects. All of these directories are now allowing

  Reviewed by: Sam Zaydel <szaydel@racktopsystems.com>
  Reviewed by: Paul B. Henson <henson@acm.org>
  Reviewed by: Prakash Surya <prakash.surya@delphix.com>
  Approved by: Matthew Ahrens <mahrens@delphix.com>
  Author: Dominik Hassler <hadfl@omniosce.org>

  PR:		216886
  MFC after:	2 weeks

Changes:
_U  head/sys/cddl/contrib/opensolaris/
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c
Comment 4 Andriy Gapon freebsd_committer 2018-03-07 13:52:58 UTC
Anyone having this problem, could you please test the commit from comment #3 and see if it actually fixes the problem on FreeBSD as well?
Comment 5 Dave Baukus 2018-03-08 16:14:19 UTC
(In reply to Andriy Gapon from comment #4(In reply to Andriy Gapon from comment #4)

I have verified that the patch eliminates the generation of the duplicate set of ACLs (the ones without the 'I' inherited bit):

Repeating the provided example with the fix applied:

# umask
0022
# setfacl -m owner@:rwxp--aARWcCos:fd-----:allow,group@:a-R-c--s:fd-----:allow,everyone@:a-R-c--s:fd-----:allow /export/vol0/
# getfacl /export/vol0/
# file: /export/vol0/
# owner: root
# group: wheel
            owner@:rwxp--aARWcCos:fd-----:allow
            group@:------a-R-c--s:fd-----:allow
         everyone@:------a-R-c--s:fd-----:allow
# ls -al
total 17
drwx------+ 2 root  wheel  2 Mar  8 15:38 .
drwxrwxr-t  3 root  wheel  4 Mar  1 20:34 ..
# touch F0
# getfacl /export/vol0/F0
# file: /export/vol0/F0
# owner: root
# group: wheel
            owner@:rwxp--aARWcCos:------I:allow
            group@:------a-R-c--s:------I:allow
         everyone@:------a-R-c--s:------I:allow
# mkdir D0
# getfacl /export/vol0/D0
# file: /export/vol0/D0
# owner: root
# group: wheel
            owner@:rwxp--aARWcCos:fd----I:allow
            group@:------a-R-c--s:fd----I:allow
         everyone@:------a-R-c--s:fd----I:allow
Comment 6 commit-hook freebsd_committer 2018-03-21 15:07:37 UTC
A commit references this bug:

Author: avg
Date: Wed Mar 21 15:07:26 UTC 2018
New revision: 331302
URL: https://svnweb.freebsd.org/changeset/base/331302

Log:
  MFC r330592: MFV r330591: 8984 fix for 6764 breaks ACL inheritance

  PR:		216886

Changes:
_U  stable/11/
  stable/11/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_acl.c
Comment 7 Andriy Gapon freebsd_committer 2018-03-21 15:15:57 UTC
(In reply to Dave Baukus from comment #5)
Dave, thank you!
Comment 8 Harald Schmalzbauer 2018-04-21 19:12:49 UTC
The problem, explicitly reported with "aclinherit=passthrough" property is fixed indeed.
But it still persists for ZFS filesystems with the _default_ "aclinherit=restricted" property!

Very briefly looking at upstream discussion seems that they unintentionally fixed only the passthrough[-x] case and simply forgot about the default "restricted", which also respects the inheritance bits and also does ACE "stacking", with modifications like described in the man page.

So most likely this has to be reopened upstream.
Unfortunately I can't take care of it.

-harry