Bug 1666850

Looks like this problem can also be triggered when using RHEL server.

Have checked that:
 - the last good: nfs4-acl-tools-0.3.3-17.el7
 - the first bad: nfs4-acl-tools-0.3.3-18.el7
   (might be introduced via bz1412181)

Testing logs:
~~~~~~~~~~~~~
[root ~]# mount localhost:/export_test/ /mnt/
[root ~]# nfsstat -m
/mnt from localhost:/export_test
 Flags: rw,relatime,vers=4.1,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=sys,clientaddr=127.0.0.1,local_lock=none,addr=127.0.0.1

[root ~]# touch /mnt/{first,second}
#####################
# "-s acl_spec" fails
#####################
[root ~]# nfs4_setfacl --test -s "$(nfs4_getfacl /mnt/first)" /mnt/second
Failed while inserting ACE(s).            <<<<<<<<<<<<<<<<<<<<<<<<<
########################
# "-S acl_file" can work
########################
[root ~]# nfs4_getfacl /mnt/first >/tmp/spec 
[root ~]# nfs4_setfacl --test -S /tmp/spec /mnt/second
## Test mode only - the resulting ACL for "/mnt/second": 
A::OWNER@:rwatTcCy
A:g:GROUP@:rtcy
A::EVERYONE@:rtcy
[root ~]# rpm -q nfs4-acl-tools
nfs4-acl-tools-0.3.3-19.el7.x86_64   <-----------------------
[root ~]# yum downgrade -y nfs4-acl-tools-0.3.3-18.el7.x86_64.rpm >/dev/null
[root ~]# nfs4_setfacl --test -s "$(nfs4_getfacl /mnt/first)" /mnt/second
Failed while inserting ACE(s).            <<<<<<<<<<<<<<<<<<<<<<<<<
[root ~]# yum downgrade -y nfs4-acl-tools-0.3.3-17.el7.x86_64.rpm >/dev/null
[root ~]# nfs4_setfacl --test -s "$(nfs4_getfacl /mnt/first)" /mnt/second
## Test mode only - the resulting ACL for "/mnt/second": <<<<<<<<<
A::OWNER@:rwatTcCy
A:g:GROUP@:rtcy
A::EVERYONE@:rtcy
[root ~]# rpm -q nfs4-acl-tools
nfs4-acl-tools-0.3.3-17.el7.x86_64   <-----------------------
[root ~]# 
~~~~~~~~~~~~~

This was caused due to the below patch.

nfs4_getfacl: Add support to accept more paths
http://git.linux-nfs.org/?p=bfields/nfs4-acl-tools.git;a=commit;h=6630629bb661a7f48fb9856f7fd9616ce1499efa

As we are accepting more paths it is necessary to print the filename for which the corresponding output is shown. (just like how getfacl does)

From code nfs4_getfacl/nfs4_getfacl.c

       for (counter = 1; counter < argc; counter++) {
               acl = nfs4_acl_for_path(argv[counter]);
               if (acl != NULL) {
                       printf("\n# file: %s\n",argv[counter]); <--------------
                       nfs4_print_acl(stdout, acl);
                       nfs4_free_acl(acl);
                       res = 0;
               }
        }
$ nfs4_getfacl /test/

# file: /test/ <------------------------ 
A::OWNER@:rwaDxtTcCy
A::GROUP@:rwaDxtcy
A::EVERYONE@:rwaDxtcy

As "# file: /test/" is and invalid entry we get the below error.
Failed while inserting ACE(s). <---------------

To fix this issue we need to ignore the # line and proceed further.

The correct fix would be skip printing the file name if exactly one filename was requested.

Passing more than one ACL list to nfs4_setfacl wouldn't be useful anyway. If really needed for some odd reason, a pipe through grep -v '^# file:' would work.

Bruce / Steve D, can you take a look at this and update the bug status? It has landed on the blockers list for 7.7, so we need to either get it off that list or to give frequent status updates.

