This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 658734 - ACL: setfacl -x does not fail when asked to remove a non-existent Access Control Entry
ACL: setfacl -x does not fail when asked to remove a non-existent Access Cont...
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: man-pages-overrides (Show other bugs)
6.0
Unspecified Unspecified
low Severity low
: rc
: 6.1
Assigned To: Ivana Varekova
BaseOS QE - Apps
: ManPageChange
Depends On:
Blocks: 674883
  Show dependency treegraph
 
Reported: 2010-12-01 00:19 EST by Harshula Jayasuriya
Modified: 2011-05-19 10:28 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 674883 (view as bug list)
Environment:
Last Closed: 2011-05-19 10:28:58 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
merged patch that applies on top of acl-2.2.49-4.el6 (872 bytes, patch)
2010-12-06 07:54 EST, Kamil Dudka
no flags Details | Diff

  None (edit)
Description Harshula Jayasuriya 2010-12-01 00:19:07 EST
Description of problem:

* A customer has noticed inconsistent error reporting behaviour when a filesystem is remounted with noacl (the file in question actually has ACL entries).

Version-Release number of selected component (if applicable):
* RHEL 6: acl-2.2.49-4.el6
* RHEL 5: acl-2.2.39-6.el5

How reproducible:
* Always

Actual results:
------------------------------------------------------------
# setfacl -x u:test  /mnt/export/root

# setfacl -m u:test:rwx  /mnt/export/root
setfacl: /mnt/export/root: Operation not supported
------------------------------------------------------------

Expected results:
------------------------------------------------------------
# setfacl -x u:test  /mnt/export/root
setfacl: /mnt/export/root: Operation not supported

# setfacl -m u:test:rwx  /mnt/export/root
setfacl: /mnt/export/root: Operation not supported
------------------------------------------------------------

Regards,
Harshula
Comment 2 Harshula Jayasuriya 2010-12-01 00:24:24 EST
I had a look through the code and found that when there isn't an entry we fail silently instead of jumping to the "fail" label. The following patch changes the behaviour to the anticipated behaviour:
------------------------------------------------------------
--- a/setfacl/do_set.c
+++ b/setfacl/do_set.c
@@ -339,7 +340,7 @@ do_set(
 				if (ent)
 					acl_delete_entry(*xacl, ent);
 				else
-					/* ignore */;
+					goto fail;
 				break;
 
 			case CMD_REMOVE_EXTENDED_ACL:
------------------------------------------------------------
However, I'm uncertain if this change will impact the expected behaviour elsewhere.

Regards,
Harshula
Comment 4 Harshula Jayasuriya 2010-12-01 09:05:54 EST
I should have mentioned that an earlier getxattr(), via RETRIEVE_ACL(), sets errno to EOPNOTSUPP when setfacl -x is run on a f/s with "noacl".

RETRIEVE_ACL()
- retrieve_acl()
  - acl_get_file()
    - getxattr()
Comment 7 Andreas Gruenbacher 2010-12-05 17:06:50 EST
The getfacl and setfacl utilities can be transparently used on file systems with and without POSIX ACL support.  When a file has no permissions set other than the file permission bits, this is equivalent to a three-entry acl.  For example, a file mode of rw-r----- is equivalent to this acl:

   user::rw-
   group::r--
   other::---

When asked to modify the acl of a file, the setfacl utility retrieves the acl, modifies it according to the commands given, and saves the result.

The -x operation does not fail when asked to remove a non-existent entry; this is by design.  (I have clarified this in the man page now.)

The consequence is that on a file system without POSIX ACL support, "setfacl -x u:test" is a no-op, and "setfacl -m u:test:rwx" will try to set a real POSIX ACL which the file system cannot store, which must obviously fail.
Comment 8 Kamil Dudka 2010-12-05 17:18:57 EST
Andreas, thank you for the clarification and the man page fix.

upstream commit:

http://git.savannah.gnu.org/cgit/acl.git/commit/?id=5a5670a
Comment 9 Harshula Jayasuriya 2010-12-05 22:07:46 EST
Thanks Andreas for adding your comment.

Removing a non-existent ACL entry does, however, fail in some cases. It quite rightly reports an error for:

# setfacl -x u:test1 f
setfacl: Option -x: Invalid argument near character 3

The user "test1" does not exist. Similarly, I think the correct behaviour when the file system does not support non file permission bit ACLs is to report EOPNOTSUPP to the user. As soon as getxattr() completes (via retrieve_acl()) and sets errno to EOPNOTSUPP, we know both commands (setfacl -x u:test f and setfacl -m u:test:rw f) can not succeed. And that happens before we check if the ACL entry exists.

That's why I believe, if the f/s does not support ACLs:

1) getfacl should report the file permission bits
2) setfacl should succeed when operating ONLY on file permission bits
3) setfacl should report EOPNOTSUPP when NOT operating on file permission bits.

The patch in Comment 2 is definitely incorrect. To get the aforementioned behaviour would require a larger change.

Also, Andreas noted that it is not required that removing a non-existing ACL entry be a no-op by the draft standard.

Regards,
Harshula
Comment 10 Andreas Gruenbacher 2010-12-06 01:14:45 EST
The behavior you would like to see is not obviously better than the status quo, it is just different.  I am open to continuing this discussion, but not without any additional, sound arguments on your side.  Thanks.
Comment 11 Kamil Dudka 2010-12-06 03:12:47 EST
Harshula, how are you confirming that the current behavior is wrong?  If you just need to check whether the file system supports ACLs, there is a better way to do so...
Comment 12 Harshula Jayasuriya 2010-12-06 03:40:15 EST
Andreas, perhaps the "setfacl --remove" behaviour you have a personal preference for should be executed when the user wants to force the command by giving -f|--force ? e.g. see how rm behaves:
----------------------------------------------------
$ /bin/rm filedoesnotexist ; echo $?
/bin/rm: cannot remove `filedoesnotexist': No such file or directory
1
$ /bin/rm -f filedoesnotexist ; echo $?
0
----------------------------------------------------
User's already have expectations based on the precedent set by commonly used commands. We should strive for consistency.

I've conveyed my reasons for why at a minimum EOPNOTSUPP should be propagated to the user. I'll let this stew with you, and leave it up to you guys to decide. At least we have this BZ when the next customer complains.

BTW, there's a small typo in the updated man page: s/enries/entries/ .

Regards,
Harshula
Comment 13 Andreas Gruenbacher 2010-12-06 04:10:14 EST
> BTW, there's a small typo in the updated man page: s/enries/entries/ .

Thanks, the typo was even there before the latest change; it is fixed now:

  http://git.savannah.gnu.org/cgit/acl.git/commit/?id=986642a
Comment 14 Kamil Dudka 2010-12-06 07:54:57 EST
Created attachment 464977 [details]
merged patch that applies on top of acl-2.2.49-4.el6
Comment 21 errata-xmlrpc 2011-05-19 10:28:58 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-0780.html

Note You need to log in before you can comment on or make changes to this bug.