[PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[NeilBrown]

V1 of this series didn't update the usage() message for mountd,
and omited the required ':' after the 'T' sort-option.  This 
series fixes those two omissions.

Original series comment:

When NFSv3 is used mountd provides logs of successful and failed mount
attempts which can be used for auditing.
When NFSv4 is used there are no such logs as NFSv4 does not have a
distinct "mount" request.

However mountd still knows about which filesysytems are being accessed
from which clients, and can actually provide more reliable logs than it
currently does, though they must be more verbose - with periodic "is
being accessed" message replacing a single "was mounted" message.

This series adds support for that logging, and adds some related
improvements to make the logs as useful as possible.

NeilBrown

---

NeilBrown (5):
      mountd: reject unknown client IP when !use_ipaddr.
      mountd: Don't proactively add export info when fh info is requested.
      mountd: add logging for authentication results for accesses.
      mountd: add --cache-use-ipaddr option to force use_ipaddr
      mountd: make default ttl settable by option


 support/export/auth.c      |  4 +++
 support/export/cache.c     | 32 +++++++++++------
 support/export/v4root.c    |  3 +-
 support/include/exportfs.h |  3 +-
 support/nfs/exports.c      |  4 ++-
 utils/mountd/mountd.c      | 30 +++++++++++++++-
 utils/mountd/mountd.man    | 70 ++++++++++++++++++++++++++++++++++++++
 7 files changed, 131 insertions(+), 15 deletions(-)

--
Signature

[PATCH 1/5] mountd: reject unknown client IP when !use_ipaddr.

[NeilBrown]

From: NeilBrown <neil@brown.name>

When use_ipaddr is not in effect, an auth_unix_ip lookup request from
the kernel for an unknown client will be rejected.
When it IS in effect, these requests are always granted with the IP
address being mapped to a string form of the address, preceded by a '$'.

This is inconsistent behaviour and could present a small information
leak.
It means that, for example, a SETCLIENT NFSv4 request may or may not
succeed depending on an internal setting in rpc.mountd.

This is easily rectified by always checking if the client is known.

Signed-off-by: NeilBrown <neil@brown.name>
---
 support/export/cache.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/support/export/cache.c b/support/export/cache.c
index f1569afb558c..156ebfd4087c 100644
--- a/support/export/cache.c
+++ b/support/export/cache.c
@@ -114,6 +114,7 @@ static void auth_unix_ip(int f)
 	char class[20];
 	char ipaddr[INET6_ADDRSTRLEN + 1];
 	char *client = NULL;
+	struct addrinfo *ai = NULL;
 	struct addrinfo *tmp = NULL;
 	char buf[RPC_CHAN_BUF_SIZE], *bp;
 	int blen;
@@ -139,21 +140,17 @@ static void auth_unix_ip(int f)
 
 	auth_reload();
 
-	/* addr is a valid, interesting address, find the domain name... */
-	if (!use_ipaddr) {
-		struct addrinfo *ai = NULL;
-
-		ai = client_resolve(tmp->ai_addr);
-		if (ai) {
-			client = client_compose(ai);
-			nfs_freeaddrinfo(ai);
-		}
+	/* addr is a valid address, find the domain name... */
+	ai = client_resolve(tmp->ai_addr);
+	if (ai) {
+		client = client_compose(ai);
+		nfs_freeaddrinfo(ai);
 	}
 	bp = buf; blen = sizeof(buf);
 	qword_add(&bp, &blen, "nfsd");
 	qword_add(&bp, &blen, ipaddr);
 	qword_adduint(&bp, &blen, time(0) + DEFAULT_TTL);
-	if (use_ipaddr) {
+	if (use_ipaddr && client) {
 		memmove(ipaddr + 1, ipaddr, strlen(ipaddr) + 1);
 		ipaddr[0] = '$';
 		qword_add(&bp, &blen, ipaddr);

[PATCH 2/5] mountd: Don't proactively add export info when fh info is requested.

[NeilBrown]

From: NeilBrown <neil@brown.name>

When an "nfsd.fh" request is received from the kernel, we map the
file-handle prefix to a path name and report that (as required) and then
also add "nfsd.export" information with export flags applicable to that
path.

This is not necessary and was added as a perceived optimisation.
When updating data already in the kernel, it is unlikely to help as the
kernel can be expected to ask for both details at much the same time.
With NFSv3, new information is normally added by a MOUNT rpc request, so
this is irrelevant.
With NFSv4, the kernel requests the "nfsd.export" information when
walking down from ROOT, *before* requesting the nfsd.fh information, so
this "optimisation" causes unnecessary work.

A future patch will add logging of authentication requests, and this
double-handling would result in extra unnecessary log messages.

As this "optimisation" appears to have no practical value and some
(small) cost, let's remove it.

Signed-off-by: NeilBrown <neil@brown.name>
---
 support/export/cache.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/support/export/cache.c b/support/export/cache.c
index 156ebfd4087c..49a761749ec6 100644
--- a/support/export/cache.c
+++ b/support/export/cache.c
@@ -96,7 +96,6 @@ static bool path_lookup_error(int err)
  * Record is terminated with newline.
  *
  */
-static int cache_export_ent(char *buf, int buflen, char *domain, struct exportent *exp, char *path);
 
 #define INITIAL_MANAGED_GROUPS 100
 
@@ -870,18 +869,13 @@ static void nfsd_fh(int f)
 	    !is_mountpoint(found->e_mountpoint[0]?
 			   found->e_mountpoint:
 			   found->e_path)) {
-		/* Cannot export this yet 
+		/* Cannot export this yet
 		 * should log a warning, but need to rate limit
 		   xlog(L_WARNING, "%s not exported as %d not a mountpoint",
 		   found->e_path, found->e_mountpoint);
 		 */
 		/* FIXME we need to make sure we re-visit this later */
 		goto out;
-	} else if (cache_export_ent(buf, sizeof(buf), dom, found, found_path) < 0) {
-		if (!path_lookup_error(errno))
-			goto out;
-		/* The kernel is saying the path is unexportable */
-		found = NULL;
 	}
 
 	bp = buf; blen = sizeof(buf);

[PATCH 4/5] mountd: add --cache-use-ipaddr option to force use_ipaddr

[NeilBrown]

From: NeilBrown <neil@brown.name>

When logging authentication requests, it can be easier to read the logs
if clients are always identified by IP address, not intermediate names
like netgroups or subnets.

To allow this, add --cache-use-ipaddr or -i which tell mountd to always
enable use_ipaddr.

Signed-off-by: NeilBrown <neil@brown.name>
---
 support/export/auth.c   |    4 ++++
 utils/mountd/mountd.c   |   10 ++++++++--
 utils/mountd/mountd.man |   18 ++++++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/support/export/auth.c b/support/export/auth.c
index 0bfa77d18469..cea376300d01 100644
--- a/support/export/auth.c
+++ b/support/export/auth.c
@@ -66,6 +66,10 @@ check_useipaddr(void)
 	int old_use_ipaddr = use_ipaddr;
 	unsigned int len = 0;
 
+	if (use_ipaddr > 1)
+		/* fixed - don't check */
+		return;
+
 	/* add length of m_hostname + 1 for the comma */
 	for (clp = clientlist[MCL_NETGROUP]; clp; clp = clp->m_next)
 		len += (strlen(clp->m_hostname) + 1);
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 9fecf2f04c3b..b9260aeb86a3 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -75,9 +75,10 @@ static struct option longopts[] =
 	{ "manage-gids", 0, 0, 'g' },
 	{ "no-udp", 0, 0, 'u' },
 	{ "log-auth", 0, 0, 'l'},
+	{ "cache-use-ipaddr", 0, 0, 'i'},
 	{ NULL, 0, 0, 0 }
 };
-static char shortopts[] = "o:nFd:p:P:hH:N:V:vurs:t:gl";
+static char shortopts[] = "o:nFd:p:P:hH:N:V:vurs:t:gli";
 
 #define NFSVERSBIT(vers)	(0x1 << (vers - 1))
 #define NFSVERSBIT_ALL		(NFSVERSBIT(2) | NFSVERSBIT(3) | NFSVERSBIT(4))
@@ -681,6 +682,8 @@ read_mountd_conf(char **argv)
 	num_threads = conf_get_num("mountd", "threads", num_threads);
 	reverse_resolve = conf_get_bool("mountd", "reverse-lookup", reverse_resolve);
 	ha_callout_prog = conf_get_str("mountd", "ha-callout");
+	if (conf_get_bool("mountd", "cache-use-ipaddr", 0))
+		use_ipaddr = 2;
 
 	s = conf_get_str("mountd", "state-directory-path");
 	if (s && !state_setup_basedir(argv[0], s))
@@ -803,6 +806,9 @@ main(int argc, char **argv)
 		case 'l':
 			xlog_sconfig("auth", 1);
 			break;
+		case 'i':
+			use_ipaddr = 2;
+			break;
 		case 0:
 			break;
 		case '?':
@@ -918,7 +924,7 @@ usage(const char *prog, int n)
 {
 	fprintf(stderr,
 "Usage: %s [-F|--foreground] [-h|--help] [-v|--version] [-d kind|--debug kind]\n"
-"	[-l|--log-auth]\n"
+"	[-l|--log-auth] [-i|--cache-use-ipaddr]\n"
 "	[-o num|--descriptors num]\n"
 "	[-p|--port port] [-V version|--nfs-version version]\n"
 "	[-N version|--no-nfs-version version] [-n|--no-tcp]\n"
diff --git a/utils/mountd/mountd.man b/utils/mountd/mountd.man
index df4e5356cb05..44d237e56110 100644
--- a/utils/mountd/mountd.man
+++ b/utils/mountd/mountd.man
@@ -118,6 +118,23 @@ section.
 will always log authentication responses to MOUNT requests when NFSv3 is
 used, but to get similar logs for NFSv4, this option is required.
 .TP
+.BR \-i " or " \-\-cache\-use\-ipaddr
+Normally each client IP address is matched against each host identifier
+(name, wildcard, netgroup etc) found in
+.B /etc/exports
+and a combined identity is formed from all matching identifiers.
+Often many clients will map to the same combined identity so performing
+this mapping reduces the number of distinct access details that the
+kernel needs to store.
+Specifying the
+.B \-i
+option suppresses this mapping so that access to each filesystem is
+requested and cached separately for each client IP address.  Doing this
+can increase the burden of updating the cache slightly, but can make the
+log messages produced by the
+.B -l
+option easier to read.
+.TP
 .B \-F " or " \-\-foreground
 Run in foreground (do not daemonize)
 .TP
@@ -248,6 +265,7 @@ Values recognized in the
 .B [mountd]
 section include
 .BR manage-gids ,
+.BR cache\-use\-ipaddr ,
 .BR descriptors ,
 .BR port ,
 .BR threads ,

[PATCH 3/5] mountd: add logging for authentication results for accesses.

[NeilBrown]

From: NeilBrown <neil@brown.name>

When NFSv3 is used to mount a filesystem, success/failure messages are
logged by mountd and can be used for auditing.
When NFSv4 is used, there is no distinct "MOUNT" request, and nothing is
logged.

We can instead log authentication requests from the kernel.  These will
happen regularly - typically every 15 minutes of ongoing access - so
they may be too noisy, or might be more useful.  As they might not be
wanted, make them selectable with the "AUTH" facility in xlog().

Add a "-l" to enable these logs.  Alternately "debug = auth" will have
the same effect.

Signed-off-by: NeilBrown <neil@brown.name>
---
 support/export/cache.c  |   18 +++++++++++++++++-
 utils/mountd/mountd.c   |    8 +++++++-
 utils/mountd/mountd.man |   39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/support/export/cache.c b/support/export/cache.c
index 49a761749ec6..50f7c7a15ceb 100644
--- a/support/export/cache.c
+++ b/support/export/cache.c
@@ -145,6 +145,15 @@ static void auth_unix_ip(int f)
 		client = client_compose(ai);
 		nfs_freeaddrinfo(ai);
 	}
+	if (!client)
+		xlog(D_AUTH, "failed authentication for IP %s", ipaddr);
+	else if	(!use_ipaddr)
+		xlog(D_AUTH, "successful authentication for IP %s as %s",
+		     ipaddr, *client ? client : "DEFAULT");
+	else
+		xlog(D_AUTH, "successful authentication for IP %s",
+			     ipaddr);
+
 	bp = buf; blen = sizeof(buf);
 	qword_add(&bp, &blen, "nfsd");
 	qword_add(&bp, &blen, ipaddr);
@@ -896,6 +905,8 @@ static void nfsd_fh(int f)
 	qword_addeol(&bp, &blen);
 	if (blen <= 0 || cache_write(f, buf, bp - buf) != bp - buf)
 		xlog(L_ERROR, "nfsd_fh: error writing reply");
+	if (!found)
+		xlog(D_AUTH, "denied access to %s", *dom == '$' ? dom+1 : dom);
 out:
 	if (found_path)
 		free(found_path);
@@ -987,8 +998,13 @@ static int dump_to_cache(int f, char *buf, int blen, char *domain,
 			qword_add(&bp, &blen, "uuid");
 			qword_addhex(&bp, &blen, u, 16);
 		}
-	} else
+		xlog(D_AUTH, "granted access to %s for %s",
+		     path, *domain == '$' ? domain+1 : domain);
+	} else {
 		qword_adduint(&bp, &blen, now + ttl);
+		xlog(D_AUTH, "denied access to %s for %s",
+		     path, *domain == '$' ? domain+1 : domain);
+	}
 	qword_addeol(&bp, &blen);
 	if (blen <= 0) {
 		errno = ENOBUFS;
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 612063ba2340..9fecf2f04c3b 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -74,8 +74,10 @@ static struct option longopts[] =
 	{ "reverse-lookup", 0, 0, 'r' },
 	{ "manage-gids", 0, 0, 'g' },
 	{ "no-udp", 0, 0, 'u' },
+	{ "log-auth", 0, 0, 'l'},
 	{ NULL, 0, 0, 0 }
 };
+static char shortopts[] = "o:nFd:p:P:hH:N:V:vurs:t:gl";
 
 #define NFSVERSBIT(vers)	(0x1 << (vers - 1))
 #define NFSVERSBIT_ALL		(NFSVERSBIT(2) | NFSVERSBIT(3) | NFSVERSBIT(4))
@@ -727,7 +729,7 @@ main(int argc, char **argv)
 
 	/* Parse the command line options and arguments. */
 	opterr = 0;
-	while ((c = getopt_long(argc, argv, "o:nFd:p:P:hH:N:V:vurs:t:g", longopts, NULL)) != EOF)
+	while ((c = getopt_long(argc, argv, shortopts, longopts, NULL)) != EOF)
 		switch (c) {
 		case 'g':
 			manage_gids = 1;
@@ -798,6 +800,9 @@ main(int argc, char **argv)
 		case 'u':
 			NFSCTL_UDPUNSET(_rpcprotobits);
 			break;
+		case 'l':
+			xlog_sconfig("auth", 1);
+			break;
 		case 0:
 			break;
 		case '?':
@@ -913,6 +918,7 @@ usage(const char *prog, int n)
 {
 	fprintf(stderr,
 "Usage: %s [-F|--foreground] [-h|--help] [-v|--version] [-d kind|--debug kind]\n"
+"	[-l|--log-auth]\n"
 "	[-o num|--descriptors num]\n"
 "	[-p|--port port] [-V version|--nfs-version version]\n"
 "	[-N version|--no-nfs-version version] [-n|--no-tcp]\n"
diff --git a/utils/mountd/mountd.man b/utils/mountd/mountd.man
index 9978afcdb4cc..df4e5356cb05 100644
--- a/utils/mountd/mountd.man
+++ b/utils/mountd/mountd.man
@@ -13,6 +13,8 @@ The
 .B rpc.mountd
 daemon implements the server side of the NFS MOUNT protocol,
 an NFS side protocol used by NFS version 2 [RFC1094] and NFS version 3 [RFC1813].
+It also responds to requests from the Linux kernel to authenticate
+clients and provides details of access permissions.
 .PP
 An NFS server maintains a table of local physical file systems
 that are accessible to NFS clients.
@@ -78,11 +80,44 @@ A client may continue accessing an export even after invoking UMNT.
 If the client reboots without sending a UMNT request, stale entries
 remain for that client in
 .IR /var/lib/nfs/rmtab .
+.SS Mounting File Systems with NFSv4
+Version 4 (and later) of NFS does not use a separate NFS MOUNT
+protocol.  Instead mounting is performed using regular NFS requests
+handled by the NFS server in the Linux kernel
+.RI ( nfsd ).
+When
+.I nfsd
+needs to confirm if a client has access to a particular filesystem, it
+communicates with
+.B rpc.mountd
+to authenticate the client and to then determine what access that client
+has to a given filesystem.
 .SH OPTIONS
 .TP
 .B \-d kind " or " \-\-debug kind
 Turn on debugging. Valid kinds are: all, auth, call, general and parse.
 .TP
+.BR \-l " or " \-\-log\-auth
+Enable logging of responses to authentication and access requests from
+nfsd.  Each response is then cached by the kernel for 30 minutes, and
+will be refreshed after 15 minutes if the relevant client remains
+active.
+Note that
+.B -l
+is equivalent to
+.B "-d auth"
+and so can be enabled in
+.B /etc/nfs.conf
+with
+.B "\[dq]debug = auth\[dq]"
+in the
+.B "[mountd]"
+section.
+.IP
+.B rpc.mountd
+will always log authentication responses to MOUNT requests when NFSv3 is
+used, but to get similar logs for NFSv4, this option is required.
+.TP
 .B \-F " or " \-\-foreground
 Run in foreground (do not daemonize)
 .TP
@@ -295,5 +330,9 @@ table of clients accessing server's exports
 RFC 1094 - "NFS: Network File System Protocol Specification"
 .br
 RFC 1813 - "NFS Version 3 Protocol Specification"
+.br
+RFC 7530 - "Network File System (NFS) Version 4 Protocol"
+.br
+RFC 8881 - "Network File System (NFS) Version 4 Minor Version 1 Protocol"
 .SH AUTHOR
 Olaf Kirch, H. J. Lu, G. Allan Morris III, and a host of others.

[PATCH 5/5] mountd: make default ttl settable by option

[NeilBrown]

From: NeilBrown <neil@brown.name>

The DEFAULT_TTL affects the rate at which authentication messages are
logged.  So it is useful to make it settable.

Add "-ttl" and "-T", and add clear statement in the documentation of
both the benefits and the possible negative effects of choosing a larger
value

Signed-off-by: NeilBrown <neil@brown.name>
---
 support/export/cache.c     |    6 +++---
 support/export/v4root.c    |    3 ++-
 support/include/exportfs.h |    3 ++-
 support/nfs/exports.c      |    4 +++-
 utils/mountd/mountd.c      |   20 ++++++++++++++++++--
 utils/mountd/mountd.man    |   19 ++++++++++++++++---
 6 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/support/export/cache.c b/support/export/cache.c
index 50f7c7a15ceb..c0848c3e437b 100644
--- a/support/export/cache.c
+++ b/support/export/cache.c
@@ -157,7 +157,7 @@ static void auth_unix_ip(int f)
 	bp = buf; blen = sizeof(buf);
 	qword_add(&bp, &blen, "nfsd");
 	qword_add(&bp, &blen, ipaddr);
-	qword_adduint(&bp, &blen, time(0) + DEFAULT_TTL);
+	qword_adduint(&bp, &blen, time(0) + default_ttl);
 	if (use_ipaddr && client) {
 		memmove(ipaddr + 1, ipaddr, strlen(ipaddr) + 1);
 		ipaddr[0] = '$';
@@ -230,7 +230,7 @@ static void auth_unix_gid(int f)
 
 	bp = buf; blen = sizeof(buf);
 	qword_adduint(&bp, &blen, uid);
-	qword_adduint(&bp, &blen, time(0) + DEFAULT_TTL);
+	qword_adduint(&bp, &blen, time(0) + default_ttl);
 	if (rv >= 0) {
 		qword_adduint(&bp, &blen, ngroups);
 		for (i=0; i<ngroups; i++)
@@ -968,7 +968,7 @@ static int dump_to_cache(int f, char *buf, int blen, char *domain,
 	ssize_t err;
 
 	if (ttl <= 1)
-		ttl = DEFAULT_TTL;
+		ttl = default_ttl;
 
 	qword_add(&bp, &blen, domain);
 	qword_add(&bp, &blen, path);
diff --git a/support/export/v4root.c b/support/export/v4root.c
index 6f640aa9aa3f..3654bd7c10c0 100644
--- a/support/export/v4root.c
+++ b/support/export/v4root.c
@@ -45,7 +45,7 @@ static nfs_export pseudo_root = {
 		.e_nsqgids = 0,
 		.e_fsid = 0,
 		.e_mountpoint = NULL,
-		.e_ttl = DEFAULT_TTL,
+		.e_ttl = 0,
 	},
 	.m_exported = 0,
 	.m_xtabent = 1,
@@ -84,6 +84,7 @@ v4root_create(char *path, nfs_export *export)
 	struct exportent *curexp = &export->m_export;
 
 	dupexportent(&eep, &pseudo_root.m_export);
+	eep.e_ttl = default_ttl;
 	eep.e_hostname = curexp->e_hostname;
 	strncpy(eep.e_path, path, sizeof(eep.e_path)-1);
 	if (strcmp(path, "/") != 0)
diff --git a/support/include/exportfs.h b/support/include/exportfs.h
index daa7e2a06d82..81d137210862 100644
--- a/support/include/exportfs.h
+++ b/support/include/exportfs.h
@@ -105,7 +105,8 @@ typedef struct mexport {
 } nfs_export;
 
 #define HASH_TABLE_SIZE 1021
-#define DEFAULT_TTL	(30 * 60)
+
+extern int default_ttl;
 
 typedef struct _exp_hash_entry {
 	nfs_export * p_first;
diff --git a/support/nfs/exports.c b/support/nfs/exports.c
index 037febd08d9b..2c8f0752ad9d 100644
--- a/support/nfs/exports.c
+++ b/support/nfs/exports.c
@@ -47,6 +47,8 @@ struct flav_info flav_map[] = {
 
 const int flav_map_size = sizeof(flav_map)/sizeof(flav_map[0]);
 
+int default_ttl = 30 * 60;
+
 static char	*efname = NULL;
 static XFILE	*efp = NULL;
 static int	first;
@@ -100,7 +102,7 @@ static void init_exportent (struct exportent *ee, int fromkernel)
 	ee->e_nsquids = 0;
 	ee->e_nsqgids = 0;
 	ee->e_uuid = NULL;
-	ee->e_ttl = DEFAULT_TTL;
+	ee->e_ttl = default_ttl;
 }
 
 struct exportent *
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index b9260aeb86a3..fce389661e7a 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -76,9 +76,10 @@ static struct option longopts[] =
 	{ "no-udp", 0, 0, 'u' },
 	{ "log-auth", 0, 0, 'l'},
 	{ "cache-use-ipaddr", 0, 0, 'i'},
+	{ "ttl", 1, 0, 'T'},
 	{ NULL, 0, 0, 0 }
 };
-static char shortopts[] = "o:nFd:p:P:hH:N:V:vurs:t:gli";
+static char shortopts[] = "o:nFd:p:P:hH:N:V:vurs:t:gliT:";
 
 #define NFSVERSBIT(vers)	(0x1 << (vers - 1))
 #define NFSVERSBIT_ALL		(NFSVERSBIT(2) | NFSVERSBIT(3) | NFSVERSBIT(4))
@@ -672,6 +673,7 @@ inline static void
 read_mountd_conf(char **argv)
 {
 	char	*s;
+	int	ttl;
 
 	conf_init_file(NFS_CONFFILE);
 
@@ -706,6 +708,10 @@ read_mountd_conf(char **argv)
 		else
 			NFSCTL_VERUNSET(nfs_version, vers);
 	}
+
+	ttl = conf_get_num("mountd", "ttl", default_ttl);
+	if (ttl > 0)
+		default_ttl = ttl;
 }
 
 int
@@ -715,6 +721,7 @@ main(int argc, char **argv)
 	unsigned int listeners = 0;
 	int	foreground = 0;
 	int	c;
+	int	ttl;
 	struct sigaction sa;
 	struct rlimit rlim;
 
@@ -809,6 +816,15 @@ main(int argc, char **argv)
 		case 'i':
 			use_ipaddr = 2;
 			break;
+		case 'T':
+			ttl = atoi(optarg);
+			if (ttl <= 0) {
+				fprintf(stderr, "%s: bad ttl number of seconds: %s\n",
+					argv[0], optarg);
+				usage(argv[0], 1);
+			}
+			default_ttl = ttl;
+			break;
 		case 0:
 			break;
 		case '?':
@@ -924,7 +940,7 @@ usage(const char *prog, int n)
 {
 	fprintf(stderr,
 "Usage: %s [-F|--foreground] [-h|--help] [-v|--version] [-d kind|--debug kind]\n"
-"	[-l|--log-auth] [-i|--cache-use-ipaddr]\n"
+"	[-l|--log-auth] [-i|--cache-use-ipaddr] [-T|--ttl ttl]\n"
 "	[-o num|--descriptors num]\n"
 "	[-p|--port port] [-V version|--nfs-version version]\n"
 "	[-N version|--no-nfs-version version] [-n|--no-tcp]\n"
diff --git a/utils/mountd/mountd.man b/utils/mountd/mountd.man
index 44d237e56110..82e07cf221fa 100644
--- a/utils/mountd/mountd.man
+++ b/utils/mountd/mountd.man
@@ -99,9 +99,10 @@ Turn on debugging. Valid kinds are: all, auth, call, general and parse.
 .TP
 .BR \-l " or " \-\-log\-auth
 Enable logging of responses to authentication and access requests from
-nfsd.  Each response is then cached by the kernel for 30 minutes, and
-will be refreshed after 15 minutes if the relevant client remains
-active.
+nfsd.  Each response is then cached by the kernel for 30 minutes (or as set by
+.B \-\-ttl
+below), and will be refreshed after 15 minutes (half the ttl time) if
+the relevant client remains active.
 Note that
 .B -l
 is equivalent to
@@ -135,6 +136,17 @@ log messages produced by the
 .B -l
 option easier to read.
 .TP
+.B \-T " or " \-\-ttl
+Provide a time-to-live (TTL) for cached information given to the kernel.
+The kernel will normally request an update if the information is needed
+after half of this time has expired.  Increasing the provided number,
+which is in seconds, reduces the rate of cache update requests, and this
+is particularly noticeable when these requests are logged with
+.BR \-l .
+However increasing also means that changes to hostname to address
+mappings can take longer to be noticed.
+The default TTL is 1800 (30 minutes).
+.TP
 .B \-F " or " \-\-foreground
 Run in foreground (do not daemonize)
 .TP
@@ -269,6 +281,7 @@ section include
 .BR descriptors ,
 .BR port ,
 .BR threads ,
+.BR ttl ,
 .BR reverse-lookup ", and"
 .BR state-directory-path ,
 .B ha-callout

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[Yongcheng Yang]

Hi NeilBrown,

Shall we further add these 2 options (cache-use-ipaddr & ttl) with
their default values to the file /etc/nfs.conf under section [mountd]?
By which the users can find it easier to configure them.

Also someone may check the mountd "Recognized values" from
nfs.conf(5), the file systemd/nfs.conf.man may also needs to be
updated mentioning "cache-use-ipaddr" and "ttl" IMHO. 

Thanks,
Yongcheng


On Mon, Mar 01, 2021 at 01:17:15PM +1100, NeilBrown wrote:
> V1 of this series didn't update the usage() message for mountd,
> and omited the required ':' after the 'T' sort-option.  This 
> series fixes those two omissions.
> 
> Original series comment:
> 
> When NFSv3 is used mountd provides logs of successful and failed mount
> attempts which can be used for auditing.
> When NFSv4 is used there are no such logs as NFSv4 does not have a
> distinct "mount" request.
> 
> However mountd still knows about which filesysytems are being accessed
> from which clients, and can actually provide more reliable logs than it
> currently does, though they must be more verbose - with periodic "is
> being accessed" message replacing a single "was mounted" message.
> 
> This series adds support for that logging, and adds some related
> improvements to make the logs as useful as possible.
> 
> NeilBrown
> 
> ---
> 
> NeilBrown (5):
>       mountd: reject unknown client IP when !use_ipaddr.
>       mountd: Don't proactively add export info when fh info is requested.
>       mountd: add logging for authentication results for accesses.
>       mountd: add --cache-use-ipaddr option to force use_ipaddr
>       mountd: make default ttl settable by option
> 
> 
>  support/export/auth.c      |  4 +++
>  support/export/cache.c     | 32 +++++++++++------
>  support/export/v4root.c    |  3 +-
>  support/include/exportfs.h |  3 +-
>  support/nfs/exports.c      |  4 ++-
>  utils/mountd/mountd.c      | 30 +++++++++++++++-
>  utils/mountd/mountd.man    | 70 ++++++++++++++++++++++++++++++++++++++
>  7 files changed, 131 insertions(+), 15 deletions(-)
> 
> --
> Signature
> 

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[J. Bruce Fields]

I've gotten requests for similar functionality, and intended to
implement it using directory notifications on /proc/fs/nfsd/clients.

But this is another way to do it that I hadn't thought of.  That's
interesting.  I haven't thought about the relative advantages or
disadvantages.

--b.

On Mon, Mar 01, 2021 at 01:17:15PM +1100, NeilBrown wrote:
> V1 of this series didn't update the usage() message for mountd,
> and omited the required ':' after the 'T' sort-option.  This 
> series fixes those two omissions.
> 
> Original series comment:
> 
> When NFSv3 is used mountd provides logs of successful and failed mount
> attempts which can be used for auditing.
> When NFSv4 is used there are no such logs as NFSv4 does not have a
> distinct "mount" request.
> 
> However mountd still knows about which filesysytems are being accessed
> from which clients, and can actually provide more reliable logs than it
> currently does, though they must be more verbose - with periodic "is
> being accessed" message replacing a single "was mounted" message.
> 
> This series adds support for that logging, and adds some related
> improvements to make the logs as useful as possible.
> 
> NeilBrown
> 
> ---
> 
> NeilBrown (5):
>       mountd: reject unknown client IP when !use_ipaddr.
>       mountd: Don't proactively add export info when fh info is requested.
>       mountd: add logging for authentication results for accesses.
>       mountd: add --cache-use-ipaddr option to force use_ipaddr
>       mountd: make default ttl settable by option
> 
> 
>  support/export/auth.c      |  4 +++
>  support/export/cache.c     | 32 +++++++++++------
>  support/export/v4root.c    |  3 +-
>  support/include/exportfs.h |  3 +-
>  support/nfs/exports.c      |  4 ++-
>  utils/mountd/mountd.c      | 30 +++++++++++++++-
>  utils/mountd/mountd.man    | 70 ++++++++++++++++++++++++++++++++++++++
>  7 files changed, 131 insertions(+), 15 deletions(-)
> 
> --
> Signature

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2756 bytes --]

On Mon, Mar 01 2021, J. Bruce Fields wrote:

> I've gotten requests for similar functionality, and intended to
> implement it using directory notifications on /proc/fs/nfsd/clients.

Interesting idea.  That could report successful mounts and unmounts but
wouldn't report failed mount attempts or (I think) which export point is
being mounted.

If we could reliably correlate the entry in 'clients' with each auth
request, we could filter out multiple auth refresh requests for the same
client+filesystem.  The only field we could do that on would be IP
address, and that wouldn't be seamless..

Might be worth pursuing if only to report "mount" and "unmount" events
for a client identified by IP, and leave it to the admin to see any
connection between 'mount' events and 'auth' events.

Thanks,
NeilBrown


>
> But this is another way to do it that I hadn't thought of.  That's
> interesting.  I haven't thought about the relative advantages or
> disadvantages.
>
> --b.
>
> On Mon, Mar 01, 2021 at 01:17:15PM +1100, NeilBrown wrote:
>> V1 of this series didn't update the usage() message for mountd,
>> and omited the required ':' after the 'T' sort-option.  This 
>> series fixes those two omissions.
>> 
>> Original series comment:
>> 
>> When NFSv3 is used mountd provides logs of successful and failed mount
>> attempts which can be used for auditing.
>> When NFSv4 is used there are no such logs as NFSv4 does not have a
>> distinct "mount" request.
>> 
>> However mountd still knows about which filesysytems are being accessed
>> from which clients, and can actually provide more reliable logs than it
>> currently does, though they must be more verbose - with periodic "is
>> being accessed" message replacing a single "was mounted" message.
>> 
>> This series adds support for that logging, and adds some related
>> improvements to make the logs as useful as possible.
>> 
>> NeilBrown
>> 
>> ---
>> 
>> NeilBrown (5):
>>       mountd: reject unknown client IP when !use_ipaddr.
>>       mountd: Don't proactively add export info when fh info is requested.
>>       mountd: add logging for authentication results for accesses.
>>       mountd: add --cache-use-ipaddr option to force use_ipaddr
>>       mountd: make default ttl settable by option
>> 
>> 
>>  support/export/auth.c      |  4 +++
>>  support/export/cache.c     | 32 +++++++++++------
>>  support/export/v4root.c    |  3 +-
>>  support/include/exportfs.h |  3 +-
>>  support/nfs/exports.c      |  4 ++-
>>  utils/mountd/mountd.c      | 30 +++++++++++++++-
>>  utils/mountd/mountd.man    | 70 ++++++++++++++++++++++++++++++++++++++
>>  7 files changed, 131 insertions(+), 15 deletions(-)
>> 
>> --
>> Signature

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

On Mon, Mar 01 2021, Yongcheng Yang wrote:

> Hi NeilBrown,
>
> Shall we further add these 2 options (cache-use-ipaddr & ttl) with
> their default values to the file /etc/nfs.conf under section [mountd]?
> By which the users can find it easier to configure them.
>
> Also someone may check the mountd "Recognized values" from
> nfs.conf(5), the file systemd/nfs.conf.man may also needs to be
> updated mentioning "cache-use-ipaddr" and "ttl" IMHO. 

Excellent suggestion, thanks.

I've made those changes and will resend the series.

Thanks,
NeilBrown

>
> Thanks,
> Yongcheng
>
>
> On Mon, Mar 01, 2021 at 01:17:15PM +1100, NeilBrown wrote:
>> V1 of this series didn't update the usage() message for mountd,
>> and omited the required ':' after the 'T' sort-option.  This 
>> series fixes those two omissions.
>> 
>> Original series comment:
>> 
>> When NFSv3 is used mountd provides logs of successful and failed mount
>> attempts which can be used for auditing.
>> When NFSv4 is used there are no such logs as NFSv4 does not have a
>> distinct "mount" request.
>> 
>> However mountd still knows about which filesysytems are being accessed
>> from which clients, and can actually provide more reliable logs than it
>> currently does, though they must be more verbose - with periodic "is
>> being accessed" message replacing a single "was mounted" message.
>> 
>> This series adds support for that logging, and adds some related
>> improvements to make the logs as useful as possible.
>> 
>> NeilBrown
>> 
>> ---
>> 
>> NeilBrown (5):
>>       mountd: reject unknown client IP when !use_ipaddr.
>>       mountd: Don't proactively add export info when fh info is requested.
>>       mountd: add logging for authentication results for accesses.
>>       mountd: add --cache-use-ipaddr option to force use_ipaddr
>>       mountd: make default ttl settable by option
>> 
>> 
>>  support/export/auth.c      |  4 +++
>>  support/export/cache.c     | 32 +++++++++++------
>>  support/export/v4root.c    |  3 +-
>>  support/include/exportfs.h |  3 +-
>>  support/nfs/exports.c      |  4 ++-
>>  utils/mountd/mountd.c      | 30 +++++++++++++++-
>>  utils/mountd/mountd.man    | 70 ++++++++++++++++++++++++++++++++++++++
>>  7 files changed, 131 insertions(+), 15 deletions(-)
>> 
>> --
>> Signature
>> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

On Mon, Mar 01 2021, J. Bruce Fields wrote:

> I've gotten requests for similar functionality, and intended to
> implement it using directory notifications on /proc/fs/nfsd/clients.

I've been exploring this a bit.
When I mount a filesystem, 2 clients get created.
With NFSv4.0, the second client is immediately deleted, and the first
client is deleted one grace period after the filesystem is unmounted.
With NFSv4.1 and 4.2, the first client is immediately deleted, and the
second client is deleted immediately after the unmount.

Do you know why two clients are created?  I could dig through the code
... but maybe you already know.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[J. Bruce Fields]

On Tue, Mar 02, 2021 at 02:01:36PM +1100, NeilBrown wrote:
> On Mon, Mar 01 2021, J. Bruce Fields wrote:
> 
> > I've gotten requests for similar functionality, and intended to
> > implement it using directory notifications on /proc/fs/nfsd/clients.
> 
> I've been exploring this a bit.
> When I mount a filesystem, 2 clients get created.
> With NFSv4.0, the second client is immediately deleted, and the first
> client is deleted one grace period after the filesystem is unmounted.
> With NFSv4.1 and 4.2, the first client is immediately deleted, and the
> second client is deleted immediately after the unmount.

Yeah, internally it's creating an "unconfirmed client" on SETCLIENTID
(or EXCHANGE_ID) and then a new "confirmed client" on
SETCLIENTID_CONFIRM (or CREATE_SESSION).

I'm not sure why the ordering's a little different between the 4.0/4.1+
cases.

The difference on unmount is because 4.1+ clients immediately send a
DESTROY_CLIENTID on unmount, but that op was new to 4.1.

(Note of course this isn't precisely mount/unmount, as the same client
can be used for multiple filesystems.)

Honestly, I think this is exposing an implementation detail and it's
dumb.  I'll look into fixing it.

(I don't know if that change itself would cause additional difficulty.)

--b.

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]

On Mon, Mar 01 2021, J. Bruce Fields wrote:

> On Tue, Mar 02, 2021 at 02:01:36PM +1100, NeilBrown wrote:
>> On Mon, Mar 01 2021, J. Bruce Fields wrote:
>> 
>> > I've gotten requests for similar functionality, and intended to
>> > implement it using directory notifications on /proc/fs/nfsd/clients.
>> 
>> I've been exploring this a bit.
>> When I mount a filesystem, 2 clients get created.
>> With NFSv4.0, the second client is immediately deleted, and the first
>> client is deleted one grace period after the filesystem is unmounted.
>> With NFSv4.1 and 4.2, the first client is immediately deleted, and the
>> second client is deleted immediately after the unmount.
>
> Yeah, internally it's creating an "unconfirmed client" on SETCLIENTID
> (or EXCHANGE_ID) and then a new "confirmed client" on
> SETCLIENTID_CONFIRM (or CREATE_SESSION).

Of course - the "confirm" step.  That would explain it.

>
> I'm not sure why the ordering's a little different between the 4.0/4.1+
> cases.
>
> The difference on unmount is because 4.1+ clients immediately send a
> DESTROY_CLIENTID on unmount, but that op was new to 4.1.

I thought it would be something like that - a protocol improvements.

>
> (Note of course this isn't precisely mount/unmount, as the same client
> can be used for multiple filesystems.)

True.  It could also be a network disconnect, not an explicit unmount.
Still I think it could be useful to log.

>
> Honestly, I think this is exposing an implementation detail and it's
> dumb.  I'll look into fixing it.

Thanks.

>
> (I don't know if that change itself would cause additional difficulty.)

I doubt it.  You would need to look at "info" in a directory to do
anything useful, and in my simple testing that always said
  cat: /proc/fs/nfsd/clients/41/info: No such file or directory
or similar in the transient directory.  So code is mostly likely to
quickly ignore any such directory.

I'll see how easy it is to add "client added" and "client removed" log
messages to mountd before resending my series.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[J. Bruce Fields]

On Tue, Mar 02, 2021 at 02:49:13PM +1100, NeilBrown wrote:
> I'll see how easy it is to add "client added" and "client removed" log
> messages to mountd before resending my series.

OK, thanks again for doing this!

Also if there's more information we could add to those "info" files to
make it easier to correlate with the authentication upcalls, that should
be easy to do....

--b.

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Mon, Mar 01 2021, J. Bruce Fields wrote:

> On Tue, Mar 02, 2021 at 02:01:36PM +1100, NeilBrown wrote:
>> On Mon, Mar 01 2021, J. Bruce Fields wrote:
>> 
>> > I've gotten requests for similar functionality, and intended to
>> > implement it using directory notifications on /proc/fs/nfsd/clients.
>> 
>> I've been exploring this a bit.
>> When I mount a filesystem, 2 clients get created.
>> With NFSv4.0, the second client is immediately deleted, and the first
>> client is deleted one grace period after the filesystem is unmounted.
>> With NFSv4.1 and 4.2, the first client is immediately deleted, and the
>> second client is deleted immediately after the unmount.
>
> Yeah, internally it's creating an "unconfirmed client" on SETCLIENTID
> (or EXCHANGE_ID) and then a new "confirmed client" on
> SETCLIENTID_CONFIRM (or CREATE_SESSION).
>
> I'm not sure why the ordering's a little different between the 4.0/4.1+
> cases.

The multiple clients are not really nfsd's "fault".  The Linux NFS
client sends multiple EXCHANGE_ID or SET_CLIENT_ID requests, so NFSD
really does need to create multiple clients.

For NFSv4.0, when nfsd gets a repeat SET_CLIENT_ID, it keeps the old one
and discards the new.
For NFSv4.1, the spec requires that it keep the new one and discard the
old.
This explains the different ordering.

So the clean up the logging, mountd needs to be able to see the
confirmation status.
Following this reply will be a patch to nfsd to provide this status, and
a patch to mountd/exportd to use this status.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

[PATCH] nfsd: report client confirmation status in "info" file

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]


mountd can now monitor clients appearing and disappearing in
/proc/fs/nfsd/clients, and will log these events, in liu of the logging
of mount/unmount events for NFSv3.

Currently it cannot distinguish between unconfirmed clients (which might
be transient and totally uninteresting) and confirmed clients.

So add a "status: " line to the "info" file which reports either
"confirmed" or "unconfirmed".

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 97447a64bad0..98d9fe5a2ec5 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2352,6 +2352,10 @@ static int client_info_show(struct seq_file *m, void *v)
 	memcpy(&clid, &clp->cl_clientid, sizeof(clid));
 	seq_printf(m, "clientid: 0x%llx\n", clid);
 	seq_printf(m, "address: \"%pISpc\"\n", (struct sockaddr *)&clp->cl_addr);
+	if (test_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags))
+		seq_puts(m, "status: confirmed\n");
+	else
+		seq_puts(m, "status: unconfirmed\n");
 	seq_printf(m, "name: ");
 	seq_quote_mem(m, clp->cl_name.data, clp->cl_name.len);
 	seq_printf(m, "\nminor version: %d\n", clp->cl_minorversion);
-- 
2.30.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

[PATCH] mountd/exportd: only log confirmed clients, and poll for updates

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 7645 bytes --]


It is possible (and common with the Linux NFS client) for the nfs server
to receive multiple SET_CLIENT_ID or EXCHANGE_ID requests when starting
a connection.  This results in some clients appearing in
 /proc/fs/nfsd/clients
which never get confirmed.  mountd currently logs these, but they aren't
really helpful.

If the kernel supports the reporting of the confirmation status of
clients, we can suppress the message until a client is confirmed.

With this patch we:
 - record if the client is confirmed, assuming it is if the status is
    not reported
 - don't log unconfirmed clients
 - check all unconfirmed clients each time any event is processed,
 - if there are unconfirmed clients, we request a wakeup after a
   exponentially decreasing time, and check again

This requires updating the two event loops to allow a timeout to be
specified.

When there is no other activity, but there are unconfirmed clients,
timeout for repeat checks are after 1, 2, 4 ... 512 seconds.
If any wait is interrupted, the next wait time is still chosen, so we
might advance quickly through the list.  But if there is lots of
activity, we don't really need the timeout.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 support/export/cache.c     |  7 ++-
 support/export/export.h    |  2 +-
 support/export/v4clients.c | 92 ++++++++++++++++++++++++++++++--------
 utils/mountd/svc_run.c     |  7 ++-
 4 files changed, 85 insertions(+), 23 deletions(-)

diff --git a/support/export/cache.c b/support/export/cache.c
index 3e4f53c0a32e..687b3aef27a1 100644
--- a/support/export/cache.c
+++ b/support/export/cache.c
@@ -1524,6 +1524,7 @@ void cache_process_loop(void)
 {
 	fd_set	readfds;
 	int	selret;
+	struct timeval tv = { 60*60, 0 };
 
 	FD_ZERO(&readfds);
 
@@ -1533,8 +1534,10 @@ void cache_process_loop(void)
 		v4clients_set_fds(&readfds);
 
 		selret = select(FD_SETSIZE, &readfds,
-				(void *) 0, (void *) 0, (struct timeval *) 0);
+				(void *) 0, (void *) 0, &tv);
 
+		tv.tv_sec = 60*60;
+		tv.tv_usec = 0;
 
 		switch (selret) {
 		case -1:
@@ -1546,7 +1549,7 @@ void cache_process_loop(void)
 
 		default:
 			cache_process_req(&readfds);
-			v4clients_process(&readfds);
+			v4clients_process(&readfds, &tv);
 		}
 	}
 }
diff --git a/support/export/export.h b/support/export/export.h
index 8d5a0d3004ef..71d3ed39bd28 100644
--- a/support/export/export.h
+++ b/support/export/export.h
@@ -24,7 +24,7 @@ void		cache_process_loop(void);
 
 void		v4clients_init(void);
 void		v4clients_set_fds(fd_set *fdset);
-int		v4clients_process(fd_set *fdset);
+int		v4clients_process(fd_set *fdset, struct timeval *tv);
 
 struct nfs_fh_len *
 		cache_get_filehandle(nfs_export *exp, int len, char *p);
diff --git a/support/export/v4clients.c b/support/export/v4clients.c
index 056ddc9b065d..04cec3136876 100644
--- a/support/export/v4clients.c
+++ b/support/export/v4clients.c
@@ -48,12 +48,15 @@ void v4clients_set_fds(fd_set *fdset)
 }
 
 static void *tree_root;
+static int have_unconfirmed;
+static time_t unconfirmed_wait = 1;
 
 struct ent {
 	unsigned long num;
 	char *clientid;
 	char *addr;
 	int vers;
+	int unconfirmed;
 };
 
 static int ent_cmp(const void *av, const void *bv)
@@ -89,15 +92,13 @@ static char *dup_line(char *line)
 	return ret;
 }
 
-static void add_id(int id)
+static void read_info(struct ent *key)
 {
 	char buf[2048];
-	struct ent **ent;
-	struct ent *key;
 	char *path;
 	FILE *f;
 
-	if (asprintf(&path, "/proc/fs/nfsd/clients/%d/info", id) < 0)
+	if (asprintf(&path, "/proc/fs/nfsd/clients/%lu/info", key->num) < 0)
 		return;
 
 	f = fopen(path, "r");
@@ -105,32 +106,56 @@ static void add_id(int id)
 		free(path);
 		return;
 	}
-	key = calloc(1, sizeof(*key));
-	if (!key) {
-		fclose(f);
-		free(path);
-		return;
-	}
-	key->num = id;
 	while (fgets(buf, sizeof(buf), f)) {
-		if (strncmp(buf, "clientid: ", 10) == 0)
+		if (strncmp(buf, "clientid: ", 10) == 0) {
+			free(key->clientid);
 			key->clientid = dup_line(buf+10);
-		if (strncmp(buf, "address: ", 9) == 0)
+		}
+		if (strncmp(buf, "address: ", 9) == 0) {
+			free(key->addr);
 			key->addr = dup_line(buf+9);
+		}
 		if (strncmp(buf, "minor version: ", 15) == 0)
 			key->vers = atoi(buf+15);
+		if (strncmp(buf, "status: ", 8) == 0 &&
+		    strstr(buf, " unconfirmed") != NULL) {
+			key->unconfirmed = 1;
+			have_unconfirmed = 1;
+		}
+		if (strncmp(buf, "status: ", 8) == 0 &&
+		    strstr(buf, " confirmed") != NULL)
+			key->unconfirmed = 0;
 	}
 	fclose(f);
 	free(path);
 
-	xlog(L_NOTICE, "v4.%d client attached: %s from %s",
-	     key->vers, key->clientid, key->addr);
+	if (!key->unconfirmed)
+		xlog(L_NOTICE, "v4.%d client attached: %s from %s",
+		     key->vers, key->clientid ?: "-none-",
+		     key->addr ?: "-none-");
+}
+
+static void add_id(int id)
+{
+	struct ent **ent;
+	struct ent *key;
+
+	key = calloc(1, sizeof(*key));
+	if (!key) {
+		return;
+	}
+	key->num = id;
 
 	ent = tsearch(key, &tree_root, ent_cmp);
 
 	if (!ent || *ent != key)
 		/* Already existed, or insertion failed */
 		free_ent(key);
+	else {
+		read_info(key);
+		if (key->unconfirmed)
+			unconfirmed_wait = 1;
+	}
 }
 
 static void del_id(int id)
