From 0aeb0bed6fe12afa9c3c9c5cab0fb287f768d5d5 Mon Sep 17 00:00:00 2001 From: "Paul B. Henson" Date: Fri, 6 Dec 2019 05:35:38 +0000 Subject: [PATCH] OpenZFS 6765 - zfs_zaccess_delete() comments do not accurately reflect delete permissions for ACLs Authored by: Kevin Crowe Reviewed by: Gordon Ross Reviewed by: Yuri Pankov Reviewed by: Brian Behlendorf Approved by: Richard Lowe Ported-by: Paul B. Henson Porting Notes: * Only comments are updated OpenZFS-issue: https://www.illumos.org/issues/6765 OpenZFS-commit: https://github.com/openzfs/openzfs/commit/da412744bc Closes #10266 --- module/os/linux/zfs/zfs_acl.c | 43 +++++++++++++++++------------------ 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/module/os/linux/zfs/zfs_acl.c b/module/os/linux/zfs/zfs_acl.c index cdaa51d5fa86..ff963b0a5501 100644 --- a/module/os/linux/zfs/zfs_acl.c +++ b/module/os/linux/zfs/zfs_acl.c @@ -2687,8 +2687,10 @@ int zfs_write_implies_delete_child = 1; /* * Determine whether delete access should be granted. * - * The following chart is the recommended NFSv4 enforcement for - * ability to delete an object. + * The following chart outlines how we handle delete permissions which is + * how recent versions of windows (Windows 2008) handles it. The efficiency + * comes from not having to check the parent ACL where the object itself grants + * delete: * * ------------------------------------------------------- * | Parent Dir | Target Object Permissions | @@ -2697,14 +2699,14 @@ int zfs_write_implies_delete_child = 1; * | | ACL Allows | ACL Denies| Delete | * | | Delete | Delete | unspecified| * ------------------------------------------------------- - * | ACL Allows | Permit | Permit * | Permit | - * | DELETE_CHILD | | | | + * | ACL Allows | Permit | Deny * | Permit | + * | DELETE_CHILD | | | | * ------------------------------------------------------- - * | ACL Denies | Permit * | Deny | Deny | - * | DELETE_CHILD | | | | + * | ACL Denies | Permit | Deny | Deny | + * | DELETE_CHILD | | | | * ------------------------------------------------------- * | ACL specifies | | | | - * | only allow | Permit | Permit * | Permit | + * | only allow | Permit | Deny * | Permit | * | write and | | | | * | execute | | | | * ------------------------------------------------------- @@ -2717,24 +2719,21 @@ int zfs_write_implies_delete_child = 1; * Re. execute permission on the directory: if that's missing, * the vnode lookup of the target will fail before we get here. * - * Re [*] in the table above: We are intentionally disregarding the - * NFSv4 committee recommendation for these three cells of the matrix - * because that recommendation conflicts with the behavior expected - * by Windows clients for ACL evaluation. See acl.h for notes on - * which ACE_... flags should be checked for which operations. - * Specifically, the NFSv4 committee recommendation is in conflict - * with the Windows interpretation of DENY ACEs, where DENY ACEs + * Re [*] in the table above: NFSv4 would normally Permit delete for + * these two cells of the matrix. + * See acl.h for notes on which ACE_... flags should be checked for which + * operations. Specifically, the NFSv4 committee recommendation is in + * conflict with the Windows interpretation of DENY ACEs, where DENY ACEs * should take precedence ahead of ALLOW ACEs. * - * This implementation takes a conservative approach by checking for - * DENY ACEs on both the target object and it's container; checking - * the ACE_DELETE on the target object, and ACE_DELETE_CHILD on the - * container. If a DENY ACE is found for either of those, delete - * access is denied. (Note that DENY ACEs are very rare.) + * This implementation always consults the target object's ACL first. + * If a DENY ACE is present on the target object that specifies ACE_DELETE, + * delete access is denied. If an ALLOW ACE with ACE_DELETE is present on + * the target object, access is allowed. If and only if no entries with + * ACE_DELETE are present in the object's ACL, check the container's ACL + * for entries with ACE_DELETE_CHILD. * - * Note that after these changes, entire the second row and the - * entire middle column of the table above change to Deny. - * Accordingly, the logic here is somewhat simplified. + * A summary of the logic implemented from the table above is as follows: * * First check for DENY ACEs that apply. * If either target or container has a deny, EACCES.