[PATCH v3 0/4] nfsd: add a usermodehelper upcall for client id tracking

[PATCH v3 0/4] nfsd: add a usermodehelper upcall for client id tracking

[Jeff Layton]

This is a respin of the set with the same title that I sent on October
3rd. The main difference is the addition of a printk warning for people
that are using the nfsdcld upcall, and some minor bugfixes and renaming
of functions for consistency.

I'd like to see this go into 3.8 if possible.

Jeff Layton (4):
  nfsd: add a usermodehelper upcall for NFSv4 client ID tracking
  nfsd: change heuristic for selecting the client_tracking_ops
  nfsd: pass info about the legacy recoverydir in environment variables
  nfsd: warn about impending removal of nfsdcld upcall

 fs/nfsd/nfs4recover.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 237 insertions(+), 10 deletions(-)

-- 
1.7.11.7

[PATCH v3 1/4] nfsd: add a usermodehelper upcall for NFSv4 client ID tracking

[Jeff Layton]

Add a new client tracker upcall type that uses call_usermodehelper to
call out to a program. This seems to be the preferred method of
calling out to usermode these days for seldom-called upcalls. It's
simple and doesn't require a running daemon, so it should "just work"
as long as the binary is installed.

The client tracking exit operation is also changed to check for a
NULL pointer before running. The UMH upcall doesn't need to do anything
at module teardown time.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 133 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 43295d4..46e4081 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -926,6 +926,137 @@ static struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops = {
 	.grace_done	= nfsd4_cld_grace_done,
 };
 