@@ -143,18 +168,43 @@ static void del_id(int id)
 		return;
 	ent = *e;
 	tdelete(ent, &tree_root, ent_cmp);
-	xlog(L_NOTICE, "v4.%d client detached: %s from %s",
-	     ent->vers, ent->clientid, ent->addr);
+	if (!ent->unconfirmed)
+		xlog(L_NOTICE, "v4.%d client detached: %s from %s",
+		     ent->vers, ent->clientid, ent->addr);
 	free_ent(ent);
 }
 
-int v4clients_process(fd_set *fdset)
+static void check_one(const void *ventp, VISIT which, int depth)
+{
+	struct ent * const *ep = ventp;
+	struct ent *ent;
+
+	if (depth >= 0 && /* Silence "unused paramter" warning */
+	    which != preorder && which != leaf)
+		return;
+
+	ent = *ep;
+	if (ent->unconfirmed)
+		read_info(ent);
+}
+
+static void check_unconfirmed(void)
+{
+	if (!have_unconfirmed)
+		return;
+	have_unconfirmed = 0;
+	twalk(tree_root, check_one);
+}
+
+int v4clients_process(fd_set *fdset, struct timeval *tv)
 {
 	char buf[4096] __attribute__((aligned(__alignof__(struct inotify_event))));
 	const struct inotify_event *ev;
 	ssize_t len;
 	char *ptr;
 
+	check_unconfirmed();
+
 	if (clients_fd < 0 ||
 	    !FD_ISSET(clients_fd, fdset))
 		return 0;
@@ -174,6 +224,12 @@ int v4clients_process(fd_set *fdset)
 				del_id(id);
 		}
 	}