(In reply to Bill Carlson from comment #5)
> The correct fix would be skip printing the file name if exactly one filename
> was requested.
> 
> Passing more than one ACL list to nfs4_setfacl wouldn't be useful anyway. If
> really needed for some odd reason, a pipe through grep -v '^# file:' would
> work.

getfacl can actually use the comments (see the --restore= argument to setfacl), so we could consider doing that for nfs4_getfacl as well some day.  And getfacl also does print a comment with the filename even when only given a single filename.

For now I agree with Kenneth that nfs4_setfacl should just skip comments.

And I'd rather leave the comments in the nfs4_setfacl output, since long term I'd like to be more consistent with setfacl.  But if people already have scripts that depend on the old behavior, then, yes, I guess we should remove the comments in the single-file case too.

Kenneth, do you have patches to make those two changes (one patch to skip comments in nfs4_setfacl, one to remove comments in nfs4_setfacl in the single file case?)

>> Kenneth, do you have patches to make those two changes (one patch to skip comments in nfs4_setfacl, one to remove comments in nfs4_setfacl in the single file case?)
The first patch is a bit tedious for fixing nfs4_setfacl code to skip comments.( Not yet ready)
While the second patch to fix nfs4_getfacl code to remove file comment is possible.

I came with a better fix to ignore comments in nfs4_getfacl output by introducing a -c option. This way we don't need to touch the present nfs4_setfacl code at all.
-c, --omit-header	Do not display the comment header (Do not print filename)
 
With fix:

# nfs4_setfacl --test -s "$(nfs4_getfacl -c file1)" file2
## Test mode only - the resulting ACL for "/test/file2": 
A::OWNER@:rwatTcCy
A:g:GROUP@:rtcy
A::EVERYONE@:rtcy

Without fix:

# nfs4_setfacl --test -s "$(nfs4_getfacl  file1)" file2
Failed while inserting ACE(s).


commit 81efc65737dc60590996dcf476cce03517baf5d2 (HEAD -> master)
Author: Kenneth D'souza <kdsouza>
Date:   Fri Mar 22 16:55:14 2019 +0530

    nfs4_getfacl: Add new option -c/--omit-header to not display comment header.
    
    With this patch the filename will not be printed.
    
     $ nfs4_getfacl /test/
     # file: /test/
     A::OWNER@:rwaDxtTcCy
     A::GROUP@:rwaDxtcy
     A::EVERYONE@:rwaDxtcy
    
     $ nfs4_getfacl -c /test/
    
     A::OWNER@:rwaDxtTcCy
     A::GROUP@:rwaDxtcy
     A::EVERYONE@:rwaDxtcy
    
    Signed-off-by: Kenneth D'souza <kdsouza>

Attaching the patch to this BZ.

Created attachment 1546827 [details]
[PATCH]  nfs4_getfacl: Add new option -c/--omit-header to not display comment header.

To maintain backwards compatibility, the parameter should enable the comments with default being no comments.

Any script using nfs4_getfacl as input to nfs4_setfacl will be broken otherwise.


I expect some scripts may depend on nfs4_getfacl erroring if passed more than one file, wouldn't it make sense to have a parameter to enable multiple file support, such as -R or -m?

(In reply to Kenneth D'souza from comment #9)
> Created attachment 1546827 [details]
> [PATCH]  nfs4_getfacl: Add new option -c/--omit-header to not display
> comment header.

Would it be possible to post this patch upstream 
Linux NFS Mailing List <linux-nfs.org>

(In reply to Steve Dickson from comment #13)
> (In reply to Kenneth D'souza from comment #9)
> > Created attachment 1546827 [details]
> > [PATCH]  nfs4_getfacl: Add new option -c/--omit-header to not display
> > comment header.
> 
> Would it be possible to post this patch upstream 
> Linux NFS Mailing List <linux-nfs.org>

Sure, will post both the patches.

nfs4_setfacl patch to skip comments.

commit af4e1a360aac53ab1fbf3e61372464ed080d537b (HEAD -> master)
Author: Kenneth D'souza <kdsouza>
Date:   Mon Mar 25 19:29:20 2019 +0530

    nfs4_setfacl: Skip comment field while reading ACE(s).
    
     With fix:
    
     $ nfs4_setfacl --test -s "$(nfs4_getfacl  file1)" file2
     Skipping comment # file: file1
     ## Test mode only - the resulting ACL for "/test/file2":
     A::OWNER@:rwatTcCy
     A:g:GROUP@:rtcy
     A::EVERYONE@:rtcy
    
     Without fix:
    
     $ nfs4_setfacl --test -s "$(nfs4_getfacl  file1)" file2
     Failed while inserting ACE(s).
    
    Signed-off-by: Kenneth D'souza <kdsouza>

diff --git a/libnfs4acl/nfs4_insert_string_aces.c b/libnfs4acl/nfs4_insert_string_aces.c
index 5a482d5..50b7bbf 100644
--- a/libnfs4acl/nfs4_insert_string_aces.c
+++ b/libnfs4acl/nfs4_insert_string_aces.c
@@ -45,21 +45,25 @@ int nfs4_insert_string_aces(struct nfs4_acl *acl, const char *acl_spec, unsigned
        if ((s = sp = strdup(acl_spec)) == NULL)
                goto out_failed;
 
+
        while ((ssp = strsep(&sp, ",\t\n\r")) != NULL) {
                if (!strlen(ssp))
                        continue;
 
-               if ((ace = nfs4_ace_from_string(ssp, acl->is_directory)) == NULL)
-                       goto out_failed;
+               if(*ssp == '#')
+                       printf("Skipping comment %s\n", ssp);
+               else {
+                       if ((ace = nfs4_ace_from_string(ssp, acl->is_directory)) == NULL)
+                               goto out_failed;
 
-               if (nfs4_insert_ace_at(acl, ace, index++)) {
-                       free(ace);
-                       goto out_failed;
+                       if (nfs4_insert_ace_at(acl, ace, index++)) {
+                               free(ace);
+                               goto out_failed;
+                       }
                }
        }
        if (acl->naces == 0)
                goto out_failed;
-
 out:
        if (s)
                free(s);

Here is the upstream posting

https://marc.info/?l=linux-nfs&m=155360947427957&w=2

which has not been committed to the upstream repo.

Verified in nfs4-acl-tools-0.3.3-20.el7

https://beaker.engineering.redhat.com/recipes/6801361#task91870379
(please ignore the failures due to lab servers' issue)
-----------------------------------------------------------------
# do-Test-RHEL-bz 1666850 - nfs4_setfacl error applying nfs4_getfacl output
[20:38:23 root@ ~~]# mount -t nfs4 localhost:/exportDir-nfs4_acl /nfsmp/nfsmp-nfs4_acl
[20:38:23 root@ ~~]# cat /proc/mounts | grep /nfsmp/nfsmp-nfs4_acl
localhost:/exportDir-nfs4_acl /nfsmp/nfsmp-nfs4_acl nfs4 rw,relatime,vers=4.1,rsize=524288,wsize=524288,namlen=255,hard,proto=tcp6,timeo=600,retrans=2,sec=sys,clientaddr=::1,local_lock=none,addr=::1 0 0
[20:38:23 root@ ~~]# touch /nfsmp/nfsmp-nfs4_acl/1 /nfsmp/nfsmp-nfs4_acl/2
[20:38:23 root@ ~~]# nfs4_getfacl /nfsmp/nfsmp-nfs4_acl/1 &> >(tee acl_spec.log)

# file: /nfsmp/nfsmp-nfs4_acl/1
A::OWNER@:rwatTcCy
A::GROUP@:rtcy
A::EVERYONE@:rtcy
[20:38:23 root@ ~~]# nfs4_setfacl -s "
# file: /nfsmp/nfsmp-nfs4_acl/1
A::OWNER@:rwatTcCy
A::GROUP@:rtcy
A::EVERYONE@:rtcy" /nfsmp/nfsmp-nfs4_acl/2
[20:38:23 root@ ~~]# nfs4_setfacl -S acl_spec.log /nfsmp/nfsmp-nfs4_acl/2
[20:38:23 root@ ~~]# rm -f /nfsmp/nfsmp-nfs4_acl/1 /nfsmp/nfsmp-nfs4_acl/2
[20:38:23 root@ ~~]# umount /nfsmp/nfsmp-nfs4_acl
------------------------------------------------------------------


Compared with previous version e.g. nfs4-acl-tools-0.3.3-19.el7

https://beaker.engineering.redhat.com/recipes/6725689#task90970467
------------------------------------------------------------------
[08:46:33 root@ ~~]# nfs4_getfacl /nfsmp/nfsmp-nfs4_acl/1 &> >(tee acl_spec.log)

# file: /nfsmp/nfsmp-nfs4_acl/1
A::OWNER@:rwatTcCy
A::GROUP@:rtcy
A::EVERYONE@:rtcy
[08:46:33 root@ ~~]# nfs4_setfacl -s "
# file: /nfsmp/nfsmp-nfs4_acl/1
A::OWNER@:rwatTcCy
A::GROUP@:rtcy
A::EVERYONE@:rtcy" /nfsmp/nfsmp-nfs4_acl/2
Failed while inserting ACE(s).
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [FAIL]
------------------------------------------------------------------

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2019:2306