kNFSD - Allowing rpc.nfsd to setting of the port, transport and version the server will use

Neil,

Here is a kernel patch that will enable the setting of the port knfsd will listens on, the transport knfsd will support and which NFS version will be advertised.

The nfs-utils patch, which is also attached, will added the following flags to rpc.nfsd that will enable the kernel functionality (Note: These patches are NOT dependent on each other. Meaning rpc.nfsd and knfsd will still function correctly if one or the other patch do or do not exist):

-N or --no-nfs-version vers
This option can be used to request that rpc.nfsd does not offer certain versions of NFS. The current version of rpc.nfsd can support both NFS version 2,3 and the newer version 4.

-T or --no-tcp
Disable rpc.nfsd from accepting TCP connections from clients.

-U or --no-udp
Disable rpc.nfsd from accepting UDP connections from clients.

Also Note: Both patches are in the latest release of the FC4 and Rawhide which has definitely helped in the testing of these patches.

Please consider the kernel patch for inclusion for both the -mm and mainline kernels.

steved.

This kernel patch allows the setting of the port and transport in which NFS server will listen on, as well as, which NFS protocol version will be advertised.

Signed-off: Steve Dickson <[email protected]>
-----------------------------------------

@@ -3319,6 +3319,9 @@ __nfs4_state_shutdown(void)
 void
 nfs4_state_shutdown(void)
 {
+	if (!nfs4_init)
+		return;
+
 	nfs4_lock_state();
 	nfs4_release_reclaim();
 	__nfs4_state_shutdown();

@@ -39,6 +39,22 @@
 #define NFSCTL_GETFD		7	/* get an fh by path (used by mountd) */
 #define	NFSCTL_GETFS		8	/* get an fh by path with max FH len */
 
+/*
+ * Macros used to set version and protocol
+ */
+#define NFSCTL_VERSET(_cltbits, _v)   ((_cltbits) |=  (1 << ((_v) - 1)))
+#define NFSCTL_VERUNSET(_cltbits, _v) ((_cltbits) &= ~(1 << ((_v) - 1)))
+#define NFSCTL_VERISSET(_cltbits, _v) ((_cltbits) & (1 << ((_v) - 1)))
+
+#define NFSCTL_UDPSET(_cltbits)       ((_cltbits) |=  (1 << (17 - 1)))
+#define NFSCTL_UDPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (17 - 1)))
+#define NFSCTL_UDPISSET(_cltbits)     ((_cltbits) & (1 << (17 - 1)))
+
+#define NFSCTL_TCPSET(_cltbits)       ((_cltbits) |=  (1 << (18 - 1)))
+#define NFSCTL_TCPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (18 - 1)))
+#define NFSCTL_TCPISSET(_cltbits)     ((_cltbits) & (1 << (18 - 1)))
+
+
 /* SVC */
 struct nfsctl_svc {
 	unsigned short		svc_port;

@@ -120,6 +136,9 @@ extern int		exp_delclient(struct nfsctl_
 extern int		exp_export(struct nfsctl_export *nxp);
 extern int		exp_unexport(struct nfsctl_export *nxp);
 
+extern int nfsd_port;
+extern unsigned int nfsd_versbits, nfsd_portbits;
+
 #endif /* __KERNEL__ */
 
 #endif /* NFSD_SYSCALL_H */

This patch adds the following command line arguments to rpc.nfsd:

-N or --no-nfs-version vers
This option can be used to request that rpc.nfsd does not offer certain versions of NFS. The current version of rpc.nfsd can support both NFS version 2,3 and the newer version 4.

-T or --no-tcp
Disable rpc.nfsd from accepting TCP connections from clients.

-U or --no-udp
Disable rpc.nfsd from accepting UDP connections from clients.

---------

@@ -10,6 +10,9 @@
 #define	NFS3_FHSIZE	64
 #define	NFS_FHSIZE	32
 
+#define NFSD_MINVERS 2
+#define NFSD_MAXVERS 4
+
 struct nfs_fh_len {
 	int		fh_size;
 	u_int8_t	fh_handle[NFS3_FHSIZE];

@@ -40,7 +43,15 @@ struct nfs_fh_old {
 #define NFSCTL_LOCKD		0x10000
 #define LOCKDCTL_SVC		NFSCTL_LOCKD
 
+#define NFSCTL_VERUNSET(_cltbits, _v) ((_cltbits) &= ~(1 << ((_v) - 1))) 
+#define NFSCTL_UDPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (17 - 1))) 
+#define NFSCTL_TCPUNSET(_cltbits)     ((_cltbits) &= ~(1 << (18 - 1))) 
+
+#define NFSCTL_VERISSET(_cltbits, _v) ((_cltbits) & (1 << ((_v) - 1))) 
+#define NFSCTL_UDPISSET(_cltbits)     ((_cltbits) & (1 << (17 - 1))) 
+#define NFSCTL_TCPISSET(_cltbits)     ((_cltbits) & (1 << (18 - 1))) 
 
+#define NFSCTL_ALLBITS (~0)
 
 /* SVC */
 struct nfsctl_svc {

@@ -118,7 +118,7 @@ int			wildmat(char *text, char *pattern)
  * nfsd library functions.
  */
 int			nfsctl(int, struct nfsctl_arg *, union nfsctl_res *);
-int			nfssvc(int port, int nrservs);
+int			nfssvc(int port, int nrservs, unsigned int versbits, unsigned int portbits);
 int			nfsaddclient(struct nfsctl_client *clp);
 int			nfsdelclient(struct nfsctl_client *clp);
 int			nfsexport(struct nfsctl_export *exp);

@@ -6,7 +6,7 @@
 .SH NAME
 rpc.nfsd \- NFS server process
 .SH SYNOPSIS
-.BI "/usr/sbin/rpc.nfsd [-p " port "] " nproc
+.BI "/usr/sbin/rpc.nfsd [" options "]" " "nproc
 .SH DESCRIPTION
 The
 .B rpc.nfsd

@@ -22,11 +22,28 @@ server provides an ancillary service nee
 by NFS clients.
 .SH OPTIONS
 .TP
-.BI \-p " port"
+.B \-p " or " \-\-port  port
 specify a diferent port to listen on for NFS requests. By default,
 .B rpc.nfsd
 will listen on port 2049.
 .TP
+.B \-N " or " \-\-no-nfs-version vers
+This option can be used to request that 
+.B rpc.nfsd
+does not offer certain versions of NFS. The current version of
+.B rpc.nfsd
+can support both NFS version 2,3 and the newer version 4.
+.TP
+.B \-T " or " \-\-no-tcp
+Disable 
+.B rpc.nfsd 
+from accepting TCP connections from clients.
+.TP
+.B \-U " or " \-\-no-udp
+Disable
+.B rpc.nfsd
+from accepting UDP connections from clients.
+.TP
 .I nproc
 specify the number of NFS server threads. By default, just one
 thread is started. However, for optimum performance several threads

So the obvious question is what will happen if someone does

rpc.nfsd -N 3

on a server supporting 2, 3, and 4.

It looks like the code in svc_create() will set pg_lovers to 2 and pg_hivers to 4 in that case. So if someone tries to use version 3, the error they get back will be a somewhat contradictory "sorry, I only support versions 2 through 4."

It seems to me that it'd be cleaner if the kernel interface only accepted a range (e.g., "2--4" or "2--3"). Then if someone attempted the above, they'll get an error back immediately.

Or svc_create could be adjusted to report a more conservative range ("2--2" or "4--4" instead of "2--4").

But I don't have really strong feelings about it. Maybe we shouldn't care enough about that case.

--b.

Hey Bruce,

hmmm.... good point... But I wonder if this would be better handled in rpc.nfsd process; Similar to what happens when no transports are specified (i.e. both -T and -U flags are set), the server fails to started.

steved.

Well, you are right, it seems contradictory, but it is the right thing to do in such a situation. It follows the RPC semantics as defined. (I suspect that Bob didn't consider something like this when he was designing this part of RPC.)

I would suggest that doing this to an NFS server would be pretty silly, not done very much if at all, and not something to care about.

It also isn't the first time that such a situation has occurred. SGI used to have a MOUNT version 99 (or some such value) and responses such as this could be seen from their server at times.

Thanx...

ps

Thanks.
The 'version' bit is mostly OK. The 'port' bit I don't like at all :-(

I've taken the 'version' bit, modified is so:
1/ you can read back the current setting (needs an extra patch to
restore the ability to read without writing)
By looking at this, you can see which versions the kernel supports, and which are enabled.
2/ You can only change the setting when there are no active threads.

New version this part patch follows. It should be completely compatible with your patch. from a user-space perspective.

The 'port' bit I had trouble liking.
You write:

family proto proto addr port

to the 'ports' file.
'family' and 'addr' are completely ignored.
'port' is effectively ignored (value is stored in a variable which isn't used).

That leaves 'proto' and 'proto'. One should be 'tcp' or 'notcp', the other should be 'udp' or 'noudp'. Which is which? Udp comes first, but it isn't at all obvious from the interface..

If you want an interface like this, I think you should write:

[+-]family proto addr port

and every field must be checked and used.
So while we only support ipv4, the 'family' must by 'ipv4' or an error is returned.
'+' adds an endpoint. '-' removes it.

The old nfssvc syscall should add 'ipv4 udp * %port' and 'ipv4 tcp * %port' if they don't already exist.

An alternate interface, which would be quite appealing, would be to require the user-space program to create and bind a socket and then communicate it to the kernel, possibly by writing a file-descriptor number to a file in the nfsd filesystem.
'nfsd' would check it is an appropriate type of socket, take an extra reference, and use it.
This would probably be best done *after* the nfsd threads were started, so there would need to be a way to start threads without them automatically opening sockets. I'm not sure what the best interface for that would be... Maybe establishing sockets before the thread would be ok.

NeilBrown

----
Two patches.
First restores ability to read from and 'nfsd' file without first writing.
Second provides 'version' control file.

Restore functionality to read from file in /proc/fs/nfsd/

Most files in the nfsd filesystems are transaction files. You write a request, and read a response.
For some (e.g. 'threads') it makes sense to just be able to read and get the current value.
This functionality did exist but was broken recently when someone modified nfsctl.c without going through the maintainer. This patch fixes the regression.

### Diffstat output

 ./fs/nfsd/nfsctl.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff ./fs/nfsd/nfsctl.c~current~ ./fs/nfsd/nfsctl.c
@@ -104,9 +104,23 @@ static ssize_t nfsctl_transaction_write(
 	return rv;
 }
 
+static ssize_t nfsctl_transaction_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
+{
+	if (! file->private_data) {
+		/* An attempt to read a transaction file without writing
+		 * causes a 0-byte write so that the file can return
+		 * state information
+		 */
+		ssize_t rv = nfsctl_transaction_write(file, buf, 0, pos);
+		if (rv < 0)
+			return rv;
+	}
+	return simple_transaction_read(file, buf, size, pos);
+}
+
 static struct file_operations transaction_ops = {
 	.write		= nfsctl_transaction_write,
-	.read		= simple_transaction_read,
+	.read		= nfsctl_transaction_read,
 	.release	= simple_transaction_release,
 };
 

Allow run-time selection of NFS versions to export

Provide a file in the NFSD filesystem that allows setting and querying of which version of NFS are being exported. Changes are only allowed while no server is running.

### Diffstat output

 ./fs/nfsd/nfsctl.c             |   70 ++++++++++++++++++++++++++++++++++++
 ./fs/nfsd/nfssvc.c             |   79 ++++++++++++++++++++++++++---------------
 ./include/linux/nfsd/nfsd.h    |    2 -
 ./include/linux/nfsd/syscall.h |   17 ++++++++
 4 files changed, 139 insertions(+), 29 deletions(-)

diff ./include/linux/nfsd/nfsd.h~current~ ./include/linux/nfsd/nfsd.h
@@ -60,7 +60,7 @@ typedef int (*nfsd_dirop_t)(struct inode
 extern struct svc_program	nfsd_program;
 extern struct svc_version	nfsd_version2, nfsd_version3,
 				nfsd_version4;
-
+extern struct svc_serv		*nfsd_serv;
 /*
  * Function prototypes.
  */

diff ./include/linux/nfsd/syscall.h~current~ ./include/linux/nfsd/syscall.h
@@ -39,6 +39,21 @@
 #define NFSCTL_GETFD		7	/* get an fh by path (used by mountd) */
 #define	NFSCTL_GETFS		8	/* get an fh by path with max FH len */
 
+/*
+ * Macros used to set version
+ */
+#define NFSCTL_VERSET(_cltbits, _v)   ((_cltbits) |=  (1 << (_v)))
+#define NFSCTL_VERUNSET(_cltbits, _v) ((_cltbits) &= ~(1 << (_v)))
+#define NFSCTL_VERISSET(_cltbits, _v) ((_cltbits) & (1 << (_v)))
+
+#if defined(CONFIG_NFSD_V4)
+#define	NFSCTL_VERALL	(0x1c /* 0b011100 */)
+#elif defined(CONFIG_NFSD_V3)
+#define	NFSCTL_VERALL	(0x0c /* 0b001100 */)
+#else
+#define	NFSCTL_VERALL	(0x04 /* 0b000100 */)
+#endif
+
 /* SVC */
 struct nfsctl_svc {
 	unsigned short		svc_port;

@@ -120,6 +135,8 @@ extern int		exp_delclient(struct nfsctl_
 extern int		exp_export(struct nfsctl_export *nxp);
 extern int		exp_unexport(struct nfsctl_export *nxp);
 
+extern unsigned int nfsd_versbits;
+
 #endif /* __KERNEL__ */
 
 #endif /* NFSD_SYSCALL_H */

Hey Neil,

Thanks for taking a look at this...

Good point...

cool... thanks for thinking about this...

Well at this point they are not needed since we only support ipv4... and I can only assume when we do support other families, changing this interface will be the least of our problems... ;-)

I'm not sure I understand this... nfsd_port is set and used in nfsd_svc()
@@ -104,11 +152,14 @@ nfsd_svc(unsigned short port, int nrserv
nfsd_serv = svc_create(&nfsd_program, NFSD_BUFSIZE); if (nfsd_serv == NULL)
goto out;
+ if (NFSCTL_UDPISSET(nfsd_portbits))
+ port = nfsd_port;
error = svc_makesock(nfsd_serv, IPPROTO_UDP, port); if (error < 0)
goto failure;
#ifdef CONFIG_NFSD_TCP
+ if (NFSCTL_TCPISSET(nfsd_portbits))
+ port = nfsd_port;
error = svc_makesock(nfsd_serv, IPPROTO_UDP, port); if (error < 0)
goto failure;

iff the nfsd_portbits are set... which is exactly the same concept used with the version bits... so I am a bit confused on what you want...

Maybe being the author of these interfaces you have a better perspective than I, but I could say the same thing about all the interfaces under /proc/fs/nfsd... :-) So I do agree... its not that obvious, but I guess I didn't think it needed to be.. Who else do you see, other than rpc.nfsd, using this interface?

Understood... And I'll assume the default of both TCP and UDP listening on port 2048 will stay the same when nothing is specified...

Won't this break backwards compatibility with all the 2.4 kernels? The beauty of seq files is that you can change the interface and have no effect on kABI at all... which is really a good thing in my world... So why do we even care about the old syscall interface?

Maybe I'm missing something here.... but I'm not quite sure how passing
a fd to the kernel would help, other than (possibly) with error processing... The kernel will still need to know the port and proto so it can register them with the portmapper... plus permissions could
become an problem especially with things like selinux running around..

steved.

Maybe, but either we make it ready for future needs now, or we only support current needs and allow a clear upgrade path. Ignoring fields which might later have a meaning is bad because it means that user-space code which works now much break later for no good reason.
My basic position wrt this is that if the fields are there, they should be checked.

Uhmmmm.... that code fragment isn't in the patch you sent. I got:

Ok, I guess I presented by difficulty with this rather badly. The order does seem arbitrary and non-obvious, but you are right that other things are no-obvious too...
The point I should have made was that it is non-extensible. Suppose NFSv4.4 is defined to work over SCTP to even DCCP (which might be ideal for NFS), how would we specify those?

Yes... but I'm not sure of the rest of the rules about defaults...
- should you need to explicitly remove the defaults if you don't want them?
- should there be an easy way to revert-to-defaults? Maybe when the last nfsd dies, all setting are forgotten (just like currently all exports are removed).

I don't see why it would break backwards compat. Isn't that exactly what the nfssvc syscall does? You give it a port number, and it starts the number of threads with two sockets, a udp bound to ADDR_ANY and the given port, and a TCP bound the same way? What is what I meant to say above.

I think the principle of "do as much as possible in user-space" is a good one.
socket / bind / pmap_register
can all be done in user-space, so maybe they should be. You could possible even argue that
listen / accept
the be done in user-space, and so should be, but I'm not sure I want to push it that far.

The nice thing about just passing down a filedescriptor for a socket is that it allows for any future additions of protocols.

I don't see how permissions could be a problem, but then I know very little about selinux, so maybe there is something. Is there something specific you foresee, or is it just general concern?

NeilBrown