+/* upcall via usermodehelper */
+static char cltrack_prog[PATH_MAX] = "/sbin/nfsdcltrack";
+module_param_string(cltrack_prog, cltrack_prog, sizeof(cltrack_prog),
+			S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(cltrack_prog, "Path to the nfsdcltrack upcall program");
+
+static int
+nfsd4_umh_cltrack_upcall(char *cmd, char *arg)
+{
+	char *envp[] = { NULL };
+	char *argv[4];
+	int ret;
+
+	if (unlikely(!cltrack_prog[0])) {
+		dprintk("%s: cltrack_prog is disabled\n", __func__);
+		return -EACCES;
+	}
+
+	dprintk("%s: cmd: %s\n", __func__, cmd);
+	dprintk("%s: arg: %s\n", __func__, arg ? arg : "(null)");
+
+	argv[0] = (char *)cltrack_prog;
+	argv[1] = cmd;
+	argv[2] = arg;
+	argv[3] = NULL;
+
+	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
+	/*
+	 * Disable the upcall mechanism if we're getting an ENOENT or EACCES
+	 * error. The admin can re-enable it on the fly by using sysfs
+	 * once the problem has been fixed.
+	 */
+	if (ret == -ENOENT || ret == -EACCES) {
+		dprintk("NFSD: %s was not found or isn't executable (%d). "
+			"Setting cltrack_prog to blank string!",
+			cltrack_prog, ret);
+		cltrack_prog[0] = '\0';
+	}
+	dprintk("%s: %s return value: %d\n", __func__, cltrack_prog, ret);
+
+	return ret;
+}
+
+static char *
+bin_to_hex_dup(const unsigned char *src, int srclen)
+{
+	int i;
+	char *buf, *hex;
+
+	/* +1 for terminating NULL */
+	buf = kmalloc((srclen * 2) + 1, GFP_KERNEL);
+	if (!buf)
+		return buf;
+
+	hex = buf;
+	for (i = 0; i < srclen; i++) {
+		sprintf(hex, "%2.2x", *src++);
+		hex += 2;
+	}
+	return buf;
+}
+
+static int
+nfsd4_umh_cltrack_init(struct net __attribute__((unused)) *net)
+{
+	return nfsd4_umh_cltrack_upcall("init", NULL);
+}
+
+static void
+nfsd4_umh_cltrack_create(struct nfs4_client *clp)
+{
+	char *hexid;
+
+	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
+	if (!hexid) {
+		dprintk("%s: can't allocate memory for upcall!\n", __func__);
+		return;
+	}
+	nfsd4_umh_cltrack_upcall("create", hexid);
+	kfree(hexid);
+}
+
+static void
+nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
+{
+	char *hexid;
+
+	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
+	if (!hexid) {
+		dprintk("%s: can't allocate memory for upcall!\n", __func__);
+		return;
+	}
+	nfsd4_umh_cltrack_upcall("remove", hexid);
+	kfree(hexid);
+}
+
+static int
+nfsd4_umh_cltrack_check(struct nfs4_client *clp)
+{
+	int ret;
+	char *hexid;
+
+	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
+	if (!hexid) {
+		dprintk("%s: can't allocate memory for upcall!\n", __func__);
+		return -ENOMEM;
+	}
+	ret = nfsd4_umh_cltrack_upcall("check", hexid);
+	kfree(hexid);
+	return ret;
+}
+
+static void
+nfsd4_umh_cltrack_grace_done(struct net __attribute__((unused)) *net,
+				time_t boot_time)
+{
+	char timestr[22]; /* FIXME: better way to determine max size? */
+
+	sprintf(timestr, "%ld", boot_time);
+	nfsd4_umh_cltrack_upcall("gracedone", timestr);
+}
+
+static struct nfsd4_client_tracking_ops nfsd4_umh_tracking_ops = {
+	.init		= nfsd4_umh_cltrack_init,
+	.exit		= NULL,
+	.create		= nfsd4_umh_cltrack_create,
+	.remove		= nfsd4_umh_cltrack_remove,
+	.check		= nfsd4_umh_cltrack_check,
+	.grace_done	= nfsd4_umh_cltrack_grace_done,
+};
+
 int
 nfsd4_client_tracking_init(struct net *net)
 {
@@ -956,7 +1087,8 @@ void
 nfsd4_client_tracking_exit(struct net *net)
 {
 	if (client_tracking_ops) {
-		client_tracking_ops->exit(net);
+		if (client_tracking_ops->exit)
+			client_tracking_ops->exit(net);
 		client_tracking_ops = NULL;
 	}
 }
-- 
1.7.11.7

Re: [PATCH v3 1/4] nfsd: add a usermodehelper upcall for NFSv4 client ID tracking

[J. Bruce Fields]

On Wed, Oct 24, 2012 at 10:30:15AM -0400, Jeff Layton wrote:
> Add a new client tracker upcall type that uses call_usermodehelper to
> call out to a program. This seems to be the preferred method of
> calling out to usermode these days for seldom-called upcalls. It's
> simple and doesn't require a running daemon, so it should "just work"
> as long as the binary is installed.
> 
> The client tracking exit operation is also changed to check for a
> NULL pointer before running. The UMH upcall doesn't need to do anything
> at module teardown time.

That does look relatively simple, neat.

One complaint, a preexisting problem, really: we're passing up the md5
of the client owner name.  We should pass up the original client name,
instead, and make that what we store internally in cl_name.  If nothing
else, it could be useful for debugging.

Those names can be up to 1K, though--is that a problem?

--b.

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfsd/nfs4recover.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 43295d4..46e4081 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -926,6 +926,137 @@ static struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops = {
>  	.grace_done	= nfsd4_cld_grace_done,
>  };
>  
> +/* upcall via usermodehelper */
> +static char cltrack_prog[PATH_MAX] = "/sbin/nfsdcltrack";
> +module_param_string(cltrack_prog, cltrack_prog, sizeof(cltrack_prog),
> +			S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(cltrack_prog, "Path to the nfsdcltrack upcall program");
> +
> +static int
> +nfsd4_umh_cltrack_upcall(char *cmd, char *arg)
> +{
> +	char *envp[] = { NULL };
> +	char *argv[4];
> +	int ret;
> +
> +	if (unlikely(!cltrack_prog[0])) {
> +		dprintk("%s: cltrack_prog is disabled\n", __func__);
> +		return -EACCES;
> +	}
> +
> +	dprintk("%s: cmd: %s\n", __func__, cmd);
> +	dprintk("%s: arg: %s\n", __func__, arg ? arg : "(null)");
> +
> +	argv[0] = (char *)cltrack_prog;
> +	argv[1] = cmd;
> +	argv[2] = arg;
> +	argv[3] = NULL;
> +
> +	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> +	/*
> +	 * Disable the upcall mechanism if we're getting an ENOENT or EACCES
> +	 * error. The admin can re-enable it on the fly by using sysfs
> +	 * once the problem has been fixed.
> +	 */
> +	if (ret == -ENOENT || ret == -EACCES) {
> +		dprintk("NFSD: %s was not found or isn't executable (%d). "
> +			"Setting cltrack_prog to blank string!",
> +			cltrack_prog, ret);
> +		cltrack_prog[0] = '\0';
> +	}
> +	dprintk("%s: %s return value: %d\n", __func__, cltrack_prog, ret);
> +
> +	return ret;
> +}
> +
> +static char *
> +bin_to_hex_dup(const unsigned char *src, int srclen)
> +{
> +	int i;
> +	char *buf, *hex;
> +
> +	/* +1 for terminating NULL */
> +	buf = kmalloc((srclen * 2) + 1, GFP_KERNEL);
> +	if (!buf)
> +		return buf;
> +
> +	hex = buf;
> +	for (i = 0; i < srclen; i++) {
> +		sprintf(hex, "%2.2x", *src++);
> +		hex += 2;
> +	}
> +	return buf;
> +}
> +
> +static int
> +nfsd4_umh_cltrack_init(struct net __attribute__((unused)) *net)
> +{
> +	return nfsd4_umh_cltrack_upcall("init", NULL);
> +}
> +
> +static void
> +nfsd4_umh_cltrack_create(struct nfs4_client *clp)
> +{
> +	char *hexid;
> +
> +	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
> +	if (!hexid) {
> +		dprintk("%s: can't allocate memory for upcall!\n", __func__);
> +		return;
> +	}
> +	nfsd4_umh_cltrack_upcall("create", hexid);
> +	kfree(hexid);
> +}
> +
> +static void
> +nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
> +{
> +	char *hexid;
> +
> +	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
> +	if (!hexid) {
> +		dprintk("%s: can't allocate memory for upcall!\n", __func__);
> +		return;
> +	}
> +	nfsd4_umh_cltrack_upcall("remove", hexid);
> +	kfree(hexid);
> +}
> +
> +static int
> +nfsd4_umh_cltrack_check(struct nfs4_client *clp)
> +{
> +	int ret;
> +	char *hexid;
> +
> +	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
> +	if (!hexid) {
> +		dprintk("%s: can't allocate memory for upcall!\n", __func__);
> +		return -ENOMEM;
> +	}
> +	ret = nfsd4_umh_cltrack_upcall("check", hexid);
> +	kfree(hexid);
> +	return ret;
> +}
> +
> +static void
> +nfsd4_umh_cltrack_grace_done(struct net __attribute__((unused)) *net,
> +				time_t boot_time)
> +{
> +	char timestr[22]; /* FIXME: better way to determine max size? */
> +
> +	sprintf(timestr, "%ld", boot_time);
> +	nfsd4_umh_cltrack_upcall("gracedone", timestr);
> +}
> +
> +static struct nfsd4_client_tracking_ops nfsd4_umh_tracking_ops = {
> +	.init		= nfsd4_umh_cltrack_init,
> +	.exit		= NULL,
> +	.create		= nfsd4_umh_cltrack_create,
> +	.remove		= nfsd4_umh_cltrack_remove,
> +	.check		= nfsd4_umh_cltrack_check,
> +	.grace_done	= nfsd4_umh_cltrack_grace_done,
> +};
> +
>  int
>  nfsd4_client_tracking_init(struct net *net)
>  {
> @@ -956,7 +1087,8 @@ void
>  nfsd4_client_tracking_exit(struct net *net)
>  {
>  	if (client_tracking_ops) {
> -		client_tracking_ops->exit(net);
> +		if (client_tracking_ops->exit)
> +			client_tracking_ops->exit(net);
>  		client_tracking_ops = NULL;
>  	}
>  }
> -- 
> 1.7.11.7
> 

Re: [PATCH v3 1/4] nfsd: add a usermodehelper upcall for NFSv4 client ID tracking

[Jeff Layton]

On Wed, 24 Oct 2012 17:03:10 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Oct 24, 2012 at 10:30:15AM -0400, Jeff Layton wrote:
> > Add a new client tracker upcall type that uses call_usermodehelper to
> > call out to a program. This seems to be the preferred method of
> > calling out to usermode these days for seldom-called upcalls. It's
> > simple and doesn't require a running daemon, so it should "just work"
> > as long as the binary is installed.
> > 
> > The client tracking exit operation is also changed to check for a
> > NULL pointer before running. The UMH upcall doesn't need to do anything
> > at module teardown time.
> 
> That does look relatively simple, neat.
> 
> One complaint, a preexisting problem, really: we're passing up the md5
> of the client owner name.  We should pass up the original client name,
> instead, and make that what we store internally in cl_name.  If nothing
> else, it could be useful for debugging.
> 
> Those names can be up to 1K, though--is that a problem?
> 

We do pass the original client name (hex-encoded) as the second
argument on some of the commands. If you look in the database, here's
what a row of the clients table looks like:

sqlite> select * from clients;
2001:470:8:d63:3a60:77ff:fe93:a95d/2001:470:8:d63:5054:ff:fe9b:3976 tcp|1351162903

The part before the '|' is the client name blob, and the part after is
the epoch timestamp for it.

We also pass the md5-hashed directory pathname as
$NFSDCLTRACK_LEGACY_RECDIR in "check", and the
$NFSDCLTRACK_LEGACY_TOPDIR in "gracedone". My rationale for passing
that info on rather than just having userspace find/calculate it
follows...

> I don't understand why the kernel needs to be involved here--can't
> userspace find the old directory on its own?

My original thinking was to do all of this in userspace and not have
the kernel pass any extra info, but was waved off by several folks who
do userspace development, mostly for reason #3 below.

We could, in principle, do all of the legacy migration in userland.
There are a few reasons to have the kernel involved however:

1) this is not using the keys API, so we have no way to change the
arguments passed to the program without kernel involvement. So, we'd
probably need a kernel parm to disable the legacy transition mode
anyway.

I suppose that we could make the kernel call a wrapper script that in
turn sources a file under /etc/sysconfig or /etc/default that has extra
command-line arguments. That seems like more trouble than it's worth,
but we could consider doing that if you think it's desirable.

2) I think having the kernel pass this info is at least slightly better
for performance. We eliminate the need to
open /proc/fs/nfsd/nfsv4recoverydir at all, and go straight to where we
need to go. We also eliminate the need to re-calculate the md5 hash in
userspace this way.

3) finally, we eliminate the need to pull in a md5 hashing routine into
the userland program. My reasoning for this is somewhat selfish...

The govt. FIPS certification states that you can only use certain
crypto algorithms in certain libraries. MD5 is not on that list.

Even if it were, you'd need to pull in library support to handle it --
you can't just roll your own routine to do crypto without running afoul
of the FIPS "enforcers". We'd have to add a dependency on something
like libnss or openssl (and I know how steved feels about extra
nfs-utils dependencies).

When you boot in "FIPS mode", non-approved crypto code must be disabled
(unrunnable). The kernel already has some code to handle this. I
imagine that NFSv4 serving won't work at all in FIPS mode due to this
fact since SETCLIENTID/EXCHANGE_ID ops will fail (ouch).

Now, we could claim that no one will run this program unless they can
use md5 hashes anyway, or try to get an exception for using md5 here
since it's not really for security purposes, but it's giant PITA either
way. If we have the kernel just pass this info on, then we can sidestep
that issue altogether.

Note that I'm not married to this method of doing the transition mode.
If the consensus is to do it all in userland, I can do that. I think
this way is probably better though.
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 1/4] nfsd: add a usermodehelper upcall for NFSv4 client ID tracking