+	if (have_unconfirmed && tv->tv_sec > unconfirmed_wait) {
+		tv->tv_sec = unconfirmed_wait;
+		tv->tv_usec = 0;
+		if (unconfirmed_wait < 300)
+			unconfirmed_wait <<= 1;
+	}
 	return 1;
 }
 
diff --git a/utils/mountd/svc_run.c b/utils/mountd/svc_run.c
index 167b9757bde2..042bed8957b9 100644
--- a/utils/mountd/svc_run.c
+++ b/utils/mountd/svc_run.c
@@ -95,6 +95,7 @@ my_svc_run(void)
 {
 	fd_set	readfds;
 	int	selret;
+	struct timeval tv = { 60*60, 0 };
 
 	for (;;) {
 
@@ -103,8 +104,10 @@ my_svc_run(void)
 		v4clients_set_fds(&readfds);
 
 		selret = select(FD_SETSIZE, &readfds,
-				(void *) 0, (void *) 0, (struct timeval *) 0);
+				(void *) 0, (void *) 0, &tv);
 
+		tv.tv_sec = 60*60;
+		tv.tv_usec = 0;
 
 		switch (selret) {
 		case -1:
@@ -116,7 +119,7 @@ my_svc_run(void)
 
 		default:
 			selret -= cache_process_req(&readfds);
-			selret -= v4clients_process(&readfds);
+			selret -= v4clients_process(&readfds, &tv);
 			if (selret)
 				svc_getreqset(&readfds);
 		}
-- 
2.30.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[J. Bruce Fields]

On Fri, Mar 19, 2021 at 02:36:24PM +1100, NeilBrown wrote:
> On Mon, Mar 01 2021, J. Bruce Fields wrote:
> 
> > On Tue, Mar 02, 2021 at 02:01:36PM +1100, NeilBrown wrote:
> >> On Mon, Mar 01 2021, J. Bruce Fields wrote:
> >> 
> >> > I've gotten requests for similar functionality, and intended to
> >> > implement it using directory notifications on /proc/fs/nfsd/clients.
> >> 
> >> I've been exploring this a bit.
> >> When I mount a filesystem, 2 clients get created.
> >> With NFSv4.0, the second client is immediately deleted, and the first
> >> client is deleted one grace period after the filesystem is unmounted.
> >> With NFSv4.1 and 4.2, the first client is immediately deleted, and the
> >> second client is deleted immediately after the unmount.
> >
> > Yeah, internally it's creating an "unconfirmed client" on SETCLIENTID
> > (or EXCHANGE_ID) and then a new "confirmed client" on
> > SETCLIENTID_CONFIRM (or CREATE_SESSION).
> >
> > I'm not sure why the ordering's a little different between the 4.0/4.1+
> > cases.
> 
> The multiple clients are not really nfsd's "fault".  The Linux NFS
> client sends multiple EXCHANGE_ID or SET_CLIENT_ID requests, so NFSD
> really does need to create multiple clients.
> 
> For NFSv4.0, when nfsd gets a repeat SET_CLIENT_ID, it keeps the old one
> and discards the new.
> For NFSv4.1, the spec requires that it keep the new one and discard the
> old.
> This explains the different ordering.

Hm, is this the client's trunking-detection logic?

In which case, it's not just unconfirmed clients.

> So the clean up the logging, mountd needs to be able to see the
> confirmation status.

That sounds fine.

(The other possibility might be to just not expose clients till they're
confirmed.  I don't know if unconfirmed clients are really that
interesting.  But I guess I'd rather err on the side of exposing more
information here.)

> Following this reply will be a patch to nfsd to provide this status, and
> a patch to mountd/exportd to use this status.

Re: [PATCH] mountd/exportd: only log confirmed clients, and poll for updates

[J. Bruce Fields]

On Fri, Mar 19, 2021 at 02:38:25PM +1100, NeilBrown wrote:
> 
> It is possible (and common with the Linux NFS client) for the nfs server
> to receive multiple SET_CLIENT_ID or EXCHANGE_ID requests when starting
> a connection.  This results in some clients appearing in
>  /proc/fs/nfsd/clients
> which never get confirmed.  mountd currently logs these, but they aren't
> really helpful.
> 
> If the kernel supports the reporting of the confirmation status of
> clients, we can suppress the message until a client is confirmed.
> 
> With this patch we:
>  - record if the client is confirmed, assuming it is if the status is
>     not reported
>  - don't log unconfirmed clients
>  - check all unconfirmed clients each time any event is processed,
>  - if there are unconfirmed clients, we request a wakeup after a
>    exponentially decreasing time, and check again

increasing not decreasing, I think.

Is there any better way to let userland know when the contents of a
virtual file have changed?

Looks at inofity man page....  There's an "IN_MODIFY" event.  I think we
could add an fsnotify_inode(inode, FS_MODIFY); at the end of
move_to_confirmed().  (I'm not sure what's the best way to get the inode
of the info file there.)

Would that help?

--b.

> This requires updating the two event loops to allow a timeout to be
> specified.
> 
> When there is no other activity, but there are unconfirmed clients,
> timeout for repeat checks are after 1, 2, 4 ... 512 seconds.
> If any wait is interrupted, the next wait time is still chosen, so we
> might advance quickly through the list.  But if there is lots of
> activity, we don't really need the timeout.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  support/export/cache.c     |  7 ++-
>  support/export/export.h    |  2 +-
>  support/export/v4clients.c | 92 ++++++++++++++++++++++++++++++--------
>  utils/mountd/svc_run.c     |  7 ++-
>  4 files changed, 85 insertions(+), 23 deletions(-)
> 
> diff --git a/support/export/cache.c b/support/export/cache.c
> index 3e4f53c0a32e..687b3aef27a1 100644
> --- a/support/export/cache.c
> +++ b/support/export/cache.c
> @@ -1524,6 +1524,7 @@ void cache_process_loop(void)
>  {
>  	fd_set	readfds;
>  	int	selret;
> +	struct timeval tv = { 60*60, 0 };
>  
>  	FD_ZERO(&readfds);
>  
> @@ -1533,8 +1534,10 @@ void cache_process_loop(void)
>  		v4clients_set_fds(&readfds);
>  
>  		selret = select(FD_SETSIZE, &readfds,
> -				(void *) 0, (void *) 0, (struct timeval *) 0);
> +				(void *) 0, (void *) 0, &tv);
>  
> +		tv.tv_sec = 60*60;
> +		tv.tv_usec = 0;
>  
>  		switch (selret) {
>  		case -1:
> @@ -1546,7 +1549,7 @@ void cache_process_loop(void)
>  
>  		default:
>  			cache_process_req(&readfds);
> -			v4clients_process(&readfds);
> +			v4clients_process(&readfds, &tv);
>  		}
>  	}
>  }
> diff --git a/support/export/export.h b/support/export/export.h
> index 8d5a0d3004ef..71d3ed39bd28 100644
> --- a/support/export/export.h
> +++ b/support/export/export.h
> @@ -24,7 +24,7 @@ void		cache_process_loop(void);
>  
>  void		v4clients_init(void);
>  void		v4clients_set_fds(fd_set *fdset);
> -int		v4clients_process(fd_set *fdset);
> +int		v4clients_process(fd_set *fdset, struct timeval *tv);
>  
>  struct nfs_fh_len *
>  		cache_get_filehandle(nfs_export *exp, int len, char *p);
> diff --git a/support/export/v4clients.c b/support/export/v4clients.c
> index 056ddc9b065d..04cec3136876 100644
> --- a/support/export/v4clients.c
> +++ b/support/export/v4clients.c
> @@ -48,12 +48,15 @@ void v4clients_set_fds(fd_set *fdset)
>  }
>  
>  static void *tree_root;
> +static int have_unconfirmed;
> +static time_t unconfirmed_wait = 1;
>  
>  struct ent {
>  	unsigned long num;
>  	char *clientid;
>  	char *addr;
>  	int vers;
> +	int unconfirmed;
>  };
>  
>  static int ent_cmp(const void *av, const void *bv)
> @@ -89,15 +92,13 @@ static char *dup_line(char *line)
>  	return ret;
>  }
>  
> -static void add_id(int id)
> +static void read_info(struct ent *key)
>  {
>  	char buf[2048];
> -	struct ent **ent;
> -	struct ent *key;
>  	char *path;
>  	FILE *f;
>  
> -	if (asprintf(&path, "/proc/fs/nfsd/clients/%d/info", id) < 0)
> +	if (asprintf(&path, "/proc/fs/nfsd/clients/%lu/info", key->num) < 0)
>  		return;
>  
>  	f = fopen(path, "r");
> @@ -105,32 +106,56 @@ static void add_id(int id)
>  		free(path);
>  		return;
>  	}
> -	key = calloc(1, sizeof(*key));
> -	if (!key) {
> -		fclose(f);
> -		free(path);
> -		return;
> -	}
> -	key->num = id;
>  	while (fgets(buf, sizeof(buf), f)) {
> -		if (strncmp(buf, "clientid: ", 10) == 0)
> +		if (strncmp(buf, "clientid: ", 10) == 0) {
> +			free(key->clientid);
>  			key->clientid = dup_line(buf+10);
> -		if (strncmp(buf, "address: ", 9) == 0)
> +		}
> +		if (strncmp(buf, "address: ", 9) == 0) {
> +			free(key->addr);
>  			key->addr = dup_line(buf+9);
> +		}
>  		if (strncmp(buf, "minor version: ", 15) == 0)
>  			key->vers = atoi(buf+15);
> +		if (strncmp(buf, "status: ", 8) == 0 &&
> +		    strstr(buf, " unconfirmed") != NULL) {
> +			key->unconfirmed = 1;
> +			have_unconfirmed = 1;
> +		}
> +		if (strncmp(buf, "status: ", 8) == 0 &&
> +		    strstr(buf, " confirmed") != NULL)
> +			key->unconfirmed = 0;
>  	}
>  	fclose(f);
>  	free(path);
>  
> -	xlog(L_NOTICE, "v4.%d client attached: %s from %s",
> -	     key->vers, key->clientid, key->addr);
> +	if (!key->unconfirmed)
> +		xlog(L_NOTICE, "v4.%d client attached: %s from %s",
> +		     key->vers, key->clientid ?: "-none-",
> +		     key->addr ?: "-none-");
> +}
> +
> +static void add_id(int id)
> +{
> +	struct ent **ent;
> +	struct ent *key;
> +
> +	key = calloc(1, sizeof(*key));
> +	if (!key) {
> +		return;
> +	}
> +	key->num = id;
>  
>  	ent = tsearch(key, &tree_root, ent_cmp);
>  
>  	if (!ent || *ent != key)
>  		/* Already existed, or insertion failed */
>  		free_ent(key);
> +	else {
> +		read_info(key);
> +		if (key->unconfirmed)
> +			unconfirmed_wait = 1;
> +	}
>  }
>  
>  static void del_id(int id)
> @@ -143,18 +168,43 @@ static void del_id(int id)
>  		return;
>  	ent = *e;
>  	tdelete(ent, &tree_root, ent_cmp);
> -	xlog(L_NOTICE, "v4.%d client detached: %s from %s",
> -	     ent->vers, ent->clientid, ent->addr);
> +	if (!ent->unconfirmed)
> +		xlog(L_NOTICE, "v4.%d client detached: %s from %s",
> +		     ent->vers, ent->clientid, ent->addr);
>  	free_ent(ent);
>  }
>  
> -int v4clients_process(fd_set *fdset)
> +static void check_one(const void *ventp, VISIT which, int depth)
> +{
> +	struct ent * const *ep = ventp;
> +	struct ent *ent;
> +
> +	if (depth >= 0 && /* Silence "unused paramter" warning */
> +	    which != preorder && which != leaf)
> +		return;
> +
> +	ent = *ep;
> +	if (ent->unconfirmed)
> +		read_info(ent);
> +}
> +
> +static void check_unconfirmed(void)
> +{
> +	if (!have_unconfirmed)
> +		return;
> +	have_unconfirmed = 0;
> +	twalk(tree_root, check_one);
> +}
> +
> +int v4clients_process(fd_set *fdset, struct timeval *tv)
>  {
>  	char buf[4096] __attribute__((aligned(__alignof__(struct inotify_event))));
>  	const struct inotify_event *ev;
>  	ssize_t len;
>  	char *ptr;
>  
> +	check_unconfirmed();
> +
>  	if (clients_fd < 0 ||
>  	    !FD_ISSET(clients_fd, fdset))
>  		return 0;
> @@ -174,6 +224,12 @@ int v4clients_process(fd_set *fdset)
>  				del_id(id);
>  		}
>  	}
> +	if (have_unconfirmed && tv->tv_sec > unconfirmed_wait) {
> +		tv->tv_sec = unconfirmed_wait;
> +		tv->tv_usec = 0;
> +		if (unconfirmed_wait < 300)
> +			unconfirmed_wait <<= 1;
> +	}
>  	return 1;
>  }
>  
> diff --git a/utils/mountd/svc_run.c b/utils/mountd/svc_run.c
> index 167b9757bde2..042bed8957b9 100644
> --- a/utils/mountd/svc_run.c
> +++ b/utils/mountd/svc_run.c
> @@ -95,6 +95,7 @@ my_svc_run(void)
>  {
>  	fd_set	readfds;
>  	int	selret;
> +	struct timeval tv = { 60*60, 0 };
>  
>  	for (;;) {
>  
> @@ -103,8 +104,10 @@ my_svc_run(void)
>  		v4clients_set_fds(&readfds);
>  
>  		selret = select(FD_SETSIZE, &readfds,
> -				(void *) 0, (void *) 0, (struct timeval *) 0);
> +				(void *) 0, (void *) 0, &tv);
>  
> +		tv.tv_sec = 60*60;
> +		tv.tv_usec = 0;
>  
>  		switch (selret) {
>  		case -1:
> @@ -116,7 +119,7 @@ my_svc_run(void)
>  
>  		default:
>  			selret -= cache_process_req(&readfds);
> -			selret -= v4clients_process(&readfds);
> +			selret -= v4clients_process(&readfds, &tv);
>  			if (selret)
>  				svc_getreqset(&readfds);
>  		}
> -- 
> 2.30.1
> 

Re: [PATCH] mountd/exportd: only log confirmed clients, and poll for updates

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]

