nfsidmap and NFS key timeouts and quotas

nfsidmap and NFS key timeouts and quotas

[Jan Sanislo]

The man page for nfsidmap says that it will set a default timeout
of 600 seconds on keys requested by the NFS client.  But the
keys instantiated are listed a permanent in /proc/keys and it's
easily possible to run out of key quota in an NFS environment with
100s/1000s of uids/gids.

Nfsidmap's call to keyctl_set_timeout fails with a permission
error because the call is made *after* the key is instantiated
and permission to modify the key attributes has been revoked as
a result.  The following patch seems to be more effective in
actually setting the key timeout:

====================================

--- nfsidmap.c_orig	2012-10-19 11:32:29.806374240 -0700
+++ nfsidmap.c	2012-10-19 11:40:06.334674363 -0700
@@ -320,6 +320,16 @@
 			key, type, value, timeout);
 	}
 
+	/*
+	 * Set timeout before instantiation revokes our rights
+	 * over the key.
+	 */
+	if ( timeout > 0 ) {
+		rc = keyctl_set_timeout(key, timeout);
+		if ( rc != 0 )
+			xlog_warn("keyctl_set_timeout key 0x%x failed: %m",key);
+	}
+
 	if (strcmp(type, "uid") == 0)
 		rc = id_lookup(value, key, USER);
 	else if (strcmp(type, "gid") == 0)
@@ -329,10 +339,6 @@
 	else if (strcmp(type, "group") == 0)
 		rc = name_lookup(value, key, GROUP);
 
-	/* Set timeout to 10 (600 seconds) minutes */
-	if (rc == 0)
-		keyctl_set_timeout(key, timeout);
-
 	free(arg);
 	return rc;
 }

====================================

Also, it appears that the check for EDQUOT/ENFILE/ENOMEM after the
keyctl_instantiate call is ineffective.  Those errors seem to be
handled within the kernel a key_alloc time -- if one of them occurs
an upcall to nfsidmap is not made.

Finally, the key LRU discard patch: http://lkml.org/lkml/2012/3/28/144
looks promising for managing key quotas.  But it only seems to be
invoked when a key is linked into a destination keyring. fs/nfs/idmap.c
uses a call to security/keys/request_key which by default provides an
NULL dest_keyring. Might consider changing the request_key call in nfs/idmap.c
to request_key_and_link (although I don't pretend to know all the
implications of making such a change).

Re: nfsidmap and NFS key timeouts and quotas

[Jeff Layton]

On 19 Oct 2012 13:53 PDT
Jan Sanislo <oystr@cs.washington.edu> wrote:

> The man page for nfsidmap says that it will set a default timeout
> of 600 seconds on keys requested by the NFS client.  But the
> keys instantiated are listed a permanent in /proc/keys and it's
> easily possible to run out of key quota in an NFS environment with
> 100s/1000s of uids/gids.
> 
> Nfsidmap's call to keyctl_set_timeout fails with a permission
> error because the call is made *after* the key is instantiated
> and permission to modify the key attributes has been revoked as
> a result.  The following patch seems to be more effective in
> actually setting the key timeout:
> 
> ====================================
> 
> --- nfsidmap.c_orig	2012-10-19 11:32:29.806374240 -0700
> +++ nfsidmap.c	2012-10-19 11:40:06.334674363 -0700
> @@ -320,6 +320,16 @@
>  			key, type, value, timeout);
>  	}
>  
> +	/*
> +	 * Set timeout before instantiation revokes our rights
> +	 * over the key.
> +	 */
> +	if ( timeout > 0 ) {
> +		rc = keyctl_set_timeout(key, timeout);
> +		if ( rc != 0 )
> +			xlog_warn("keyctl_set_timeout key 0x%x failed: %m",key);
> +	}
> +
>  	if (strcmp(type, "uid") == 0)
>  		rc = id_lookup(value, key, USER);
>  	else if (strcmp(type, "gid") == 0)
> @@ -329,10 +339,6 @@
>  	else if (strcmp(type, "group") == 0)
>  		rc = name_lookup(value, key, GROUP);
>  
> -	/* Set timeout to 10 (600 seconds) minutes */
> -	if (rc == 0)
> -		keyctl_set_timeout(key, timeout);
> -
>  	free(arg);
>  	return rc;
>  }
> 

Nice catch. The rpc.idmapd downcall handler does this already in the
kernel:

        ret = nfs_idmap_read_and_verify_message(&im,
                        &idmap->idmap_upcall_data->idmap_msg,
                        cons->key, cons->authkey);
        if (ret >= 0) {
                key_set_timeout(cons->key, nfs_idmap_cache_timeout);
                ret = mlen;
        }