[J. Bruce Fields]

On Thu, Oct 25, 2012 at 07:39:36AM -0400, Jeff Layton wrote:
> On Wed, 24 Oct 2012 17:03:10 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Wed, Oct 24, 2012 at 10:30:15AM -0400, Jeff Layton wrote:
> > > Add a new client tracker upcall type that uses call_usermodehelper to
> > > call out to a program. This seems to be the preferred method of
> > > calling out to usermode these days for seldom-called upcalls. It's
> > > simple and doesn't require a running daemon, so it should "just work"
> > > as long as the binary is installed.
> > > 
> > > The client tracking exit operation is also changed to check for a
> > > NULL pointer before running. The UMH upcall doesn't need to do anything
> > > at module teardown time.
> > 
> > That does look relatively simple, neat.
> > 
> > One complaint, a preexisting problem, really: we're passing up the md5
> > of the client owner name.  We should pass up the original client name,
> > instead, and make that what we store internally in cl_name.  If nothing
> > else, it could be useful for debugging.
> > 
> > Those names can be up to 1K, though--is that a problem?
> > 
> 
> We do pass the original client name (hex-encoded) as the second
> argument on some of the commands. If you look in the database, here's
> what a row of the clients table looks like:
> 
> sqlite> select * from clients;
> 2001:470:8:d63:3a60:77ff:fe93:a95d/2001:470:8:d63:5054:ff:fe9b:3976 tcp|1351162903
> 
> The part before the '|' is the client name blob, and the part after is
> the epoch timestamp for it.