On Fri, Mar 19 2021, J. Bruce Fields wrote:

> On Fri, Mar 19, 2021 at 02:38:25PM +1100, NeilBrown wrote:
>> 
>> It is possible (and common with the Linux NFS client) for the nfs server
>> to receive multiple SET_CLIENT_ID or EXCHANGE_ID requests when starting
>> a connection.  This results in some clients appearing in
>>  /proc/fs/nfsd/clients
>> which never get confirmed.  mountd currently logs these, but they aren't
>> really helpful.
>> 
>> If the kernel supports the reporting of the confirmation status of
>> clients, we can suppress the message until a client is confirmed.
>> 
>> With this patch we:
>>  - record if the client is confirmed, assuming it is if the status is
>>     not reported
>>  - don't log unconfirmed clients
>>  - check all unconfirmed clients each time any event is processed,
>>  - if there are unconfirmed clients, we request a wakeup after a
>>    exponentially decreasing time, and check again
>
> increasing not decreasing, I think.

or frequency, not time??  Thanks.

>
> Is there any better way to let userland know when the contents of a
> virtual file have changed?

i had thought of using sysfs_notify_dirent(), though the file isn't in
sysfs (or kernfs), so extra work might be needed.  And I didn't want to
hold the file descriptor open, as then I would need to worry about where
the available fds could be exhausted.

