Bug 2042362

(In reply to Dave Wysochanski from comment #0)
> Description of problem:
> I'm entering this bug because a customer hit this when using our nfshelper
> app, and also it was discussed on an internal nfs team call.  Customer
> wanted to enable NFSv4.2 only, and used the nfshelper "lab" on the portal
> (https://access.redhat.com/labs/nfshelper/).  On the nfshelper app, there
> was a checkbox for NFS4 as well as NFS4.2.  The customer checked 4.2 but not
> 4, and got something like this when they ran the setup script for the nfs
> server:
> ~~~
> # grep -v ^# /etc/nfs.conf
> ...
> [nfsd]
> vers2 = n
> vers3 = n
> vers4 = n
> vers4.2 = y
> ~~~
> 
> Then they tried to start nfs-server and got a weird mountd error they didn't
> understand:
> ~~~
> # systemctl start nfs-server
> A dependency job for nfs-server.service failed. See 'journalctl -xe' for
> details.
> # journalctl -xe
> ░░ The job identifier is 1153.
> Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]: rpc.mountd: No
> protocol versions s>
^^^^ problem is specified ^^^

It just the usage messages kinda confuses things... 
 
> Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]: Usage: rpc.mountd
> [-F|--foreground> 
> Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:        
> [-l|--log-auth] [-i|--cach>
> Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-o
> num|--descriptors num]
> Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-p|--port
> port] [-V versi>
> Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-N
> version|--no-nfs-versi>
> Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-H prog
> |--ha-callout pro>
> Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:        
> [-s|--state-directory-path>
> Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-t
> num|--num-threads=num]>
> Jan 19 05:24:23 nfs-server.example.com systemd[1]: nfs-mountd.service:
> Control process exit>

I guess we could turn off the usage message....

(In reply to Steve Dickson from comment #1)
> (In reply to Dave Wysochanski from comment #0)
> > Description of problem:
> > I'm entering this bug because a customer hit this when using our nfshelper
> > app, and also it was discussed on an internal nfs team call.  Customer
> > wanted to enable NFSv4.2 only, and used the nfshelper "lab" on the portal
> > (https://access.redhat.com/labs/nfshelper/).  On the nfshelper app, there
> > was a checkbox for NFS4 as well as NFS4.2.  The customer checked 4.2 but not
> > 4, and got something like this when they ran the setup script for the nfs
> > server:
> > ~~~
> > # grep -v ^# /etc/nfs.conf
> > ...
> > [nfsd]
> > vers2 = n
> > vers3 = n
> > vers4 = n
> > vers4.2 = y
> > ~~~
> > 
> > Then they tried to start nfs-server and got a weird mountd error they didn't
> > understand:
> > ~~~
> > # systemctl start nfs-server
> > A dependency job for nfs-server.service failed. See 'journalctl -xe' for
> > details.
> > # journalctl -xe
> > ░░ The job identifier is 1153.
> > Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]: rpc.mountd: No
> > protocol versions s>
> ^^^^ problem is specified ^^^
> 
> It just the usage messages kinda confuses things... 
>  
Not really.  The customer didn't know what was wrong because there's no checking of incompatible options in the config file.


> > Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]: Usage: rpc.mountd
> > [-F|--foreground> 
> > Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:        
> > [-l|--log-auth] [-i|--cach>
> > Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-o
> > num|--descriptors num]
> > Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-p|--port
> > port] [-V versi>
> > Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-N
> > version|--no-nfs-versi>
> > Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-H prog
> > |--ha-callout pro>
> > Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:        
> > [-s|--state-directory-path>
> > Jan 19 05:24:23 nfs-server.example.com rpc.mountd[1419]:         [-t
> > num|--num-threads=num]>
> > Jan 19 05:24:23 nfs-server.example.com systemd[1]: nfs-mountd.service:
> > Control process exit>
> 
> I guess we could turn off the usage message....

If you did that then how would that solve the underlying issue of a customer not knowing they entered an incompatible option / turned off all NFS versions in /etc/nfs.conf?

The customer's /etc/nfs.conf ended up like this:
[nfsd]
vers2 = n
vers3 = n
vers4 = n
vers4.2 = y

And the customer expected NFS4.2 only to be enabled, but it was not.

(In reply to Dave Wysochanski from comment #2)
Taking a second look at this...

> The customer's /etc/nfs.conf ended up like this:
> [nfsd]
> vers2 = n
> vers3 = n
> vers4 = n
> vers4.2 = y
The problem here is "vers4 = n". It turns off all v4 versions
because the parsing code do not look at the .x part of v4
in the mountd code but in the nfsd code that part is looked at.

But if you due something like 

#vers4=n
vers4.0=n
vers4.1=n
vers4.2=y

or even something like 
#vers4=n
vers4.2=y

The v4 version is interrupted as not being set, 
so the default is used... luckily that is on.

Now the rpc.nfsd(8) man page does say 
   vers4  Enable or disable a major NFS version.

Maybe it should say "NFS versions"? So maybe this is
just a documentation issue?

(In reply to Steve Dickson from comment #4)
> (In reply to Dave Wysochanski from comment #2)
> Taking a second look at this...
> 
> > The customer's /etc/nfs.conf ended up like this:
> > [nfsd]
> > vers2 = n
> > vers3 = n
> > vers4 = n
> > vers4.2 = y
> The problem here is "vers4 = n". It turns off all v4 versions
> because the parsing code do not look at the .x part of v4
> in the mountd code but in the nfsd code that part is looked at.
> 
> But if you due something like 
> 
> #vers4=n
> vers4.0=n
> vers4.1=n
> vers4.2=y
> 
> or even something like 
> #vers4=n
> vers4.2=y
> 
> The v4 version is interrupted as not being set, 
> so the default is used... luckily that is on.
> 
> Now the rpc.nfsd(8) man page does say 
>    vers4  Enable or disable a major NFS version.
> 
> Maybe it should say "NFS versions"? So maybe this is
> just a documentation issue?

Steve thanks for looking at this.  I think it's fine to improve documentation, and arguably this is a low priority bug.
Can we do better though?  Just a documentation update does not clue in the customer to the fact they've misconfigured something.
Can't we at least throw a warning if this happens?

See comment #0 

Expected results:
Better error message pointing customer to the problem in /etc/nfs.conf

Steved - take a look at nfsd.c:

118         for (i = 2; i <= 4; i++) {
119                 char tag[20];
120                 sprintf(tag, "vers%d", i);
121                 if (conf_get_bool("nfsd", tag, NFSCTL_VERISSET(versbits, i))) {
122                         NFSCTL_VERSET(versbits, i);
123                         if (i == 4)
124                                 minorvers = minorversset = minormask;
125                 } else {
126                         NFSCTL_VERUNSET(versbits, i);
127                         if (i == 4) {
128                                 minorvers = 0;
129                                 minorversset = minormask;
130                         }
131                 }
132         }
133 
134         /* We assume the kernel will default all minor versions to 'on',
135          * and allow the config file to disable some.
136          */
137         for (i = NFS4_MINMINOR; i <= NFS4_MAXMINOR; i++) {
138                 char tag[20];
139                 sprintf(tag, "vers4.%d", i);
140                 /* The default for minor version support is to let the
141                  * kernel decide.  We could ask the kernel what that choice
142                  * will be, but that is needlessly complex.
143                  * Instead, perform a config-file lookup using each of the
144                  * two possible default.  If the result is different from the
145                  * default, then impose that value, else don't make a change
146                  * (i.e. don't set the bit in minorversset).
147                  */
148                 if (!conf_get_bool("nfsd", tag, 1)) {
149                         NFSCTL_MINORSET(minorversset, i);
150                         NFSCTL_MINORUNSET(minorvers, i);
151                         if (i == 0)
152                                 force4dot0 = 1;
153                 }
154                 if (conf_get_bool("nfsd", tag, 0)) {
155                         NFSCTL_MINORSET(minorversset, i);
156                         NFSCTL_MINORSET(minorvers, i);
157                         if (i == 0)
158                                 force4dot0 = 1;
159                 }
160         }
161 
"./nfsd/nfsd.c" 426 lines --16%--    


Now look at mountd.c
670 inline static void
671 read_mountd_conf(char **argv)
672 {
673         char    *s;
674         int     ttl;
675 
676         conf_init_file(NFS_CONFFILE);
677 
678         xlog_set_debug("mountd");
679         manage_gids = conf_get_bool("mountd", "manage-gids", manage_gids);
680         descriptors = conf_get_num("mountd", "descriptors", descriptors);
681         port = conf_get_num("mountd", "port", port);
682         num_threads = conf_get_num("mountd", "threads", num_threads);
683         reverse_resolve = conf_get_bool("mountd", "reverse-lookup", reverse_resolve);
684         ha_callout_prog = conf_get_str("mountd", "ha-callout");
685         if (conf_get_bool("mountd", "cache-use-ipaddr", 0))
686                 use_ipaddr = 2;
687 
688         s = conf_get_str("mountd", "state-directory-path");
689         if (s && !state_setup_basedir(argv[0], s))
690                 exit(1);
691 
692         /* NOTE: following uses "nfsd" section of nfs.conf !!!! */
693         if (conf_get_bool("nfsd", "udp", NFSCTL_UDPISSET(_rpcprotobits)))
694                 NFSCTL_UDPSET(_rpcprotobits);
695         else
696                 NFSCTL_UDPUNSET(_rpcprotobits);
697         if (conf_get_bool("nfsd", "tcp", NFSCTL_TCPISSET(_rpcprotobits)))
698                 NFSCTL_TCPSET(_rpcprotobits);
699         else
700                 NFSCTL_TCPUNSET(_rpcprotobits);
701         for (vers = 2; vers <= 4; vers++) {
702                 char tag[20];
703                 sprintf(tag, "vers%d", vers);
704                 if (conf_get_bool("nfsd", tag, NFSCTL_VERISSET(nfs_version, vers)))
705                         NFSCTL_VERSET(nfs_version, vers);
706                 else
707                         NFSCTL_VERUNSET(nfs_version, vers);
708         }
709 
710         ttl = conf_get_num("mountd", "ttl", default_ttl);
711         if (ttl > 0)
712                 default_ttl = ttl;
713 }
714 


Seems like mountd.c is missing some parsing logic for the minor versions?
Maybe parse them, look for the conflict, and throw an error to the user is all we need?

Untested, incomplete but here's one approach:

--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -698,7 +698,7 @@ read_mountd_conf(char **argv)
                NFSCTL_TCPSET(_rpcprotobits);
        else
                NFSCTL_TCPUNSET(_rpcprotobits);
-       for (vers = 2; vers <= 4; vers++) {
+       for (vers = 2; vers <= 3; vers++) {
                char tag[20];
                sprintf(tag, "vers%d", vers);
                if (conf_get_bool("nfsd", tag, NFSCTL_VERISSET(nfs_version, vers)))
@@ -706,6 +706,10 @@ read_mountd_conf(char **argv)
                else
                        NFSCTL_VERUNSET(nfs_version, vers);
        }
+       if (conf_get_bool("nfsd", "vers4", NFSCTL_VERISSET(nfs_version, 4)))
+                       NFSCTL_VERSET(nfs_version, vers);
+       else { /* check minor versions */
+       }
 
        ttl = conf_get_num("mountd", "ttl", default_ttl);
        if (ttl > 0)

Created attachment 1911847 [details]
Very lightly tested patch

(In reply to Dave Wysochanski from comment #9)
> Created attachment 1911847 [details]
> Very lightly tested patch

The error is coming for rpc.nfsd not mountd... 
But I did see the difference, mountd does 
not deal with 4.x versions. 

Since we no longer support v2, we should
rework code in both the command and deamon

upstream posting

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

commit d4de031fbb2d797ec9e738deda50feec97db7593 (HEAD -> master, tag: nfs-utils-2-6-3-rc3, origin/master, origin/HEAD)
Author: Steve Dickson <steved>
Date:   Fri Oct 28 10:56:36 2022 -0400

    nfsd.man: Explain that setting nfsv4=n turns off all v4 versions
    
    Update the man page to explicitly say setting
    nfsv4=n turns off all v4 versions

The man page has already been updated now:

[root@hp-dl388g8-20 ~]# rpm -q nfs-utils
nfs-utils-2.5.4-16.el9.x86_64
[root@hp-dl388g8-20 ~]# man nfsd
[root@hp-dl388g8-20 ~]# man nfsd | grep vers4
       vers4  Enable or disable all NFSv4 versions.  All versions are normally enabled by default.
       vers4.1
       vers4.2
[root@hp-dl388g8-20 ~]#

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 (nfs-utils bug fix and enhancement update), 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-2023:2510