Bah, sorry, I forgot how the state code worked and confused cl_name and
cl_recdir....

> We also pass the md5-hashed directory pathname as
> $NFSDCLTRACK_LEGACY_RECDIR in "check", and the
> $NFSDCLTRACK_LEGACY_TOPDIR in "gracedone". My rationale for passing
> that info on rather than just having userspace find/calculate it
> follows...

And you may well be right, but I'm still confused:

> > I don't understand why the kernel needs to be involved here--can't
> > userspace find the old directory on its own?
> 
> My original thinking was to do all of this in userspace and not have
> the kernel pass any extra info, but was waved off by several folks who
> do userspace development, mostly for reason #3 below.
> 
> We could, in principle, do all of the legacy migration in userland.
> There are a few reasons to have the kernel involved however:
> 
> 1) this is not using the keys API, so we have no way to change the
> arguments passed to the program without kernel involvement. So, we'd
> probably need a kernel parm to disable the legacy transition mode
> anyway.

There's no need to change the arguments if one can be calculated from
the other.  So this comes down to the crypto police problem?

> I suppose that we could make the kernel call a wrapper script that in
> turn sources a file under /etc/sysconfig or /etc/default that has extra
> command-line arguments. That seems like more trouble than it's worth,
> but we could consider doing that if you think it's desirable.
> 
> 2) I think having the kernel pass this info is at least slightly better
> for performance. We eliminate the need to
> open /proc/fs/nfsd/nfsv4recoverydir at all, and go straight to where we
> need to go. We also eliminate the need to re-calculate the md5 hash in
> userspace this way.

The latter's a one-time expense at transition time, and the former
doesn't sound like a big deal.

> 3) finally, we eliminate the need to pull in a md5 hashing routine into
> the userland program. My reasoning for this is somewhat selfish...
> 
> The govt. FIPS certification states that you can only use certain
> crypto algorithms in certain libraries. MD5 is not on that list.
> 
> Even if it were, you'd need to pull in library support to handle it --
> you can't just roll your own routine to do crypto without running afoul
> of the FIPS "enforcers".

MD5 is lame, good for FIPS for discouraging it, but this seems a little
nuts.

But I guess I can understand: developers have shown a weakness for
reimplementing these things badly, so the security people may feel they
have no choice but to go around harassing anything that looks vaguely
like crypto.  Ugh.

> We'd have to add a dependency on something
> like libnss or openssl (and I know how steved feels about extra
> nfs-utils dependencies).
> 
> When you boot in "FIPS mode", non-approved crypto code must be disabled
> (unrunnable). The kernel already has some code to handle this. I
> imagine that NFSv4 serving won't work at all in FIPS mode due to this
> fact since SETCLIENTID/EXCHANGE_ID ops will fail (ouch).
> 
> Now, we could claim that no one will run this program unless they can
> use md5 hashes anyway, or try to get an exception for using md5 here
> since it's not really for security purposes, but it's giant PITA either
> way. If we have the kernel just pass this info on, then we can sidestep
> that issue altogether.

I'm fine with the transition not working in FIPS mode, as long as NFSv4
doesn't work in FIPS mode anyway.

Ugly stupid idea: the kernel appears to have a userspace api now (using
an AF_ALG socket family?).  Probably more trouble to use than just to
cut-and-paste an md5 implementation, but maybe that works?

--b.

> 
> Note that I'm not married to this method of doing the transition mode.
> If the consensus is to do it all in userland, I can do that. I think
> this way is probably better though.
> -- 
> Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 1/4] nfsd: add a usermodehelper upcall for NFSv4 client ID tracking

[Jeff Layton]

On Thu, 25 Oct 2012 11:06:09 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Oct 25, 2012 at 07:39:36AM -0400, Jeff Layton wrote:
> > On Wed, 24 Oct 2012 17:03:10 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Wed, Oct 24, 2012 at 10:30:15AM -0400, Jeff Layton wrote:
> > > > Add a new client tracker upcall type that uses call_usermodehelper to
> > > > call out to a program. This seems to be the preferred method of
> > > > calling out to usermode these days for seldom-called upcalls. It's
> > > > simple and doesn't require a running daemon, so it should "just work"
> > > > as long as the binary is installed.
> > > > 
> > > > The client tracking exit operation is also changed to check for a
> > > > NULL pointer before running. The UMH upcall doesn't need to do anything
> > > > at module teardown time.
> > > 
> > > That does look relatively simple, neat.
> > > 
> > > One complaint, a preexisting problem, really: we're passing up the md5
> > > of the client owner name.  We should pass up the original client name,
> > > instead, and make that what we store internally in cl_name.  If nothing
> > > else, it could be useful for debugging.
> > > 
> > > Those names can be up to 1K, though--is that a problem?
> > > 
> > 
> > We do pass the original client name (hex-encoded) as the second
> > argument on some of the commands. If you look in the database, here's
> > what a row of the clients table looks like:
> > 
> > sqlite> select * from clients;
> > 2001:470:8:d63:3a60:77ff:fe93:a95d/2001:470:8:d63:5054:ff:fe9b:3976 tcp|1351162903
> > 
> > The part before the '|' is the client name blob, and the part after is
> > the epoch timestamp for it.
> 
> Bah, sorry, I forgot how the state code worked and confused cl_name and
> cl_recdir....
> 