>
> Looks at inofity man page....  There's an "IN_MODIFY" event.  I think we
> could add an fsnotify_inode(inode, FS_MODIFY); at the end of
> move_to_confirmed().  (I'm not sure what's the best way to get the inode
> of the info file there.)
>
> Would that help?

Yes, that is a much better idea.
We could pass an array of dentry pointers to nfsd_client_mkdir and
thence to nfsdfs_create_files.
Then create_client could grab the required dentry and stash it next to
cl_nfsd_dentry.  Then use fsnotify_dirent() which is better than _inode
as the file name gets included in the event.

I might give that a try.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2592 bytes --]

On Fri, Mar 19 2021, J. Bruce Fields wrote:

> On Fri, Mar 19, 2021 at 02:36:24PM +1100, NeilBrown wrote:
>> On Mon, Mar 01 2021, J. Bruce Fields wrote:
>> 
>> > On Tue, Mar 02, 2021 at 02:01:36PM +1100, NeilBrown wrote:
>> >> On Mon, Mar 01 2021, J. Bruce Fields wrote:
>> >> 
>> >> > I've gotten requests for similar functionality, and intended to
>> >> > implement it using directory notifications on /proc/fs/nfsd/clients.
>> >> 
>> >> I've been exploring this a bit.
>> >> When I mount a filesystem, 2 clients get created.
>> >> With NFSv4.0, the second client is immediately deleted, and the first
>> >> client is deleted one grace period after the filesystem is unmounted.
>> >> With NFSv4.1 and 4.2, the first client is immediately deleted, and the
>> >> second client is deleted immediately after the unmount.
>> >
>> > Yeah, internally it's creating an "unconfirmed client" on SETCLIENTID
>> > (or EXCHANGE_ID) and then a new "confirmed client" on
>> > SETCLIENTID_CONFIRM (or CREATE_SESSION).
>> >
>> > I'm not sure why the ordering's a little different between the 4.0/4.1+
>> > cases.
>> 
>> The multiple clients are not really nfsd's "fault".  The Linux NFS
>> client sends multiple EXCHANGE_ID or SET_CLIENT_ID requests, so NFSD
>> really does need to create multiple clients.
>> 
>> For NFSv4.0, when nfsd gets a repeat SET_CLIENT_ID, it keeps the old one
>> and discards the new.
>> For NFSv4.1, the spec requires that it keep the new one and discard the
>> old.
>> This explains the different ordering.
>
> Hm, is this the client's trunking-detection logic?

Yes.

>
> In which case, it's not just unconfirmed clients.

For NFSv4.1, only the EXCHANGE_ID is duplicate.  There is only one
CREATE_SESSION, and that is where the client is confirmed.  So only one
confirmed client.

For NFSv4.0 bother SETCLIENTID and SETCLIENDID_CONFIRM are duplicate.
So maybe both clients get confirmed.  I should check that.

>
>> So the clean up the logging, mountd needs to be able to see the
>> confirmation status.
>
> That sounds fine.
>
> (The other possibility might be to just not expose clients till they're
> confirmed.  I don't know if unconfirmed clients are really that
> interesting.  But I guess I'd rather err on the side of exposing more
> information here.)

That was my feeling to: expose all the info, and filter out the
uninteresting bits a point of use.

Thanks,
NeilBrown

>
>> Following this reply will be a patch to nfsd to provide this status, and
>> a patch to mountd/exportd to use this status.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[J. Bruce Fields]

On Sat, Mar 20, 2021 at 07:48:44AM +1100, NeilBrown wrote:
> On Fri, Mar 19 2021, J. Bruce Fields wrote:
> 
> > On Fri, Mar 19, 2021 at 02:36:24PM +1100, NeilBrown wrote:
> >> On Mon, Mar 01 2021, J. Bruce Fields wrote:
> >> 
> >> > On Tue, Mar 02, 2021 at 02:01:36PM +1100, NeilBrown wrote:
> >> >> On Mon, Mar 01 2021, J. Bruce Fields wrote:
> >> >> 
> >> >> > I've gotten requests for similar functionality, and intended to
> >> >> > implement it using directory notifications on /proc/fs/nfsd/clients.
> >> >> 
> >> >> I've been exploring this a bit.
> >> >> When I mount a filesystem, 2 clients get created.
> >> >> With NFSv4.0, the second client is immediately deleted, and the first
> >> >> client is deleted one grace period after the filesystem is unmounted.
> >> >> With NFSv4.1 and 4.2, the first client is immediately deleted, and the
> >> >> second client is deleted immediately after the unmount.
> >> >
> >> > Yeah, internally it's creating an "unconfirmed client" on SETCLIENTID
> >> > (or EXCHANGE_ID) and then a new "confirmed client" on
> >> > SETCLIENTID_CONFIRM (or CREATE_SESSION).
> >> >
> >> > I'm not sure why the ordering's a little different between the 4.0/4.1+
> >> > cases.
> >> 
> >> The multiple clients are not really nfsd's "fault".  The Linux NFS
> >> client sends multiple EXCHANGE_ID or SET_CLIENT_ID requests, so NFSD
> >> really does need to create multiple clients.
> >> 
> >> For NFSv4.0, when nfsd gets a repeat SET_CLIENT_ID, it keeps the old one
> >> and discards the new.
> >> For NFSv4.1, the spec requires that it keep the new one and discard the
> >> old.
> >> This explains the different ordering.
> >
> > Hm, is this the client's trunking-detection logic?
> 
> Yes.
> 
> >
> > In which case, it's not just unconfirmed clients.
> 
> For NFSv4.1, only the EXCHANGE_ID is duplicate.  There is only one
> CREATE_SESSION, and that is where the client is confirmed.  So only one
> confirmed client.
> 
> For NFSv4.0 bother SETCLIENTID and SETCLIENDID_CONFIRM are duplicate.
> So maybe both clients get confirmed.  I should check that.

Drifting off topic, but I don't see how this client behavior makes
sense.  Mount is chatty enough without the unnecessary duplication.
Looking at the code....

--b.

[PATCH v2] nfsd: report client confirmation status in "info" file

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6196 bytes --]


mountd can now monitor clients appearing and disappearing in
/proc/fs/nfsd/clients, and will log these events, in liu of the logging
of mount/unmount events for NFSv3.

Currently it cannot distinguish between unconfirmed clients (which might
be transient and totally uninteresting) and confirmed clients.

So add a "status: " line which reports either "confirmed" or
"unconfirmed", and use fsnotify to report that the info file
has been modified.

This requires a bit of infrastructure to keep the dentry for the "info"
file.  There is no need to take a counted reference as the dentry must
remain around until the client is removed.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 19 +++++++++++++++----
 fs/nfsd/nfsctl.c    | 14 ++++++++------
 fs/nfsd/nfsd.h      |  4 +++-
 fs/nfsd/state.h     |  4 ++++
 4 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 97447a64bad0..ec1b2dd8fd45 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -43,6 +43,7 @@
 #include <linux/sunrpc/addr.h>
 #include <linux/jhash.h>
 #include <linux/string_helpers.h>
+#include <linux/fsnotify.h>
 #include "xdr4.h"
 #include "xdr4cb.h"
 #include "vfs.h"
@@ -2352,6 +2353,10 @@ static int client_info_show(struct seq_file *m, void *v)
 	memcpy(&clid, &clp->cl_clientid, sizeof(clid));
 	seq_printf(m, "clientid: 0x%llx\n", clid);
 	seq_printf(m, "address: \"%pISpc\"\n", (struct sockaddr *)&clp->cl_addr);
+	if (test_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags))
+		seq_puts(m, "status: confirmed\n");
+	else
+		seq_puts(m, "status: unconfirmed\n");
 	seq_printf(m, "name: ");
 	seq_quote_mem(m, clp->cl_name.data, clp->cl_name.len);
 	seq_printf(m, "\nminor version: %d\n", clp->cl_minorversion);
@@ -2702,6 +2707,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name,
 	int ret;
 	struct net *net = SVC_NET(rqstp);
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+	struct dentry *dentries[ARRAY_SIZE(client_files)];
 
 	clp = alloc_client(name);
 	if (clp == NULL)