...where nfs_idmap_cache_timeout defaults to 600 and is tunable via
module parameter. Might it be better for consistency's sake to make the
keys API upcall do the same thing?

> ====================================
> 
> Also, it appears that the check for EDQUOT/ENFILE/ENOMEM after the
> keyctl_instantiate call is ineffective.  Those errors seem to be
> handled within the kernel a key_alloc time -- if one of them occurs
> an upcall to nfsidmap is not made.
> 
> Finally, the key LRU discard patch: http://lkml.org/lkml/2012/3/28/144
> looks promising for managing key quotas.  But it only seems to be
> invoked when a key is linked into a destination keyring. fs/nfs/idmap.c
> uses a call to security/keys/request_key which by default provides an
> NULL dest_keyring. Might consider changing the request_key call in nfs/idmap.c
> to request_key_and_link (although I don't pretend to know all the
> implications of making such a change).

Have you tested that? I thought these keys did end up in a keyring
(hence the whole override_creds/revert_creds song-and-dance). I haven't
looked closely at the LRU discard stuff though, so you may be correct
here...

-- 
Jeff Layton <jlayton@redhat.com>

Re: nfsidmap and NFS key timeouts and quotas

[Steve Dickson]

On 19/10/12 16:53, Jan Sanislo wrote:
> The man page for nfsidmap says that it will set a default timeout
> of 600 seconds on keys requested by the NFS client.  But the
> keys instantiated are listed a permanent in /proc/keys and it's
> easily possible to run out of key quota in an NFS environment with
> 100s/1000s of uids/gids.
> 
> Nfsidmap's call to keyctl_set_timeout fails with a permission
> error because the call is made *after* the key is instantiated
> and permission to modify the key attributes has been revoked as
> a result.  The following patch seems to be more effective in
> actually setting the key timeout:
> 
> ====================================
> 
> --- nfsidmap.c_orig	2012-10-19 11:32:29.806374240 -0700
> +++ nfsidmap.c	2012-10-19 11:40:06.334674363 -0700
> @@ -320,6 +320,16 @@
>  			key, type, value, timeout);
>  	}
>  
> +	/*
> +	 * Set timeout before instantiation revokes our rights
> +	 * over the key.
> +	 */
> +	if ( timeout > 0 ) {
> +		rc = keyctl_set_timeout(key, timeout);
> +		if ( rc != 0 )
> +			xlog_warn("keyctl_set_timeout key 0x%x failed: %m",key);
> +	}
> +
>  	if (strcmp(type, "uid") == 0)
>  		rc = id_lookup(value, key, USER);
>  	else if (strcmp(type, "gid") == 0)
> @@ -329,10 +339,6 @@
>  	else if (strcmp(type, "group") == 0)
>  		rc = name_lookup(value, key, GROUP);
>  
> -	/* Set timeout to 10 (600 seconds) minutes */
> -	if (rc == 0)
> -		keyctl_set_timeout(key, timeout);
> -
>  	free(arg);
>  	return rc;
>  }
> 
> ====================================
Is this a proposed patch? If so, could you please add he proper
Signed-off-by lines? The main reason I ask is I'm looking to 
do a nfs-utils release in the near future...

steved.
 
> 
> Also, it appears that the check for EDQUOT/ENFILE/ENOMEM after the
> keyctl_instantiate call is ineffective.  Those errors seem to be
> handled within the kernel a key_alloc time -- if one of them occurs
> an upcall to nfsidmap is not made.
> 
> Finally, the key LRU discard patch: http://lkml.org/lkml/2012/3/28/144
> looks promising for managing key quotas.  But it only seems to be
> invoked when a key is linked into a destination keyring. fs/nfs/idmap.c
> uses a call to security/keys/request_key which by default provides an
> NULL dest_keyring. Might consider changing the request_key call in nfs/idmap.c
> to request_key_and_link (although I don't pretend to know all the
> implications of making such a change).
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: nfsidmap and NFS key timeouts and quotas

[Jan Sanislo]

> Nice catch. The rpc.idmapd downcall handler does this already in the
> kernel:
>
>        ret = nfs_idmap_read_and_verify_message(&im,
>                        &idmap->idmap_upcall_data->idmap_msg,
>                        cons->key, cons->authkey);
>        if (ret >= 0) {
>                key_set_timeout(cons->key, nfs_idmap_cache_timeout);
>                ret = mlen;
>        }
>
> ...where nfs_idmap_cache_timeout defaults to 600 and is tunable via
> module parameter. Might it be better for consistency's sake to make the
> keys API upcall do the same thing?