Yes, it's quite confusing ;)

> > We also pass the md5-hashed directory pathname as
> > $NFSDCLTRACK_LEGACY_RECDIR in "check", and the
> > $NFSDCLTRACK_LEGACY_TOPDIR in "gracedone". My rationale for passing
> > that info on rather than just having userspace find/calculate it
> > follows...
> 
> And you may well be right, but I'm still confused:
> 
> > > I don't understand why the kernel needs to be involved here--can't
> > > userspace find the old directory on its own?
> > 
> > My original thinking was to do all of this in userspace and not have
> > the kernel pass any extra info, but was waved off by several folks who
> > do userspace development, mostly for reason #3 below.
> > 
> > We could, in principle, do all of the legacy migration in userland.
> > There are a few reasons to have the kernel involved however:
> > 
> > 1) this is not using the keys API, so we have no way to change the
> > arguments passed to the program without kernel involvement. So, we'd
> > probably need a kernel parm to disable the legacy transition mode
> > anyway.
> 
> There's no need to change the arguments if one can be calculated from
> the other.  So this comes down to the crypto police problem?
> 

Mostly. I think doing it this way is also slightly more efficient. Why
bother with all of the code to recalculate the hash if the kernel has
already done that for us? Passing that info up to userland is simple...

> > I suppose that we could make the kernel call a wrapper script that in
> > turn sources a file under /etc/sysconfig or /etc/default that has extra
> > command-line arguments. That seems like more trouble than it's worth,
> > but we could consider doing that if you think it's desirable.
> > 
> > 2) I think having the kernel pass this info is at least slightly better
> > for performance. We eliminate the need to
> > open /proc/fs/nfsd/nfsv4recoverydir at all, and go straight to where we
> > need to go. We also eliminate the need to re-calculate the md5 hash in
> > userspace this way.
> 
> The latter's a one-time expense at transition time, and the former
> doesn't sound like a big deal.
> 

Well, not one time exactly. We don't have a mechanism to say that the
transition is "done". So, any time we end up not finding the client
string in the new DB, we'll have to recompute the hash and see if the
dir is there.

It is true that none of this is a big deal, but the less code we have
to deal with, the better, IMO...

> > 3) finally, we eliminate the need to pull in a md5 hashing routine into
> > the userland program. My reasoning for this is somewhat selfish...
> > 
> > The govt. FIPS certification states that you can only use certain
> > crypto algorithms in certain libraries. MD5 is not on that list.
> > 
> > Even if it were, you'd need to pull in library support to handle it --
> > you can't just roll your own routine to do crypto without running afoul
> > of the FIPS "enforcers".
> 
> MD5 is lame, good for FIPS for discouraging it, but this seems a little
> nuts.
> 
> But I guess I can understand: developers have shown a weakness for
> reimplementing these things badly, so the security people may feel they
> have no choice but to go around harassing anything that looks vaguely
> like crypto.  Ugh.
> 

Right. That's basically what this comes down to. It's possible that
this would be no big deal, but why bother with it if we don't have to?

> > We'd have to add a dependency on something
> > like libnss or openssl (and I know how steved feels about extra
> > nfs-utils dependencies).
> > 
> > When you boot in "FIPS mode", non-approved crypto code must be disabled
> > (unrunnable). The kernel already has some code to handle this. I
> > imagine that NFSv4 serving won't work at all in FIPS mode due to this
> > fact since SETCLIENTID/EXCHANGE_ID ops will fail (ouch).
> > 
> > Now, we could claim that no one will run this program unless they can
> > use md5 hashes anyway, or try to get an exception for using md5 here
> > since it's not really for security purposes, but it's giant PITA either
> > way. If we have the kernel just pass this info on, then we can sidestep
> > that issue altogether.
> 
> I'm fine with the transition not working in FIPS mode, as long as NFSv4
> doesn't work in FIPS mode anyway.
> 
> Ugly stupid idea: the kernel appears to have a userspace api now (using
> an AF_ALG socket family?).  Probably more trouble to use than just to
> cut-and-paste an md5 implementation, but maybe that works?
> 

I've never used it, but I think libnss might do that under the covers.

What disadvantages do you see in doing it the way I've proposed?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 1/4] nfsd: add a usermodehelper upcall for NFSv4 client ID tracking

[J. Bruce Fields]

On Thu, Oct 25, 2012 at 11:27:06AM -0400, Jeff Layton wrote:
> What disadvantages do you see in doing it the way I've proposed?

Just a vague feeling, along the lines of: APIs can outlast
implementations, and it'll take longer to rip this out if it has to be
removed from both sides....

But it's just an environment variable, easy to ignore, I agree.  I can
live with it.

--b.

Re: [PATCH v3 1/4] nfsd: add a usermodehelper upcall for NFSv4 client ID tracking

[Jeff Layton]