@@ -2721,9 +2727,11 @@ static struct nfs4_client *create_client(struct xdr_netobj name,
 	memcpy(&clp->cl_addr, sa, sizeof(struct sockaddr_storage));
 	clp->cl_cb_session = NULL;
 	clp->net = net;
-	clp->cl_nfsd_dentry = nfsd_client_mkdir(nn, &clp->cl_nfsdfs,
-			clp->cl_clientid.cl_id - nn->clientid_base,
-			client_files);
+	clp->cl_nfsd_dentry = nfsd_client_mkdir(
+		nn, &clp->cl_nfsdfs,
+		clp->cl_clientid.cl_id - nn->clientid_base,
+		client_files, dentries);
+	clp->cl_nfsd_info_dentry = dentries[0];
 	if (!clp->cl_nfsd_dentry) {
 		free_client(clp);
 		return NULL;
@@ -2798,7 +2806,10 @@ move_to_confirmed(struct nfs4_client *clp)
 	list_move(&clp->cl_idhash, &nn->conf_id_hashtbl[idhashval]);
 	rb_erase(&clp->cl_namenode, &nn->unconf_name_tree);
 	add_clp_to_name_tree(clp, &nn->conf_name_tree);
-	set_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags);
+	if (!test_and_set_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags) &&
+	    clp->cl_nfsd_dentry &&
+	    clp->cl_nfsd_info_dentry)
+		fsnotify_dentry(clp->cl_nfsd_info_dentry, FS_MODIFY);
 	renew_client_locked(clp);
 }
 
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index ef86ed23af82..94ce1eabd9d1 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1266,7 +1266,8 @@ static void nfsdfs_remove_files(struct dentry *root)
 /* XXX: cut'n'paste from simple_fill_super; figure out if we could share
  * code instead. */
 static  int nfsdfs_create_files(struct dentry *root,
-					const struct tree_descr *files)
+				const struct tree_descr *files,
+				struct dentry **fdentries)
 {
 	struct inode *dir = d_inode(root);
 	struct inode *inode;
@@ -1275,8 +1276,6 @@ static  int nfsdfs_create_files(struct dentry *root,
 
 	inode_lock(dir);
 	for (i = 0; files->name && files->name[0]; i++, files++) {
-		if (!files->name)
-			continue;
 		dentry = d_alloc_name(root, files->name);
 		if (!dentry)
 			goto out;
@@ -1290,6 +1289,8 @@ static  int nfsdfs_create_files(struct dentry *root,
 		inode->i_private = __get_nfsdfs_client(dir);
 		d_add(dentry, inode);
 		fsnotify_create(dir, dentry);
+		if (fdentries)
+			fdentries[i] = dentry;
 	}
 	inode_unlock(dir);
 	return 0;
@@ -1301,8 +1302,9 @@ static  int nfsdfs_create_files(struct dentry *root,
 
 /* on success, returns positive number unique to that client. */
 struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
-		struct nfsdfs_client *ncl, u32 id,
-		const struct tree_descr *files)
+				 struct nfsdfs_client *ncl, u32 id,
+				 const struct tree_descr *files,
+				 struct dentry **fdentries)
 {
 	struct dentry *dentry;
 	char name[11];
@@ -1313,7 +1315,7 @@ struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
 	dentry = nfsd_mkdir(nn->nfsd_client_dir, ncl, name);
 	if (IS_ERR(dentry)) /* XXX: tossing errors? */
 		return NULL;
-	ret = nfsdfs_create_files(dentry, files);
+	ret = nfsdfs_create_files(dentry, files, fdentries);
 	if (ret) {
 		nfsd_client_rmdir(dentry);
 		return NULL;
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 8bdc37aa2c2e..9aee72f65330 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -107,7 +107,9 @@ struct nfsdfs_client {
 
 struct nfsdfs_client *get_nfsdfs_client(struct inode *);
 struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
-		struct nfsdfs_client *ncl, u32 id, const struct tree_descr *);
+				 struct nfsdfs_client *ncl, u32 id,
+				 const struct tree_descr *,
+				 struct dentry **fdentries);
 void nfsd_client_rmdir(struct dentry *dentry);
 
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 73deea353169..54cab651ac1d 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -371,6 +371,10 @@ struct nfs4_client {
 
 	/* debugging info directory under nfsd/clients/ : */
 	struct dentry		*cl_nfsd_dentry;
+	/* 'info' file within that directory. Ref is not counted,
+	 * but will remain valid iff cl_nfsd_dentry != NULL
+	 */
+	struct dentry		*cl_nfsd_info_dentry;
 
 	/* for nfs41 callbacks */
 	/* We currently support a single back channel with a single slot */
-- 
2.30.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

[PATCH v2] mountd/exportd: only log confirmed clients, and poll for updates

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4759 bytes --]


It is possible (and common with the Linux NFS client) for the nfs server
to receive multiple SET_CLIENT_ID or EXCHANGE_ID requests when starting
a connection.  This results in some clients appearing in
 /proc/fs/nfsd/clients
which never get confirmed.  mountd currently logs these, but they aren't
really helpful.

If the kernel supports the reporting of the confirmation status of
clients, we can suppress the message until a client is confirmed.

With this patch we:
 - record if the client is confirmed, assuming it is if the status is
    not reported
 - don't log unconfirmed clients
 - request MODIFY notification from unconfirmed clients.
 - recheck an info file when it is modified.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 support/export/v4clients.c | 86 +++++++++++++++++++++++++++++---------
 1 file changed, 67 insertions(+), 19 deletions(-)

diff --git a/support/export/v4clients.c b/support/export/v4clients.c
index 056ddc9b065d..f2c9bb482ba7 100644
--- a/support/export/v4clients.c
+++ b/support/export/v4clients.c
@@ -48,12 +48,15 @@ void v4clients_set_fds(fd_set *fdset)
 }
 
 static void *tree_root;
+static int have_unconfirmed;
 
 struct ent {
 	unsigned long num;
 	char *clientid;
 	char *addr;
 	int vers;
+	int unconfirmed;
+	int wid;
 };
 
 static int ent_cmp(const void *av, const void *bv)
@@ -89,15 +92,14 @@ static char *dup_line(char *line)
 	return ret;
 }
 
-static void add_id(int id)
+static void read_info(struct ent *key)
 {
 	char buf[2048];
-	struct ent **ent;
-	struct ent *key;
 	char *path;
+	int was_unconfirmed = key->unconfirmed;
 	FILE *f;
 
-	if (asprintf(&path, "/proc/fs/nfsd/clients/%d/info", id) < 0)
+	if (asprintf(&path, "/proc/fs/nfsd/clients/%lu/info", key->num) < 0)
 		return;
 
 	f = fopen(path, "r");
@@ -105,35 +107,64 @@ static void add_id(int id)
 		free(path);
 		return;
 	}
-	key = calloc(1, sizeof(*key));
-	if (!key) {
-		fclose(f);
-		free(path);
-		return;
-	}
-	key->num = id;
+	if (key->wid < 0)
+		key->wid = inotify_add_watch(clients_fd, path, IN_MODIFY);
+
 	while (fgets(buf, sizeof(buf), f)) {
-		if (strncmp(buf, "clientid: ", 10) == 0)
+		if (strncmp(buf, "clientid: ", 10) == 0) {
+			free(key->clientid);
 			key->clientid = dup_line(buf+10);
-		if (strncmp(buf, "address: ", 9) == 0)
+		}
+		if (strncmp(buf, "address: ", 9) == 0) {
+			free(key->addr);
 			key->addr = dup_line(buf+9);
+		}
 		if (strncmp(buf, "minor version: ", 15) == 0)
 			key->vers = atoi(buf+15);
+		if (strncmp(buf, "status: ", 8) == 0 &&
+		    strstr(buf, " unconfirmed") != NULL) {
+			key->unconfirmed = 1;
+			have_unconfirmed = 1;
+		}
+		if (strncmp(buf, "status: ", 8) == 0 &&
+		    strstr(buf, " confirmed") != NULL)
+			key->unconfirmed = 0;
 	}
 	fclose(f);
 	free(path);
 
-	xlog(L_NOTICE, "v4.%d client attached: %s from %s",
-	     key->vers, key->clientid, key->addr);
+	if (was_unconfirmed && !key->unconfirmed)
+		xlog(L_NOTICE, "v4.%d client attached: %s from %s",
+		     key->vers, key->clientid ?: "-none-",
+		     key->addr ?: "-none-");
+	if (!key->unconfirmed && ent->wid >= 0) {
+		inotify_rm_watch(clients_fd, ent->wid);
+		ent->wid = -1;
+	}
+}
+
+static void add_id(int id)
+{
+	struct ent **ent;
+	struct ent *key;
+
+	key = calloc(1, sizeof(*key));
+	if (!key) {
+		return;
+	}
+	key->num = id;
+	key->wid = -1;
 
 	ent = tsearch(key, &tree_root, ent_cmp);
 
 	if (!ent || *ent != key)
 		/* Already existed, or insertion failed */
 		free_ent(key);
+	else
+		read_info(key);
 }
 
-static void del_id(int id)
+static void del_id(unsigned long id)
 {
 	struct ent key = {.num = id};
 	struct ent **e, *ent;
@@ -143,11 +174,27 @@ static void del_id(int id)
 		return;
 	ent = *e;
 	tdelete(ent, &tree_root, ent_cmp);
-	xlog(L_NOTICE, "v4.%d client detached: %s from %s",
-	     ent->vers, ent->clientid, ent->addr);
+	if (!ent->unconfirmed)
+		xlog(L_NOTICE, "v4.%d client detached: %s from %s",
+		     ent->vers, ent->clientid, ent->addr);
+	if (ent->wid >= 0)
+		inotify_rm_watch(clients_fd, ent->wid);
 	free_ent(ent);
 }
 
+static void check_id(unsigned long id)
+{
+	struct ent key = {.num = id};
+	struct ent **e, *ent;
+
+	e = tfind(&key, &tree_root, ent_cmp);
+	if (!e || !*e)
+		return;
+	ent = *e;
+	if (ent->unconfirmed)
+		read_info(ent);
+}
+
 int v4clients_process(fd_set *fdset)
 {
 	char buf[4096] __attribute__((aligned(__alignof__(struct inotify_event))));
@@ -172,8 +219,9 @@ int v4clients_process(fd_set *fdset)
 				add_id(id);
 			if (ev->mask & IN_DELETE)
 				del_id(id);
+			if (ev->mask & IN_MODIFY)
+				check_id(id);
 		}
 	}
 	return 1;
 }
-
-- 
2.30.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

Re: [PATCH v2] mountd/exportd: only log confirmed clients, and poll for updates

[Chuck Lever III]

> On Mar 19, 2021, at 6:39 PM, NeilBrown <neilb@suse.de> wrote:
> 
> 
> It is possible (and common with the Linux NFS client) for the nfs server
> to receive multiple SET_CLIENT_ID or EXCHANGE_ID requests when starting
> a connection.  This results in some clients appearing in
> /proc/fs/nfsd/clients
> which never get confirmed.  mountd currently logs these, but they aren't
> really helpful.
> 
> If the kernel supports the reporting of the confirmation status of
> clients, we can suppress the message until a client is confirmed.
> 
> With this patch we:
> - record if the client is confirmed, assuming it is if the status is
>    not reported
> - don't log unconfirmed clients
> - request MODIFY notification from unconfirmed clients.
> - recheck an info file when it is modified.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>

Hello Neil!

I've committed v2 of this patch to the for-next topic branch at

git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git


> ---
> support/export/v4clients.c | 86 +++++++++++++++++++++++++++++---------
> 1 file changed, 67 insertions(+), 19 deletions(-)
> 
> diff --git a/support/export/v4clients.c b/support/export/v4clients.c
> index 056ddc9b065d..f2c9bb482ba7 100644
> --- a/support/export/v4clients.c
> +++ b/support/export/v4clients.c
> @@ -48,12 +48,15 @@ void v4clients_set_fds(fd_set *fdset)
> }
> 
> static void *tree_root;
> +static int have_unconfirmed;
> 
> struct ent {
> 	unsigned long num;
> 	char *clientid;
> 	char *addr;
> 	int vers;
> +	int unconfirmed;
> +	int wid;
> };
> 
> static int ent_cmp(const void *av, const void *bv)
> @@ -89,15 +92,14 @@ static char *dup_line(char *line)
> 	return ret;
> }
> 
> -static void add_id(int id)
> +static void read_info(struct ent *key)
> {
> 	char buf[2048];
> -	struct ent **ent;
> -	struct ent *key;
> 	char *path;
> +	int was_unconfirmed = key->unconfirmed;
> 	FILE *f;
> 
> -	if (asprintf(&path, "/proc/fs/nfsd/clients/%d/info", id) < 0)
> +	if (asprintf(&path, "/proc/fs/nfsd/clients/%lu/info", key->num) < 0)
> 		return;
> 
> 	f = fopen(path, "r");
> @@ -105,35 +107,64 @@ static void add_id(int id)
> 		free(path);
> 		return;
> 	}
> -	key = calloc(1, sizeof(*key));
> -	if (!key) {
> -		fclose(f);
> -		free(path);
> -		return;
> -	}
> -	key->num = id;
> +	if (key->wid < 0)
> +		key->wid = inotify_add_watch(clients_fd, path, IN_MODIFY);
> +
> 	while (fgets(buf, sizeof(buf), f)) {
> -		if (strncmp(buf, "clientid: ", 10) == 0)
> +		if (strncmp(buf, "clientid: ", 10) == 0) {
> +			free(key->clientid);
> 			key->clientid = dup_line(buf+10);
> -		if (strncmp(buf, "address: ", 9) == 0)
> +		}
> +		if (strncmp(buf, "address: ", 9) == 0) {
> +			free(key->addr);
> 			key->addr = dup_line(buf+9);
> +		}
> 		if (strncmp(buf, "minor version: ", 15) == 0)
> 			key->vers = atoi(buf+15);
> +		if (strncmp(buf, "status: ", 8) == 0 &&
> +		    strstr(buf, " unconfirmed") != NULL) {
> +			key->unconfirmed = 1;
> +			have_unconfirmed = 1;
> +		}
> +		if (strncmp(buf, "status: ", 8) == 0 &&
> +		    strstr(buf, " confirmed") != NULL)
> +			key->unconfirmed = 0;
> 	}
> 	fclose(f);
> 	free(path);
> 
> -	xlog(L_NOTICE, "v4.%d client attached: %s from %s",
> -	     key->vers, key->clientid, key->addr);
> +	if (was_unconfirmed && !key->unconfirmed)
> +		xlog(L_NOTICE, "v4.%d client attached: %s from %s",
> +		     key->vers, key->clientid ?: "-none-",
> +		     key->addr ?: "-none-");
> +	if (!key->unconfirmed && ent->wid >= 0) {
> +		inotify_rm_watch(clients_fd, ent->wid);
> +		ent->wid = -1;
> +	}
> +}
> +
> +static void add_id(int id)
> +{
> +	struct ent **ent;
> +	struct ent *key;
> +
> +	key = calloc(1, sizeof(*key));
> +	if (!key) {
> +		return;
> +	}
> +	key->num = id;
> +	key->wid = -1;
> 
> 	ent = tsearch(key, &tree_root, ent_cmp);
> 
> 	if (!ent || *ent != key)
> 		/* Already existed, or insertion failed */
> 		free_ent(key);
> +	else
> +		read_info(key);
> }
> 
> -static void del_id(int id)
> +static void del_id(unsigned long id)
> {
> 	struct ent key = {.num = id};
> 	struct ent **e, *ent;
> @@ -143,11 +174,27 @@ static void del_id(int id)
> 		return;
> 	ent = *e;
> 	tdelete(ent, &tree_root, ent_cmp);
> -	xlog(L_NOTICE, "v4.%d client detached: %s from %s",
> -	     ent->vers, ent->clientid, ent->addr);
> +	if (!ent->unconfirmed)
> +		xlog(L_NOTICE, "v4.%d client detached: %s from %s",
> +		     ent->vers, ent->clientid, ent->addr);
> +	if (ent->wid >= 0)
> +		inotify_rm_watch(clients_fd, ent->wid);
> 	free_ent(ent);
> }
> 
> +static void check_id(unsigned long id)
> +{
> +	struct ent key = {.num = id};
> +	struct ent **e, *ent;
> +
> +	e = tfind(&key, &tree_root, ent_cmp);
> +	if (!e || !*e)
> +		return;
> +	ent = *e;
> +	if (ent->unconfirmed)
> +		read_info(ent);
> +}
> +
> int v4clients_process(fd_set *fdset)
> {
> 	char buf[4096] __attribute__((aligned(__alignof__(struct inotify_event))));
> @@ -172,8 +219,9 @@ int v4clients_process(fd_set *fdset)
> 				add_id(id);
> 			if (ev->mask & IN_DELETE)
> 				del_id(id);
> +			if (ev->mask & IN_MODIFY)
> +				check_id(id);
> 		}
> 	}
> 	return 1;
> }
> -
> -- 
> 2.30.1
> 

--
Chuck Lever

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[J. Bruce Fields]