Yes, that certainly works.  But I wonder whether it's conspicuous
absence on that call path wasn't intentional, with the intent being
to make request-keys/nfsidmap the single stop for setting NFS key
timeouts.  If the key_set_timeout call moves into the kernel then
it would be useful to remove the nfsidmap code and docs regarding
key timeouts -- otherwise things might get a littel confusing.

>> 
>> Finally, the key LRU discard patch: http://lkml.org/lkml/2012/3/28/144
>> looks promising for managing key quotas.  But it only seems to be
>> invoked when a key is linked into a destination keyring. fs/nfs/idmap.c
>> uses a call to security/keys/request_key which by default provides an
>> NULL dest_keyring. Might consider changing the request_key call in nfs/idmap.c
>> to request_key_and_link (although I don't pretend to know all the
>> implications of making such a change).

> Have you tested that? I thought these keys did end up in a keyring
> (hence the whole override_creds/revert_creds song-and-dance). I haven't
> looked closely at the LRU discard stuff though, so you may be correct
> here...

Tested only in the sense that, after cranking down the root key quota to
20 or so, the LRU code does not seem to be working for NFS keys.  I
should probably investigate that further.  I was hoping I would get
a response of "yeah, we know it's not quite right" or "you haven't got
things configured correctly".

Thanks for the help.

Re: nfsidmap and NFS key timeouts and quotas

[Jeff Layton]

On 21 Oct 2012 13:15 PDT
Jan Sanislo <oystr@cs.washington.edu> wrote:

> > Nice catch. The rpc.idmapd downcall handler does this already in the
> > kernel:
> >
> >        ret = nfs_idmap_read_and_verify_message(&im,
> >                        &idmap->idmap_upcall_data->idmap_msg,
> >                        cons->key, cons->authkey);
> >        if (ret >= 0) {
> >                key_set_timeout(cons->key, nfs_idmap_cache_timeout);
> >                ret = mlen;
> >        }
> >
> > ...where nfs_idmap_cache_timeout defaults to 600 and is tunable via
> > module parameter. Might it be better for consistency's sake to make the
> > keys API upcall do the same thing?
> 
> Yes, that certainly works.  But I wonder whether it's conspicuous
> absence on that call path wasn't intentional, with the intent being
> to make request-keys/nfsidmap the single stop for setting NFS key
> timeouts.  If the key_set_timeout call moves into the kernel then
> it would be useful to remove the nfsidmap code and docs regarding
> key timeouts -- otherwise things might get a littel confusing.
> 

Yeah, dumb idea on my part. Leaving the policy decision for cache
timeouts in userland is probably the best thing. I think the only
reason we set this in kernel for the legacy upcall is that the keys get
instantiated there too and the legacy upcall format doesn't contain a
timeout that we can use. In principle that code should go away
eventually...

I think your patch looks fine and you can add my Reviewed-by: if you
like...

> >> 
> >> Finally, the key LRU discard patch: http://lkml.org/lkml/2012/3/28/144
> >> looks promising for managing key quotas.  But it only seems to be
> >> invoked when a key is linked into a destination keyring. fs/nfs/idmap.c
> >> uses a call to security/keys/request_key which by default provides an
> >> NULL dest_keyring. Might consider changing the request_key call in nfs/idmap.c
> >> to request_key_and_link (although I don't pretend to know all the
> >> implications of making such a change).
> 
> > Have you tested that? I thought these keys did end up in a keyring
> > (hence the whole override_creds/revert_creds song-and-dance). I haven't
> > looked closely at the LRU discard stuff though, so you may be correct
> > here...
> 
> Tested only in the sense that, after cranking down the root key quota to
> 20 or so, the LRU code does not seem to be working for NFS keys.  I
> should probably investigate that further.  I was hoping I would get
> a response of "yeah, we know it's not quite right" or "you haven't got
> things configured correctly".
> 

(cc'ing keyrings mailing list)

The keyring handling for this is a bit different than it would normally
be in userspace. Because we want to guard against someone "stuffing"
the keyring with bogus downcall info, we declare a private credential
and keyring in nfs_idmap_init_keyring.

My understanding (maybe incorrect) was that those keyrings are not
subject to quota enforcement and so they might not trigger the LRU
discard code. I may be wrong here though, the quota handling code in
the keys API is a bit difficult to follow...

-- 
Jeff Layton <jlayton@redhat.com>