On Thu, 25 Oct 2012 11:49:47 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Oct 25, 2012 at 11:27:06AM -0400, Jeff Layton wrote:
> > What disadvantages do you see in doing it the way I've proposed?
> 
> Just a vague feeling, along the lines of: APIs can outlast
> implementations, and it'll take longer to rip this out if it has to be
> removed from both sides....
> 
> But it's just an environment variable, easy to ignore, I agree.  I can
> live with it.
> 
> --b.

Yep, that was the main reason I used env vars here. Userspace is free
to ignore them, and the kernel is free to not supply them.

-- 
Jeff Layton <jlayton@redhat.com>

[PATCH v3 2/4] nfsd: change heuristic for selecting the client_tracking_ops

[Jeff Layton]

First, try to use the new usermodehelper upcall. It should succeed or
fail quickly, so there's little cost to doing so.

If it fails, and the legacy tracking dir exists, use that. If it
doesn't exist then fall back to using nfsdcld.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 46e4081..aa165fd 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -1063,17 +1063,35 @@ nfsd4_client_tracking_init(struct net *net)
 	int status;
 	struct path path;
 
-	if (!client_tracking_ops) {
-		client_tracking_ops = &nfsd4_cld_tracking_ops;
-		status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, &path);
-		if (!status) {
-			if (S_ISDIR(path.dentry->d_inode->i_mode))
-				client_tracking_ops =
-						&nfsd4_legacy_tracking_ops;
-			path_put(&path);
-		}
+	/* just run the init if it the method is already decided */
+	if (client_tracking_ops)
+		goto do_init;
+
+	/*
+	 * First, try a UMH upcall. It should succeed or fail quickly, so
+	 * there's little harm in trying that first.
+	 */
+	client_tracking_ops = &nfsd4_umh_tracking_ops;
+	status = client_tracking_ops->init(net);
+	if (!status)
+		return status;
+
+	/*
+	 * See if the recoverydir exists and is a directory. If it is,
+	 * then use the legacy ops.
+	 */
+	client_tracking_ops = &nfsd4_legacy_tracking_ops;
+	status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, &path);
+	if (!status) {
+		status = S_ISDIR(path.dentry->d_inode->i_mode);
+		path_put(&path);
+		if (status)
+			goto do_init;
 	}
 
+	/* Finally, try to use nfsdcld */
+	client_tracking_ops = &nfsd4_cld_tracking_ops;
+do_init:
 	status = client_tracking_ops->init(net);
 	if (status) {
 		printk(KERN_WARNING "NFSD: Unable to initialize client "
-- 
1.7.11.7

[PATCH v3 3/4] nfsd: pass info about the legacy recoverydir in environment variables

[Jeff Layton]

The usermodehelper upcall program can then decide to use this info as
a (one-way) transition mechanism to the new scheme. When a "check"
upcall occurs and the client doesn't exist in the database, we can
look to see whether the directory exists. If it does, then we'd add
the client to the database, remove the legacy recdir, and return
success to the kernel to allow the recovery to proceed.

For gracedone, we simply pass the v4recovery "topdir" so that the
upcall can clean it out prior to returning to the kernel.

A module parm is also added to disable the legacy conversion if
the admin chooses.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index aa165fd..782168d 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -932,10 +932,75 @@ module_param_string(cltrack_prog, cltrack_prog, sizeof(cltrack_prog),
 			S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(cltrack_prog, "Path to the nfsdcltrack upcall program");
 
+static bool cltrack_legacy_disable;
+module_param(cltrack_legacy_disable, bool, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(cltrack_legacy_disable,
+		"Disable legacy recoverydir conversion. Default: false");
+
+#define LEGACY_TOPDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_TOPDIR="
+#define LEGACY_RECDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_RECDIR="
+
+static char *
+nfsd4_cltrack_legacy_topdir(void)
+{
+	int copied;
+	size_t len;
+	char *result;
+
+	if (cltrack_legacy_disable)
+		return NULL;
+
+	len = strlen(LEGACY_TOPDIR_ENV_PREFIX) +
+		strlen(nfs4_recoverydir()) + 1;
+
+	result = kmalloc(len, GFP_KERNEL);
+	if (!result)
+		return result;
+
+	copied = snprintf(result, len, LEGACY_TOPDIR_ENV_PREFIX "%s",
+				nfs4_recoverydir());
+	if (copied >= len) {
+		/* just return nothing if output was truncated */
+		kfree(result);
+		return NULL;
+	}
+
+	return result;
+}
+
+static char *
+nfsd4_cltrack_legacy_recdir(const char *recdir)
+{
+	int copied;
+	size_t len;
+	char *result;
+
+	if (cltrack_legacy_disable)
+		return NULL;
+
+	/* +1 is for '/' between "topdir" and "recdir" */
+	len = strlen(LEGACY_RECDIR_ENV_PREFIX) +
+		strlen(nfs4_recoverydir()) + 1 + HEXDIR_LEN;
+
+	result = kmalloc(len, GFP_KERNEL);
+	if (!result)
+		return result;
+
+	copied = snprintf(result, len, LEGACY_RECDIR_ENV_PREFIX "%s/%s",
+				nfs4_recoverydir(), recdir);
+	if (copied >= len) {
+		/* just return nothing if output was truncated */
+		kfree(result);
+		return NULL;
+	}
+
+	return result;
+}
+
 static int
-nfsd4_umh_cltrack_upcall(char *cmd, char *arg)
+nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
 {
-	char *envp[] = { NULL };
+	char *envp[2];
 	char *argv[4];
 	int ret;
 
@@ -946,6 +1011,10 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg)
 
 	dprintk("%s: cmd: %s\n", __func__, cmd);
 	dprintk("%s: arg: %s\n", __func__, arg ? arg : "(null)");
+	dprintk("%s: legacy: %s\n", __func__, legacy ? legacy : "(null)");
+
+	envp[0] = legacy;
+	envp[1] = NULL;
 
 	argv[0] = (char *)cltrack_prog;
 	argv[1] = cmd;
@@ -991,7 +1060,7 @@ bin_to_hex_dup(const unsigned char *src, int srclen)
 static int
 nfsd4_umh_cltrack_init(struct net __attribute__((unused)) *net)
 {
-	return nfsd4_umh_cltrack_upcall("init", NULL);
+	return nfsd4_umh_cltrack_upcall("init", NULL, NULL);
 }
 
 static void
@@ -1004,7 +1073,7 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
 		dprintk("%s: can't allocate memory for upcall!\n", __func__);
 		return;
 	}
-	nfsd4_umh_cltrack_upcall("create", hexid);
+	nfsd4_umh_cltrack_upcall("create", hexid, NULL);
 	kfree(hexid);
 }
 
@@ -1018,7 +1087,7 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
 		dprintk("%s: can't allocate memory for upcall!\n", __func__);
 		return;
 	}
-	nfsd4_umh_cltrack_upcall("remove", hexid);
+	nfsd4_umh_cltrack_upcall("remove", hexid, NULL);
 	kfree(hexid);
 }
 