On Fri, Mar 19, 2021 at 05:09:22PM -0400, J. Bruce Fields wrote:
> On Sat, Mar 20, 2021 at 07:48:44AM +1100, NeilBrown wrote:
> > For NFSv4.1, only the EXCHANGE_ID is duplicate.  There is only one
> > CREATE_SESSION, and that is where the client is confirmed.  So only one
> > confirmed client.
> > 
> > For NFSv4.0 bother SETCLIENTID and SETCLIENDID_CONFIRM are duplicate.
> > So maybe both clients get confirmed.  I should check that.
> 
> Drifting off topic, but I don't see how this client behavior makes
> sense.  Mount is chatty enough without the unnecessary duplication.
> Looking at the code....

I spent a little time tracing through the code and couldn't figure out
what's going on.

Just a note for the future that it'd be worth figuring out why the
client is repeating SETCLIENTID+SETCLIENTID_CONFIRM or
EXCHANGE_ID+CREATE_SESSION.  I understand why it might be needed for
trunking detection when there are multiple addresses involved, but
otherwise it seems unnecessary.

--b.

Re: [PATCH v2] mountd/exportd: only log confirmed clients, and poll for updates

[Steve Dickson]

On 3/19/21 6:39 PM, NeilBrown wrote:
> 
> It is possible (and common with the Linux NFS client) for the nfs server
> to receive multiple SET_CLIENT_ID or EXCHANGE_ID requests when starting
> a connection.  This results in some clients appearing in
>  /proc/fs/nfsd/clients
> which never get confirmed.  mountd currently logs these, but they aren't
> really helpful.
> 
> If the kernel supports the reporting of the confirmation status of
> clients, we can suppress the message until a client is confirmed.
> 
> With this patch we:
>  - record if the client is confirmed, assuming it is if the status is
>     not reported
>  - don't log unconfirmed clients
>  - request MODIFY notification from unconfirmed clients.
>  - recheck an info file when it is modified.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
Sorry it took so long... PTO got in the way!

Committed... (tag: nfs-utils-2-5-4-rc2)

steved.
> ---
>  support/export/v4clients.c | 86 +++++++++++++++++++++++++++++---------
>  1 file changed, 67 insertions(+), 19 deletions(-)
> 
> diff --git a/support/export/v4clients.c b/support/export/v4clients.c
> index 056ddc9b065d..f2c9bb482ba7 100644
> --- a/support/export/v4clients.c
> +++ b/support/export/v4clients.c
> @@ -48,12 +48,15 @@ void v4clients_set_fds(fd_set *fdset)
>  }
>  
>  static void *tree_root;
> +static int have_unconfirmed;
>  
>  struct ent {
>  	unsigned long num;
>  	char *clientid;
>  	char *addr;
>  	int vers;
> +	int unconfirmed;
> +	int wid;
>  };
>  
>  static int ent_cmp(const void *av, const void *bv)
> @@ -89,15 +92,14 @@ static char *dup_line(char *line)
>  	return ret;
>  }
>  
> -static void add_id(int id)
> +static void read_info(struct ent *key)
>  {
>  	char buf[2048];
> -	struct ent **ent;
> -	struct ent *key;
>  	char *path;
> +	int was_unconfirmed = key->unconfirmed;
>  	FILE *f;
>  
> -	if (asprintf(&path, "/proc/fs/nfsd/clients/%d/info", id) < 0)
> +	if (asprintf(&path, "/proc/fs/nfsd/clients/%lu/info", key->num) < 0)
>  		return;
>  
>  	f = fopen(path, "r");
> @@ -105,35 +107,64 @@ static void add_id(int id)
>  		free(path);
>  		return;
>  	}
> -	key = calloc(1, sizeof(*key));
> -	if (!key) {
> -		fclose(f);
> -		free(path);
> -		return;
> -	}
> -	key->num = id;
> +	if (key->wid < 0)
> +		key->wid = inotify_add_watch(clients_fd, path, IN_MODIFY);
> +
>  	while (fgets(buf, sizeof(buf), f)) {
> -		if (strncmp(buf, "clientid: ", 10) == 0)
> +		if (strncmp(buf, "clientid: ", 10) == 0) {
> +			free(key->clientid);
>  			key->clientid = dup_line(buf+10);
> -		if (strncmp(buf, "address: ", 9) == 0)
> +		}
> +		if (strncmp(buf, "address: ", 9) == 0) {
> +			free(key->addr);
>  			key->addr = dup_line(buf+9);
> +		}
>  		if (strncmp(buf, "minor version: ", 15) == 0)
>  			key->vers = atoi(buf+15);
> +		if (strncmp(buf, "status: ", 8) == 0 &&
> +		    strstr(buf, " unconfirmed") != NULL) {
> +			key->unconfirmed = 1;
> +			have_unconfirmed = 1;
> +		}
> +		if (strncmp(buf, "status: ", 8) == 0 &&
> +		    strstr(buf, " confirmed") != NULL)
> +			key->unconfirmed = 0;
>  	}
>  	fclose(f);
>  	free(path);
>  
> -	xlog(L_NOTICE, "v4.%d client attached: %s from %s",
> -	     key->vers, key->clientid, key->addr);
> +	if (was_unconfirmed && !key->unconfirmed)
> +		xlog(L_NOTICE, "v4.%d client attached: %s from %s",
> +		     key->vers, key->clientid ?: "-none-",
> +		     key->addr ?: "-none-");
> +	if (!key->unconfirmed && ent->wid >= 0) {
> +		inotify_rm_watch(clients_fd, ent->wid);
> +		ent->wid = -1;
> +	}
> +}
> +
> +static void add_id(int id)
> +{
> +	struct ent **ent;
> +	struct ent *key;
> +
> +	key = calloc(1, sizeof(*key));
> +	if (!key) {
> +		return;
> +	}
> +	key->num = id;
> +	key->wid = -1;
>  
>  	ent = tsearch(key, &tree_root, ent_cmp);
>  
>  	if (!ent || *ent != key)
>  		/* Already existed, or insertion failed */
>  		free_ent(key);
> +	else
> +		read_info(key);
>  }
>  
> -static void del_id(int id)
> +static void del_id(unsigned long id)
>  {
>  	struct ent key = {.num = id};
>  	struct ent **e, *ent;
> @@ -143,11 +174,27 @@ static void del_id(int id)
>  		return;
>  	ent = *e;
>  	tdelete(ent, &tree_root, ent_cmp);
> -	xlog(L_NOTICE, "v4.%d client detached: %s from %s",
> -	     ent->vers, ent->clientid, ent->addr);
> +	if (!ent->unconfirmed)
> +		xlog(L_NOTICE, "v4.%d client detached: %s from %s",
> +		     ent->vers, ent->clientid, ent->addr);
> +	if (ent->wid >= 0)
> +		inotify_rm_watch(clients_fd, ent->wid);
>  	free_ent(ent);
>  }
>  
> +static void check_id(unsigned long id)
> +{
> +	struct ent key = {.num = id};
> +	struct ent **e, *ent;
> +
> +	e = tfind(&key, &tree_root, ent_cmp);
> +	if (!e || !*e)
> +		return;
> +	ent = *e;
> +	if (ent->unconfirmed)
> +		read_info(ent);
> +}
> +
>  int v4clients_process(fd_set *fdset)
>  {
>  	char buf[4096] __attribute__((aligned(__alignof__(struct inotify_event))));
> @@ -172,8 +219,9 @@ int v4clients_process(fd_set *fdset)
>  				add_id(id);
>  			if (ev->mask & IN_DELETE)
>  				del_id(id);
> +			if (ev->mask & IN_MODIFY)
> +				check_id(id);
>  		}
>  	}
>  	return 1;
>  }
> -
> 

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[J. Bruce Fields]

On Fri, Mar 19, 2021 at 05:09:22PM -0400, J. Bruce Fields wrote:
> On Sat, Mar 20, 2021 at 07:48:44AM +1100, NeilBrown wrote:
> > On Fri, Mar 19 2021, J. Bruce Fields wrote:
> > 
> > > On Fri, Mar 19, 2021 at 02:36:24PM +1100, NeilBrown wrote:
> > >> On Mon, Mar 01 2021, J. Bruce Fields wrote:
> > >> 
> > >> > On Tue, Mar 02, 2021 at 02:01:36PM +1100, NeilBrown wrote:
> > >> >> On Mon, Mar 01 2021, J. Bruce Fields wrote:
> > >> >> 
> > >> >> > I've gotten requests for similar functionality, and intended to
> > >> >> > implement it using directory notifications on /proc/fs/nfsd/clients.
> > >> >> 
> > >> >> I've been exploring this a bit.
> > >> >> When I mount a filesystem, 2 clients get created.
> > >> >> With NFSv4.0, the second client is immediately deleted, and the first
> > >> >> client is deleted one grace period after the filesystem is unmounted.
> > >> >> With NFSv4.1 and 4.2, the first client is immediately deleted, and the
> > >> >> second client is deleted immediately after the unmount.
> > >> >
> > >> > Yeah, internally it's creating an "unconfirmed client" on SETCLIENTID
> > >> > (or EXCHANGE_ID) and then a new "confirmed client" on
> > >> > SETCLIENTID_CONFIRM (or CREATE_SESSION).
> > >> >
> > >> > I'm not sure why the ordering's a little different between the 4.0/4.1+
> > >> > cases.
> > >> 
> > >> The multiple clients are not really nfsd's "fault".  The Linux NFS
> > >> client sends multiple EXCHANGE_ID or SET_CLIENT_ID requests, so NFSD
> > >> really does need to create multiple clients.
> > >> 
> > >> For NFSv4.0, when nfsd gets a repeat SET_CLIENT_ID, it keeps the old one
> > >> and discards the new.
> > >> For NFSv4.1, the spec requires that it keep the new one and discard the
> > >> old.
> > >> This explains the different ordering.
> > >
> > > Hm, is this the client's trunking-detection logic?
> > 
> > Yes.
> > 
> > >
> > > In which case, it's not just unconfirmed clients.
> > 
> > For NFSv4.1, only the EXCHANGE_ID is duplicate.  There is only one
> > CREATE_SESSION, and that is where the client is confirmed.  So only one
> > confirmed client.
> > 
> > For NFSv4.0 bother SETCLIENTID and SETCLIENDID_CONFIRM are duplicate.
> > So maybe both clients get confirmed.  I should check that.
> 
> Drifting off topic, but I don't see how this client behavior makes
> sense.  Mount is chatty enough without the unnecessary duplication.
> Looking at the code....

I never sorted this out, by the way.  I think there must be a bug,
though.

--b.

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[Steve Dickson]

On 4/7/21 3:14 PM, J. Bruce Fields wrote:
> On Fri, Mar 19, 2021 at 05:09:22PM -0400, J. Bruce Fields wrote:
>> On Sat, Mar 20, 2021 at 07:48:44AM +1100, NeilBrown wrote:
>>> On Fri, Mar 19 2021, J. Bruce Fields wrote:
>>>
>>>> On Fri, Mar 19, 2021 at 02:36:24PM +1100, NeilBrown wrote:
>>>>> On Mon, Mar 01 2021, J. Bruce Fields wrote:
>>>>>
>>>>>> On Tue, Mar 02, 2021 at 02:01:36PM +1100, NeilBrown wrote:
>>>>>>> On Mon, Mar 01 2021, J. Bruce Fields wrote:
>>>>>>>
>>>>>>>> I've gotten requests for similar functionality, and intended to
>>>>>>>> implement it using directory notifications on /proc/fs/nfsd/clients.
>>>>>>>
>>>>>>> I've been exploring this a bit.
>>>>>>> When I mount a filesystem, 2 clients get created.
>>>>>>> With NFSv4.0, the second client is immediately deleted, and the first
>>>>>>> client is deleted one grace period after the filesystem is unmounted.
>>>>>>> With NFSv4.1 and 4.2, the first client is immediately deleted, and the
>>>>>>> second client is deleted immediately after the unmount.
>>>>>>
>>>>>> Yeah, internally it's creating an "unconfirmed client" on SETCLIENTID
>>>>>> (or EXCHANGE_ID) and then a new "confirmed client" on
>>>>>> SETCLIENTID_CONFIRM (or CREATE_SESSION).
>>>>>>
>>>>>> I'm not sure why the ordering's a little different between the 4.0/4.1+
>>>>>> cases.
>>>>>
>>>>> The multiple clients are not really nfsd's "fault".  The Linux NFS
>>>>> client sends multiple EXCHANGE_ID or SET_CLIENT_ID requests, so NFSD
>>>>> really does need to create multiple clients.
>>>>>
>>>>> For NFSv4.0, when nfsd gets a repeat SET_CLIENT_ID, it keeps the old one
>>>>> and discards the new.
>>>>> For NFSv4.1, the spec requires that it keep the new one and discard the
>>>>> old.
>>>>> This explains the different ordering.
>>>>
>>>> Hm, is this the client's trunking-detection logic?
>>>
>>> Yes.
>>>
>>>>
>>>> In which case, it's not just unconfirmed clients.
>>>
>>> For NFSv4.1, only the EXCHANGE_ID is duplicate.  There is only one
>>> CREATE_SESSION, and that is where the client is confirmed.  So only one
>>> confirmed client.
>>>
>>> For NFSv4.0 bother SETCLIENTID and SETCLIENDID_CONFIRM are duplicate.
>>> So maybe both clients get confirmed.  I should check that.
>>
>> Drifting off topic, but I don't see how this client behavior makes
>> sense.  Mount is chatty enough without the unnecessary duplication.
>> Looking at the code....
> 
> I never sorted this out, by the way.  I think there must be a bug,
> though.
My bad... I didn't realize you had concern with the patch... 
What needs to happen... to figure this out?

steved.

Re: [PATCH 0/5 v2] nfs-utils: provide audit-logging of NFSv4 access

[J. Bruce Fields]

On Wed, Apr 07, 2021 at 03:33:41PM -0400, Steve Dickson wrote:
> 
> 
> On 4/7/21 3:14 PM, J. Bruce Fields wrote:
> > On Fri, Mar 19, 2021 at 05:09:22PM -0400, J. Bruce Fields wrote:
> >> On Sat, Mar 20, 2021 at 07:48:44AM +1100, NeilBrown wrote:
> >>> For NFSv4.1, only the EXCHANGE_ID is duplicate.  There is only one
> >>> CREATE_SESSION, and that is where the client is confirmed.  So only one
> >>> confirmed client.
> >>>
> >>> For NFSv4.0 bother SETCLIENTID and SETCLIENDID_CONFIRM are duplicate.
> >>> So maybe both clients get confirmed.  I should check that.
> >>
> >> Drifting off topic, but I don't see how this client behavior makes
> >> sense.  Mount is chatty enough without the unnecessary duplication.
> >> Looking at the code....
> > 
> > I never sorted this out, by the way.  I think there must be a bug,
> > though.
> My bad... I didn't realize you had concern with the patch... 
> What needs to happen... to figure this out?

Sorry, it wasn't a concern with Neil's patches.

The thing I don't understand is why the client is sending these calls
twice.  Anyway, that's more-or-less harmless behavior, and in kernel
code, nothing to do with nfs-utils.

--b.

Re: [PATCH v2] nfsd: report client confirmation status in "info" file

[Chuck Lever III]

Hi Neil-