@@ -1026,14 +1095,16 @@ static int
 nfsd4_umh_cltrack_check(struct nfs4_client *clp)
 {
 	int ret;
-	char *hexid;
+	char *hexid, *legacy;
 
 	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
 	if (!hexid) {
 		dprintk("%s: can't allocate memory for upcall!\n", __func__);
 		return -ENOMEM;
 	}
-	ret = nfsd4_umh_cltrack_upcall("check", hexid);
+	legacy = nfsd4_cltrack_legacy_recdir(clp->cl_recdir);
+	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy);
+	kfree(legacy);
 	kfree(hexid);
 	return ret;
 }
@@ -1042,10 +1113,13 @@ static void
 nfsd4_umh_cltrack_grace_done(struct net __attribute__((unused)) *net,
 				time_t boot_time)
 {
+	char *legacy;
 	char timestr[22]; /* FIXME: better way to determine max size? */
 
 	sprintf(timestr, "%ld", boot_time);
-	nfsd4_umh_cltrack_upcall("gracedone", timestr);
+	legacy = nfsd4_cltrack_legacy_topdir();
+	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy);
+	kfree(legacy);
 }
 
 static struct nfsd4_client_tracking_ops nfsd4_umh_tracking_ops = {
-- 
1.7.11.7

Re: [PATCH v3 3/4] nfsd: pass info about the legacy recoverydir in environment variables

[J. Bruce Fields]

On Wed, Oct 24, 2012 at 10:30:17AM -0400, Jeff Layton wrote:
> The usermodehelper upcall program can then decide to use this info as
> a (one-way) transition mechanism to the new scheme. When a "check"
> upcall occurs and the client doesn't exist in the database, we can
> look to see whether the directory exists. If it does, then we'd add
> the client to the database, remove the legacy recdir, and return
> success to the kernel to allow the recovery to proceed.
> 
> For gracedone, we simply pass the v4recovery "topdir" so that the
> upcall can clean it out prior to returning to the kernel.

I don't understand why the kernel needs to be involved here--can't
userspace find the old directory on its own?

--b.

> 
> A module parm is also added to disable the legacy conversion if
> the admin chooses.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfsd/nfs4recover.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 82 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index aa165fd..782168d 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -932,10 +932,75 @@ module_param_string(cltrack_prog, cltrack_prog, sizeof(cltrack_prog),
>  			S_IRUGO|S_IWUSR);
>  MODULE_PARM_DESC(cltrack_prog, "Path to the nfsdcltrack upcall program");
>  
> +static bool cltrack_legacy_disable;
> +module_param(cltrack_legacy_disable, bool, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(cltrack_legacy_disable,
> +		"Disable legacy recoverydir conversion. Default: false");
> +
> +#define LEGACY_TOPDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_TOPDIR="
> +#define LEGACY_RECDIR_ENV_PREFIX "NFSDCLTRACK_LEGACY_RECDIR="
> +
> +static char *
> +nfsd4_cltrack_legacy_topdir(void)
> +{
> +	int copied;
> +	size_t len;
> +	char *result;
> +
> +	if (cltrack_legacy_disable)
> +		return NULL;
> +
> +	len = strlen(LEGACY_TOPDIR_ENV_PREFIX) +
> +		strlen(nfs4_recoverydir()) + 1;
> +
> +	result = kmalloc(len, GFP_KERNEL);
> +	if (!result)
> +		return result;
> +
> +	copied = snprintf(result, len, LEGACY_TOPDIR_ENV_PREFIX "%s",
> +				nfs4_recoverydir());
> +	if (copied >= len) {
> +		/* just return nothing if output was truncated */
> +		kfree(result);
> +		return NULL;
> +	}
> +
> +	return result;
> +}
> +
> +static char *
> +nfsd4_cltrack_legacy_recdir(const char *recdir)
> +{
> +	int copied;
> +	size_t len;
> +	char *result;
> +
> +	if (cltrack_legacy_disable)
> +		return NULL;
> +
> +	/* +1 is for '/' between "topdir" and "recdir" */
> +	len = strlen(LEGACY_RECDIR_ENV_PREFIX) +
> +		strlen(nfs4_recoverydir()) + 1 + HEXDIR_LEN;
> +
> +	result = kmalloc(len, GFP_KERNEL);
> +	if (!result)
> +		return result;
> +
> +	copied = snprintf(result, len, LEGACY_RECDIR_ENV_PREFIX "%s/%s",
> +				nfs4_recoverydir(), recdir);
> +	if (copied >= len) {
> +		/* just return nothing if output was truncated */
> +		kfree(result);
> +		return NULL;
> +	}
> +
> +	return result;
> +}
> +
>  static int
> -nfsd4_umh_cltrack_upcall(char *cmd, char *arg)
> +nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *legacy)
>  {
> -	char *envp[] = { NULL };
> +	char *envp[2];
>  	char *argv[4];
>  	int ret;
>  
> @@ -946,6 +1011,10 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg)
>  
>  	dprintk("%s: cmd: %s\n", __func__, cmd);
>  	dprintk("%s: arg: %s\n", __func__, arg ? arg : "(null)");
> +	dprintk("%s: legacy: %s\n", __func__, legacy ? legacy : "(null)");
> +
> +	envp[0] = legacy;
> +	envp[1] = NULL;
>  
>  	argv[0] = (char *)cltrack_prog;
>  	argv[1] = cmd;
> @@ -991,7 +1060,7 @@ bin_to_hex_dup(const unsigned char *src, int srclen)
>  static int
>  nfsd4_umh_cltrack_init(struct net __attribute__((unused)) *net)
>  {
> -	return nfsd4_umh_cltrack_upcall("init", NULL);
> +	return nfsd4_umh_cltrack_upcall("init", NULL, NULL);
>  }
>  
>  static void
> @@ -1004,7 +1073,7 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
>  		dprintk("%s: can't allocate memory for upcall!\n", __func__);
>  		return;
>  	}
> -	nfsd4_umh_cltrack_upcall("create", hexid);
> +	nfsd4_umh_cltrack_upcall("create", hexid, NULL);
>  	kfree(hexid);
>  }
>  
> @@ -1018,7 +1087,7 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
>  		dprintk("%s: can't allocate memory for upcall!\n", __func__);
>  		return;
>  	}
> -	nfsd4_umh_cltrack_upcall("remove", hexid);
> +	nfsd4_umh_cltrack_upcall("remove", hexid, NULL);
>  	kfree(hexid);
>  }
>  
> @@ -1026,14 +1095,16 @@ static int
>  nfsd4_umh_cltrack_check(struct nfs4_client *clp)
>  {
>  	int ret;
> -	char *hexid;
> +	char *hexid, *legacy;
>  
>  	hexid = bin_to_hex_dup(clp->cl_name.data, clp->cl_name.len);
>  	if (!hexid) {
>  		dprintk("%s: can't allocate memory for upcall!\n", __func__);
>  		return -ENOMEM;
>  	}
> -	ret = nfsd4_umh_cltrack_upcall("check", hexid);
> +	legacy = nfsd4_cltrack_legacy_recdir(clp->cl_recdir);
> +	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy);
> +	kfree(legacy);
>  	kfree(hexid);
>  	return ret;
>  }
> @@ -1042,10 +1113,13 @@ static void
>  nfsd4_umh_cltrack_grace_done(struct net __attribute__((unused)) *net,
>  				time_t boot_time)
>  {
> +	char *legacy;
>  	char timestr[22]; /* FIXME: better way to determine max size? */
>  
>  	sprintf(timestr, "%ld", boot_time);
> -	nfsd4_umh_cltrack_upcall("gracedone", timestr);
> +	legacy = nfsd4_cltrack_legacy_topdir();
> +	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy);
> +	kfree(legacy);
>  }
>  
>  static struct nfsd4_client_tracking_ops nfsd4_umh_tracking_ops = {
> -- 
> 1.7.11.7
> 

[PATCH v3 4/4] nfsd: warn about impending removal of nfsdcld upcall

[Jeff Layton]

Let's shoot for removing the nfsdcld upcall in 3.10. Most likely,
no one is actually using it so I don't expect this warning to
fire often (except maybe on misconfigured systems).

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 782168d..0a9ac9d 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -1165,6 +1165,9 @@ nfsd4_client_tracking_init(struct net *net)
 
 	/* Finally, try to use nfsdcld */
 	client_tracking_ops = &nfsd4_cld_tracking_ops;
+	printk(KERN_WARNING "NFSD: the nfsdcld client tracking upcall will be "
+			"removed in 3.10. Please transition to using "
+			"nfsdcltrack.\n");
 do_init:
 	status = client_tracking_ops->init(net);
 	if (status) {
-- 
1.7.11.7

Re: [PATCH v3 0/4] nfsd: add a usermodehelper upcall for client id tracking

[J. Bruce Fields]

On Wed, Oct 24, 2012 at 10:30:14AM -0400, Jeff Layton wrote:
> This is a respin of the set with the same title that I sent on October
> 3rd. The main difference is the addition of a printk warning for people
> that are using the nfsdcld upcall, and some minor bugfixes and renaming
> of functions for consistency.
> 
> I'd like to see this go into 3.8 if possible.

Thanks, two questions (see individual replies) aside, this seems fine.

--b.

> 
> Jeff Layton (4):
>   nfsd: add a usermodehelper upcall for NFSv4 client ID tracking
>   nfsd: change heuristic for selecting the client_tracking_ops
>   nfsd: pass info about the legacy recoverydir in environment variables
>   nfsd: warn about impending removal of nfsdcld upcall
> 
>  fs/nfsd/nfs4recover.c | 247 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 237 insertions(+), 10 deletions(-)
> 
> -- 
> 1.7.11.7
>