> On Mar 19, 2021, at 6:38 PM, NeilBrown <neilb@suse.de> wrote:
> 
> 
> mountd can now monitor clients appearing and disappearing in
> /proc/fs/nfsd/clients, and will log these events, in liu of the logging
> of mount/unmount events for NFSv3.
> 
> Currently it cannot distinguish between unconfirmed clients (which might
> be transient and totally uninteresting) and confirmed clients.
> 
> So add a "status: " line which reports either "confirmed" or
> "unconfirmed", and use fsnotify to report that the info file
> has been modified.
> 
> This requires a bit of infrastructure to keep the dentry for the "info"
> file.  There is no need to take a counted reference as the dentry must
> remain around until the client is removed.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs4state.c | 19 +++++++++++++++----
> fs/nfsd/nfsctl.c    | 14 ++++++++------
> fs/nfsd/nfsd.h      |  4 +++-
> fs/nfsd/state.h     |  4 ++++
> 4 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 97447a64bad0..ec1b2dd8fd45 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -43,6 +43,7 @@
> #include <linux/sunrpc/addr.h>
> #include <linux/jhash.h>
> #include <linux/string_helpers.h>
> +#include <linux/fsnotify.h>
> #include "xdr4.h"
> #include "xdr4cb.h"
> #include "vfs.h"
> @@ -2352,6 +2353,10 @@ static int client_info_show(struct seq_file *m, void *v)
> 	memcpy(&clid, &clp->cl_clientid, sizeof(clid));
> 	seq_printf(m, "clientid: 0x%llx\n", clid);
> 	seq_printf(m, "address: \"%pISpc\"\n", (struct sockaddr *)&clp->cl_addr);
> +	if (test_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags))
> +		seq_puts(m, "status: confirmed\n");
> +	else
> +		seq_puts(m, "status: unconfirmed\n");
> 	seq_printf(m, "name: ");
> 	seq_quote_mem(m, clp->cl_name.data, clp->cl_name.len);
> 	seq_printf(m, "\nminor version: %d\n", clp->cl_minorversion);
> @@ -2702,6 +2707,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name,
> 	int ret;
> 	struct net *net = SVC_NET(rqstp);
> 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +	struct dentry *dentries[ARRAY_SIZE(client_files)];
> 
> 	clp = alloc_client(name);
> 	if (clp == NULL)
> @@ -2721,9 +2727,11 @@ static struct nfs4_client *create_client(struct xdr_netobj name,
> 	memcpy(&clp->cl_addr, sa, sizeof(struct sockaddr_storage));
> 	clp->cl_cb_session = NULL;
> 	clp->net = net;
> -	clp->cl_nfsd_dentry = nfsd_client_mkdir(nn, &clp->cl_nfsdfs,
> -			clp->cl_clientid.cl_id - nn->clientid_base,
> -			client_files);
> +	clp->cl_nfsd_dentry = nfsd_client_mkdir(
> +		nn, &clp->cl_nfsdfs,
> +		clp->cl_clientid.cl_id - nn->clientid_base,
> +		client_files, dentries);
> +	clp->cl_nfsd_info_dentry = dentries[0];
> 	if (!clp->cl_nfsd_dentry) {
> 		free_client(clp);
> 		return NULL;
> @@ -2798,7 +2806,10 @@ move_to_confirmed(struct nfs4_client *clp)
> 	list_move(&clp->cl_idhash, &nn->conf_id_hashtbl[idhashval]);
> 	rb_erase(&clp->cl_namenode, &nn->unconf_name_tree);
> 	add_clp_to_name_tree(clp, &nn->conf_name_tree);
> -	set_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags);
> +	if (!test_and_set_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags) &&
> +	    clp->cl_nfsd_dentry &&
> +	    clp->cl_nfsd_info_dentry)
> +		fsnotify_dentry(clp->cl_nfsd_info_dentry, FS_MODIFY);

I hit a "sleep while spin-locked" splat and bisected it to this
commit. fsnotify() can allocate memory with GFP_KERNEL, so it
can't be called while &nn->client_lock is held, unfortunately.


> 	renew_client_locked(clp);
> }
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index ef86ed23af82..94ce1eabd9d1 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1266,7 +1266,8 @@ static void nfsdfs_remove_files(struct dentry *root)
> /* XXX: cut'n'paste from simple_fill_super; figure out if we could share
>  * code instead. */
> static  int nfsdfs_create_files(struct dentry *root,
> -					const struct tree_descr *files)
> +				const struct tree_descr *files,
> +				struct dentry **fdentries)
> {
> 	struct inode *dir = d_inode(root);
> 	struct inode *inode;
> @@ -1275,8 +1276,6 @@ static  int nfsdfs_create_files(struct dentry *root,
> 
> 	inode_lock(dir);
> 	for (i = 0; files->name && files->name[0]; i++, files++) {
> -		if (!files->name)
> -			continue;
> 		dentry = d_alloc_name(root, files->name);
> 		if (!dentry)
> 			goto out;
> @@ -1290,6 +1289,8 @@ static  int nfsdfs_create_files(struct dentry *root,
> 		inode->i_private = __get_nfsdfs_client(dir);
> 		d_add(dentry, inode);
> 		fsnotify_create(dir, dentry);
> +		if (fdentries)
> +			fdentries[i] = dentry;
> 	}
> 	inode_unlock(dir);
> 	return 0;
> @@ -1301,8 +1302,9 @@ static  int nfsdfs_create_files(struct dentry *root,
> 
> /* on success, returns positive number unique to that client. */
> struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
> -		struct nfsdfs_client *ncl, u32 id,
> -		const struct tree_descr *files)
> +				 struct nfsdfs_client *ncl, u32 id,
> +				 const struct tree_descr *files,
> +				 struct dentry **fdentries)
> {
> 	struct dentry *dentry;
> 	char name[11];
> @@ -1313,7 +1315,7 @@ struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
> 	dentry = nfsd_mkdir(nn->nfsd_client_dir, ncl, name);
> 	if (IS_ERR(dentry)) /* XXX: tossing errors? */
> 		return NULL;
> -	ret = nfsdfs_create_files(dentry, files);
> +	ret = nfsdfs_create_files(dentry, files, fdentries);
> 	if (ret) {
> 		nfsd_client_rmdir(dentry);
> 		return NULL;
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 8bdc37aa2c2e..9aee72f65330 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -107,7 +107,9 @@ struct nfsdfs_client {
> 
> struct nfsdfs_client *get_nfsdfs_client(struct inode *);
> struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
> -		struct nfsdfs_client *ncl, u32 id, const struct tree_descr *);
> +				 struct nfsdfs_client *ncl, u32 id,
> +				 const struct tree_descr *,
> +				 struct dentry **fdentries);
> void nfsd_client_rmdir(struct dentry *dentry);
> 
> 
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 73deea353169..54cab651ac1d 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -371,6 +371,10 @@ struct nfs4_client {
> 
> 	/* debugging info directory under nfsd/clients/ : */
> 	struct dentry		*cl_nfsd_dentry;
> +	/* 'info' file within that directory. Ref is not counted,
> +	 * but will remain valid iff cl_nfsd_dentry != NULL
> +	 */
> +	struct dentry		*cl_nfsd_info_dentry;
> 
> 	/* for nfs41 callbacks */
> 	/* We currently support a single back channel with a single slot */
> -- 
> 2.30.1
> 

--
Chuck Lever

Re: [PATCH v2] nfsd: report client confirmation status in "info" file

[Chuck Lever III]

> On May 18, 2022, at 10:45 AM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
> Hi Neil-
> 
> 
>> On Mar 19, 2021, at 6:38 PM, NeilBrown <neilb@suse.de> wrote:
>> 
>> 
>> mountd can now monitor clients appearing and disappearing in
>> /proc/fs/nfsd/clients, and will log these events, in liu of the logging
>> of mount/unmount events for NFSv3.
>> 
>> Currently it cannot distinguish between unconfirmed clients (which might
>> be transient and totally uninteresting) and confirmed clients.
>> 
>> So add a "status: " line which reports either "confirmed" or
>> "unconfirmed", and use fsnotify to report that the info file
>> has been modified.
>> 
>> This requires a bit of infrastructure to keep the dentry for the "info"
>> file.  There is no need to take a counted reference as the dentry must
>> remain around until the client is removed.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.de>
>> ---
>> fs/nfsd/nfs4state.c | 19 +++++++++++++++----
>> fs/nfsd/nfsctl.c    | 14 ++++++++------
>> fs/nfsd/nfsd.h      |  4 +++-
>> fs/nfsd/state.h     |  4 ++++
>> 4 files changed, 30 insertions(+), 11 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 97447a64bad0..ec1b2dd8fd45 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -43,6 +43,7 @@
>> #include <linux/sunrpc/addr.h>
>> #include <linux/jhash.h>
>> #include <linux/string_helpers.h>
>> +#include <linux/fsnotify.h>
>> #include "xdr4.h"
>> #include "xdr4cb.h"
>> #include "vfs.h"
>> @@ -2352,6 +2353,10 @@ static int client_info_show(struct seq_file *m, void *v)
>> 	memcpy(&clid, &clp->cl_clientid, sizeof(clid));
>> 	seq_printf(m, "clientid: 0x%llx\n", clid);
>> 	seq_printf(m, "address: \"%pISpc\"\n", (struct sockaddr *)&clp->cl_addr);
>> +	if (test_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags))
>> +		seq_puts(m, "status: confirmed\n");
>> +	else
>> +		seq_puts(m, "status: unconfirmed\n");
>> 	seq_printf(m, "name: ");
>> 	seq_quote_mem(m, clp->cl_name.data, clp->cl_name.len);
>> 	seq_printf(m, "\nminor version: %d\n", clp->cl_minorversion);
>> @@ -2702,6 +2707,7 @@ static struct nfs4_client *create_client(struct xdr_netobj name,
>> 	int ret;
>> 	struct net *net = SVC_NET(rqstp);
>> 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>> +	struct dentry *dentries[ARRAY_SIZE(client_files)];
>> 
>> 	clp = alloc_client(name);
>> 	if (clp == NULL)
>> @@ -2721,9 +2727,11 @@ static struct nfs4_client *create_client(struct xdr_netobj name,
>> 	memcpy(&clp->cl_addr, sa, sizeof(struct sockaddr_storage));
>> 	clp->cl_cb_session = NULL;
>> 	clp->net = net;
>> -	clp->cl_nfsd_dentry = nfsd_client_mkdir(nn, &clp->cl_nfsdfs,
>> -			clp->cl_clientid.cl_id - nn->clientid_base,
>> -			client_files);
>> +	clp->cl_nfsd_dentry = nfsd_client_mkdir(
>> +		nn, &clp->cl_nfsdfs,
>> +		clp->cl_clientid.cl_id - nn->clientid_base,
>> +		client_files, dentries);
>> +	clp->cl_nfsd_info_dentry = dentries[0];
>> 	if (!clp->cl_nfsd_dentry) {
>> 		free_client(clp);
>> 		return NULL;
>> @@ -2798,7 +2806,10 @@ move_to_confirmed(struct nfs4_client *clp)
>> 	list_move(&clp->cl_idhash, &nn->conf_id_hashtbl[idhashval]);
>> 	rb_erase(&clp->cl_namenode, &nn->unconf_name_tree);
>> 	add_clp_to_name_tree(clp, &nn->conf_name_tree);
>> -	set_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags);
>> +	if (!test_and_set_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags) &&
>> +	    clp->cl_nfsd_dentry &&
>> +	    clp->cl_nfsd_info_dentry)
>> +		fsnotify_dentry(clp->cl_nfsd_info_dentry, FS_MODIFY);
> 
> I hit a "sleep while spin-locked" splat and bisected it to this
> commit. fsnotify() can allocate memory with GFP_KERNEL, so it
> can't be called while &nn->client_lock is held, unfortunately.

Never mind. This was fixed by

  934bd07fae7e ("nfsd: move fsnotify on client creation outside spinlock")


>> 	renew_client_locked(clp);
>> }
>> 
>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>> index ef86ed23af82..94ce1eabd9d1 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -1266,7 +1266,8 @@ static void nfsdfs_remove_files(struct dentry *root)
>> /* XXX: cut'n'paste from simple_fill_super; figure out if we could share
>> * code instead. */
>> static  int nfsdfs_create_files(struct dentry *root,
>> -					const struct tree_descr *files)
>> +				const struct tree_descr *files,
>> +				struct dentry **fdentries)
>> {
>> 	struct inode *dir = d_inode(root);
>> 	struct inode *inode;
>> @@ -1275,8 +1276,6 @@ static  int nfsdfs_create_files(struct dentry *root,
>> 
>> 	inode_lock(dir);
>> 	for (i = 0; files->name && files->name[0]; i++, files++) {
>> -		if (!files->name)
>> -			continue;
>> 		dentry = d_alloc_name(root, files->name);
>> 		if (!dentry)
>> 			goto out;
>> @@ -1290,6 +1289,8 @@ static  int nfsdfs_create_files(struct dentry *root,
>> 		inode->i_private = __get_nfsdfs_client(dir);
>> 		d_add(dentry, inode);
>> 		fsnotify_create(dir, dentry);
>> +		if (fdentries)
>> +			fdentries[i] = dentry;
>> 	}
>> 	inode_unlock(dir);
>> 	return 0;
>> @@ -1301,8 +1302,9 @@ static  int nfsdfs_create_files(struct dentry *root,
>> 
>> /* on success, returns positive number unique to that client. */
>> struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
>> -		struct nfsdfs_client *ncl, u32 id,
>> -		const struct tree_descr *files)
>> +				 struct nfsdfs_client *ncl, u32 id,
>> +				 const struct tree_descr *files,
>> +				 struct dentry **fdentries)
>> {
>> 	struct dentry *dentry;
>> 	char name[11];
>> @@ -1313,7 +1315,7 @@ struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
>> 	dentry = nfsd_mkdir(nn->nfsd_client_dir, ncl, name);
>> 	if (IS_ERR(dentry)) /* XXX: tossing errors? */
>> 		return NULL;
>> -	ret = nfsdfs_create_files(dentry, files);
>> +	ret = nfsdfs_create_files(dentry, files, fdentries);
>> 	if (ret) {
>> 		nfsd_client_rmdir(dentry);
>> 		return NULL;
>> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
>> index 8bdc37aa2c2e..9aee72f65330 100644
>> --- a/fs/nfsd/nfsd.h
>> +++ b/fs/nfsd/nfsd.h
>> @@ -107,7 +107,9 @@ struct nfsdfs_client {
>> 
>> struct nfsdfs_client *get_nfsdfs_client(struct inode *);
>> struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
>> -		struct nfsdfs_client *ncl, u32 id, const struct tree_descr *);
>> +				 struct nfsdfs_client *ncl, u32 id,
>> +				 const struct tree_descr *,
>> +				 struct dentry **fdentries);
>> void nfsd_client_rmdir(struct dentry *dentry);
>> 
>> 
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 73deea353169..54cab651ac1d 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -371,6 +371,10 @@ struct nfs4_client {
>> 
>> 	/* debugging info directory under nfsd/clients/ : */
>> 	struct dentry		*cl_nfsd_dentry;
>> +	/* 'info' file within that directory. Ref is not counted,
>> +	 * but will remain valid iff cl_nfsd_dentry != NULL
>> +	 */
>> +	struct dentry		*cl_nfsd_info_dentry;
>> 
>> 	/* for nfs41 callbacks */
>> 	/* We currently support a single back channel with a single slot */
>> -- 
>> 2.30.1
>> 
> 
> --
> Chuck Lever

--
Chuck Lever