[PATCH 1/2] NFS: Fix a number of bugs in the idmapper

[PATCH 1/2] NFS: Fix a number of bugs in the idmapper

[David Howells]

Fix a number of bugs in the NFS idmapper code:

 (1) Only registered key types can be passed to the core keys code, so
     register the legacy idmapper key type.

     This is a requirement because the unregister function cleans up keys
     belonging to that key type so that there aren't dangling pointers to the
     module left behind - including the key->type pointer.

 (2) Rename the legacy key type.  You can't have two key types with the same
     name, and (1) would otherwise require that.

 (3) complete_request_key() must be called in the error path of
     nfs_idmap_legacy_upcall().

 (4) There is one idmap struct for each nfs_client struct.  This means that
     idmap->idmap_key_cons is shared without the use of a lock.  This is a
     problem because key_instantiate_and_link() - as called indirectly by
     idmap_pipe_downcall() - releases anyone waiting for the key to be
     instantiated.

     What happens is that idmap_pipe_downcall() running in the rpc.idmapd
     thread, releases the NFS filesystem in whatever thread that is running in
     to continue.  This may then make another idmapper call, overwriting
     idmap_key_cons before idmap_pipe_downcall() gets the chance to call
     complete_request_key().

     I *think* that reading idmap_key_cons only once, before
     key_instantiate_and_link() is called, and then caching the result in a
     variable is sufficient.

Bug (4) is the cause of:

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<          (null)>]           (null)
PGD 0
Oops: 0010 [#1] SMP
CPU 1
Modules linked in: ppdev parport_pc lp parport ip6table_filter ip6_tables ebtable_nat ebtables ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack nfs fscache xt_CHECKSUM auth_rpcgss iptable_mangle nfs_acl bridge stp llc lockd be2iscsi iscsi_boot_sysfs bnx2i cnic uio cxgb4i cxgb4 cxgb3i libcxgbi cxgb3 mdio ib_iser rdma_cm ib_cm iw_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi snd_hda_codec_realtek snd_usb_audio snd_hda_intel snd_hda_codec snd_seq snd_pcm snd_hwdep snd_usbmidi_lib snd_rawmidi snd_timer uvcvideo videobuf2_core videodev media videobuf2_vmalloc snd_seq_device videobuf2_memops e1000e vhost_net iTCO_wdt joydev coretemp snd soundcore macvtap macvlan i2c_i801 snd_page_alloc tun iTCO_vendor_support microcode kvm_intel kvm sunrpc hid_logitech_dj usb_storage i915 drm_kms_helper drm i2c_algo_bit i2c_core video [last unloaded: scsi_wait_scan]
Pid: 1229, comm: rpc.idmapd Not tainted 3.4.2-1.fc16.x86_64 #1 Gateway DX4710-UB801A/G33M05G1
RIP: 0010:[<0000000000000000>]  [<          (null)>]           (null)
RSP: 0018:ffff8801a3645d40  EFLAGS: 00010246
RAX: ffff880077707e30 RBX: ffff880077707f50 RCX: ffff8801a18ccd80
RDX: 0000000000000006 RSI: ffff8801a3645e75 RDI: ffff880077707f50
RBP: ffff8801a3645d88 R08: ffff8801a430f9c0 R09: ffff8801a3645db0
R10: 000000000000000a R11: 0000000000000246 R12: ffff8801a18ccd80
R13: ffff8801a3645e75 R14: ffff8801a430f9c0 R15: 0000000000000006
FS:  00007fb6fb51a700(0000) GS:ffff8801afc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 00000001a49b0000 CR4: 00000000000027e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process rpc.idmapd (pid: 1229, threadinfo ffff8801a3644000, task ffff8801a3bf9710)
Stack:
 ffffffff81260878 ffff8801a3645db0 ffff8801a3645db0 ffff880077707a90
 ffff880077707f50 ffff8801a18ccd80 0000000000000006 ffff8801a3645e75
 ffff8801a430f9c0 ffff8801a3645dd8 ffffffff81260983 ffff8801a3645de8
Call Trace:
 [<ffffffff81260878>] ? __key_instantiate_and_link+0x58/0x100
 [<ffffffff81260983>] key_instantiate_and_link+0x63/0xa0
 [<ffffffffa057062b>] idmap_pipe_downcall+0x1cb/0x1e0 [nfs]
 [<ffffffffa0107f57>] rpc_pipe_write+0x67/0x90 [sunrpc]
 [<ffffffff8117f833>] vfs_write+0xb3/0x180
 [<ffffffff8117fb5a>] sys_write+0x4a/0x90
 [<ffffffff81600329>] system_call_fastpath+0x16/0x1b
Code:  Bad RIP value.
RIP  [<          (null)>]           (null)
 RSP <ffff8801a3645d40>
CR2: 0000000000000000

Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Steve Dickson <steved@redhat.com>
---

 fs/nfs/idmap.c |   26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index 864c51e..1b5058b 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -205,12 +205,18 @@ static int nfs_idmap_init_keyring(void)
 	if (ret < 0)
 		goto failed_put_key;
 
+	ret = register_key_type(&key_type_id_resolver_legacy);
+	if (ret < 0)
+		goto failed_reg_legacy;
+
 	set_bit(KEY_FLAG_ROOT_CAN_CLEAR, &keyring->flags);
 	cred->thread_keyring = keyring;
 	cred->jit_keyring = KEY_REQKEY_DEFL_THREAD_KEYRING;
 	id_resolver_cache = cred;
 	return 0;
 
+failed_reg_legacy:
+	unregister_key_type(&key_type_id_resolver);
 failed_put_key:
 	key_put(keyring);
 failed_put_cred:
@@ -222,6 +228,7 @@ static void nfs_idmap_quit_keyring(void)
 {
 	key_revoke(id_resolver_cache->thread_keyring);
 	unregister_key_type(&key_type_id_resolver);
+	unregister_key_type(&key_type_id_resolver_legacy);
 	put_cred(id_resolver_cache);
 }
 
@@ -385,7 +392,7 @@ static const struct rpc_pipe_ops idmap_upcall_ops = {
 };
 
 static struct key_type key_type_id_resolver_legacy = {
-	.name		= "id_resolver",
+	.name		= "id_legacy",
 	.instantiate	= user_instantiate,
 	.match		= user_match,
 	.revoke		= user_revoke,
@@ -674,6 +681,7 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons,
 	if (ret < 0)
 		goto out2;
 
+	BUG_ON(idmap->idmap_key_cons != NULL);
 	idmap->idmap_key_cons = cons;
 
 	ret = rpc_queue_upcall(idmap->idmap_pipe, msg);
@@ -687,8 +695,7 @@ out2:
 out1:
 	kfree(msg);
 out0:
-	key_revoke(cons->key);
-	key_revoke(cons->authkey);
+	complete_request_key(cons, ret);
 	return ret;
 }
 
@@ -722,11 +729,18 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
 {
 	struct rpc_inode *rpci = RPC_I(filp->f_path.dentry->d_inode);
 	struct idmap *idmap = (struct idmap *)rpci->private;
-	struct key_construction *cons = idmap->idmap_key_cons;
+	struct key_construction *cons;
 	struct idmap_msg im;
 	size_t namelen_in;
 	int ret;
 
+	/* If instantiation is successful, anyone waiting for key construction
+	 * will have been woken up and someone else may now have used
+	 * idmap_key_cons - so after this point we may no longer touch it.
+	 */
+	cons = ACCESS_ONCE(idmap->idmap_key_cons);
+	idmap->idmap_key_cons = NULL;
+
 	if (mlen != sizeof(im)) {
 		ret = -ENOSPC;
 		goto out;
@@ -739,7 +753,7 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
 
 	if (!(im.im_status & IDMAP_STATUS_SUCCESS)) {
 		ret = mlen;
-		complete_request_key(idmap->idmap_key_cons, -ENOKEY);
+		complete_request_key(cons, -ENOKEY);
 		goto out_incomplete;
 	}
 
@@ -756,7 +770,7 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
 	}
 
 out:
-	complete_request_key(idmap->idmap_key_cons, ret);
+	complete_request_key(cons, ret);
 out_incomplete:
 	return ret;
 }

[PATCH 2/2] NFS: Combine the idmapper key types

[David Howells]

The NFS idmapper has two key types (normal and legacy) but should only use one
if it can - otherwise it risks having twice as many keys as it would otherwise
need.

Get rid of the legacy key type and have the normal key type have a
.request_key() op.  The choice of which instantiation method is then made by
the upcaller, in order:

 (1) If there's no auxdata, the normal method is called, invoking
     /sbin/request-key.

 (2) If there's something attached to the idmapper pipe (rpc.idmapd) then use
     that.

 (3) Fall back to (1).

Note that this does change the prioritisation of normal vs legacy if both are
available.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/nfs/idmap.c              |  109 ++++++++++++++++++++-----------------------
 include/linux/key-type.h    |    3 +
 security/keys/request_key.c |    6 +-
 3 files changed, 57 insertions(+), 61 deletions(-)

diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index 1b5058b..ee7968d 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -55,7 +55,6 @@
 /* Default cache timeout is 10 minutes */
 unsigned int nfs_idmap_cache_timeout = 600;
 static const struct cred *id_resolver_cache;
-static struct key_type key_type_id_resolver_legacy;
 
 struct idmap {
 	struct rpc_pipe		*idmap_pipe;
@@ -63,6 +62,8 @@ struct idmap {
 	struct mutex		idmap_mutex;
 };
 
+static int nfs_idmap_upcall(struct key_construction *, const char *, void *);
+
 /**
  * nfs_fattr_init_names - initialise the nfs_fattr owner_name/group_name fields
  * @fattr: fully initialised struct nfs_fattr
@@ -173,6 +174,7 @@ static struct key_type key_type_id_resolver = {
 	.destroy	= user_destroy,
 	.describe	= user_describe,
 	.read		= user_read,
+	.request_key	= nfs_idmap_upcall,
 };
 
 static int nfs_idmap_init_keyring(void)
@@ -205,18 +207,12 @@ static int nfs_idmap_init_keyring(void)
 	if (ret < 0)
 		goto failed_put_key;
 
-	ret = register_key_type(&key_type_id_resolver_legacy);
-	if (ret < 0)
-		goto failed_reg_legacy;
-
 	set_bit(KEY_FLAG_ROOT_CAN_CLEAR, &keyring->flags);
 	cred->thread_keyring = keyring;
 	cred->jit_keyring = KEY_REQKEY_DEFL_THREAD_KEYRING;
 	id_resolver_cache = cred;
 	return 0;
 
-failed_reg_legacy:
-	unregister_key_type(&key_type_id_resolver);
 failed_put_key:
 	key_put(keyring);
 failed_put_cred:
@@ -228,7 +224,6 @@ static void nfs_idmap_quit_keyring(void)
 {
 	key_revoke(id_resolver_cache->thread_keyring);
 	unregister_key_type(&key_type_id_resolver);
-	unregister_key_type(&key_type_id_resolver_legacy);
 	put_cred(id_resolver_cache);
 }
 
@@ -276,10 +271,7 @@ static ssize_t nfs_idmap_request_key(struct key_type *key_type,
 		goto out;
 
 	saved_cred = override_creds(id_resolver_cache);
-	if (idmap)
-		rkey = request_key_with_auxdata(key_type, desc, "", 0, idmap);
-	else
-		rkey = request_key(&key_type_id_resolver, desc, "");
+	rkey = request_key_with_auxdata(key_type, desc, "", 0, idmap);
 	revert_creds(saved_cred);
 
 	kfree(desc);
@@ -318,16 +310,12 @@ static ssize_t nfs_idmap_get_key(const char *name, size_t namelen,
 				 const char *type, void *data,
 				 size_t data_size, struct idmap *idmap)
 {
-	ssize_t ret = nfs_idmap_request_key(&key_type_id_resolver,
-					    name, namelen, type, data,
-					    data_size, NULL);
-	if (ret < 0) {
-		mutex_lock(&idmap->idmap_mutex);
-		ret = nfs_idmap_request_key(&key_type_id_resolver_legacy,
-					    name, namelen, type, data,
-					    data_size, idmap);
-		mutex_unlock(&idmap->idmap_mutex);
-	}
+	ssize_t ret;
+	mutex_lock(&idmap->idmap_mutex);
+	ret = nfs_idmap_request_key(&key_type_id_resolver,
+				    name, namelen, type, data,
+				    data_size, idmap);
+	mutex_unlock(&idmap->idmap_mutex);
 	return ret;
 }
 
@@ -380,7 +368,6 @@ static const match_table_t nfs_idmap_tokens = {
 	{ Opt_find_err, NULL }
 };
 
-static int nfs_idmap_legacy_upcall(struct key_construction *, const char *, void *);
 static ssize_t idmap_pipe_downcall(struct file *, const char __user *,
 				   size_t);
 static void idmap_pipe_destroy_msg(struct rpc_pipe_msg *);
@@ -391,17 +378,6 @@ static const struct rpc_pipe_ops idmap_upcall_ops = {
 	.destroy_msg	= idmap_pipe_destroy_msg,
 };
 
-static struct key_type key_type_id_resolver_legacy = {
-	.name		= "id_legacy",
-	.instantiate	= user_instantiate,
-	.match		= user_match,
-	.revoke		= user_revoke,
-	.destroy	= user_destroy,
-	.describe	= user_describe,
-	.read		= user_read,
-	.request_key	= nfs_idmap_legacy_upcall,
-};
-
 static void __nfs_idmap_unregister(struct rpc_pipe *pipe)
 {
 	if (pipe->dentry)
@@ -658,9 +634,13 @@ out:
 	return ret;
 }
 
-static int nfs_idmap_legacy_upcall(struct key_construction *cons,
-				   const char *op,
-				   void *aux)
+/*
+ * Upcall to userspace to attempt the mapping.  We try the legacy call to
+ * rpc.idmapd first (if it is listening on its pipe) otherwise we fall back to
+ * invoking /sbin/request-key.
+ */
+static int nfs_idmap_upcall(struct key_construction *cons,
+			    const char *op, void *aux)
 {
 	struct rpc_pipe_msg *msg;
 	struct idmap_msg *im;
@@ -668,27 +648,40 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons,
 	struct key *key = cons->key;
 	int ret = -ENOMEM;
 
-	/* msg and im are freed in idmap_pipe_destroy_msg */
-	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
-	if (!msg)
-		goto out0;
-
-	im = kmalloc(sizeof(*im), GFP_KERNEL);
-	if (!im)
-		goto out1;
-
-	ret = nfs_idmap_prepare_message(key->description, im, msg);
-	if (ret < 0)
-		goto out2;
-
-	BUG_ON(idmap->idmap_key_cons != NULL);
-	idmap->idmap_key_cons = cons;
-
-	ret = rpc_queue_upcall(idmap->idmap_pipe, msg);
-	if (ret < 0)
-		goto out2;
+	/* See if it's worth trying to talk to rpc.idmapd */
+	if (idmap->idmap_pipe->nreaders ||
+	    idmap->idmap_pipe->flags & RPC_PIPE_WAIT_FOR_OPEN) {
+		/* msg and im are freed in idmap_pipe_destroy_msg */
+		msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+		if (!msg)
+			goto out0;
+
+		im = kmalloc(sizeof(*im), GFP_KERNEL);
+		if (!im)
+			goto out1;
+
+		ret = nfs_idmap_prepare_message(key->description, im, msg);
+		if (ret < 0)
+			goto out2;
+
+		BUG_ON(idmap->idmap_key_cons != NULL);
+		idmap->idmap_key_cons = cons;
+
+		ret = rpc_queue_upcall(idmap->idmap_pipe, msg);
+		if (ret < 0) {
+			idmap->idmap_key_cons = NULL;
+			if (ret == -EPIPE) {
+				kfree(im);
+				kfree(msg);
+				goto rpc_idmapd_is_not_listening;
+			}
+			goto out2;
+		}
+		return ret;
+	}
 
-	return ret;
+rpc_idmapd_is_not_listening:
+	return call_sbin_request_key(cons, op, NULL);
 
 out2:
 	kfree(im);
@@ -778,7 +771,7 @@ out_incomplete:
 static void
 idmap_pipe_destroy_msg(struct rpc_pipe_msg *msg)
 {
-	/* Free memory allocated in nfs_idmap_legacy_upcall() */
+	/* Free memory allocated in the legacy side of nfs_idmap_upcall() */
 	kfree(msg->data);
 	kfree(msg);
 }
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index f0c651c..c095b90 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -29,6 +29,9 @@ struct key_construction {
 typedef int (*request_key_actor_t)(struct key_construction *key,
 				   const char *op, void *aux);
 
+extern int call_sbin_request_key(struct key_construction *cons,
+				 const char *op, void *aux);
+
 /*
  * kernel managed key type definition
  */
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 275c4f9..bac83d1 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -102,9 +102,8 @@ static int call_usermodehelper_keys(char *path, char **argv, char **envp,
  * Request userspace finish the construction of a key
  * - execute "/sbin/request-key <op> <key> <uid> <gid> <keyring> <keyring> <keyring>"
  */
-static int call_sbin_request_key(struct key_construction *cons,
-				 const char *op,
-				 void *aux)
+int call_sbin_request_key(struct key_construction *cons,
+			  const char *op, void *aux)
 {
 	const struct cred *cred = current_cred();
 	key_serial_t prkey, sskey;
@@ -204,6 +203,7 @@ error_alloc:
 	kleave(" = %d", ret);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(call_sbin_request_key);
 
 /*
  * Call out to userspace for key construction.

Re: [PATCH 2/2] NFS: Combine the idmapper key types

[Myklebust, Trond]

T24gVHVlLCAyMDEyLTA3LTMxIGF0IDE1OjQyICswMTAwLCBEYXZpZCBIb3dlbGxzIHdyb3RlOg0K
PiBUaGUgTkZTIGlkbWFwcGVyIGhhcyB0d28ga2V5IHR5cGVzIChub3JtYWwgYW5kIGxlZ2FjeSkg
YnV0IHNob3VsZCBvbmx5IHVzZSBvbmUNCj4gaWYgaXQgY2FuIC0gb3RoZXJ3aXNlIGl0IHJpc2tz
IGhhdmluZyB0d2ljZSBhcyBtYW55IGtleXMgYXMgaXQgd291bGQgb3RoZXJ3aXNlDQo+IG5lZWQu
DQoNCldoeSBpcyB0aGlzPyBEbyB3ZSBhZGQga2V5cyBpbiB0aGUgY2FzZSB3aGVyZSB0aGUgJ25v
cm1hbCcgdXBjYWxsIGZhaWxzPw0KDQo+IEdldCByaWQgb2YgdGhlIGxlZ2FjeSBrZXkgdHlwZSBh
bmQgaGF2ZSB0aGUgbm9ybWFsIGtleSB0eXBlIGhhdmUgYQ0KPiAucmVxdWVzdF9rZXkoKSBvcC4g
IFRoZSBjaG9pY2Ugb2Ygd2hpY2ggaW5zdGFudGlhdGlvbiBtZXRob2QgaXMgdGhlbiBtYWRlIGJ5
DQo+IHRoZSB1cGNhbGxlciwgaW4gb3JkZXI6DQo+IA0KPiAgKDEpIElmIHRoZXJlJ3Mgbm8gYXV4
ZGF0YSwgdGhlIG5vcm1hbCBtZXRob2QgaXMgY2FsbGVkLCBpbnZva2luZw0KPiAgICAgIC9zYmlu
L3JlcXVlc3Qta2V5Lg0KPiANCj4gICgyKSBJZiB0aGVyZSdzIHNvbWV0aGluZyBhdHRhY2hlZCB0
byB0aGUgaWRtYXBwZXIgcGlwZSAocnBjLmlkbWFwZCkgdGhlbiB1c2UNCj4gICAgICB0aGF0Lg0K
PiANCj4gICgzKSBGYWxsIGJhY2sgdG8gKDEpLg0KPiANCj4gTm90ZSB0aGF0IHRoaXMgZG9lcyBj
aGFuZ2UgdGhlIHByaW9yaXRpc2F0aW9uIG9mIG5vcm1hbCB2cyBsZWdhY3kgaWYgYm90aCBhcmUN
Cj4gYXZhaWxhYmxlLg0KDQpXZSBkb24ndCB3YW50IHRvIGRvIHRoaXMuIFRoZSBsZWdhY3kgY2Fs
bCBpcyAxMDAlIHNlcmlhbGlzZWQsIHNvIHRoaXMNCm1lYW5zIHRoYXQgd2UgZW5kIHVwIGNob29z
aW5nIHRoZSBub24tc2NhbGFibGUgYWx0ZXJuYXRpdmUuDQoNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1
c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVz
dEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=

Re: [PATCH 2/2] NFS: Combine the idmapper key types

[David Howells]

Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:

> > The NFS idmapper has two key types (normal and legacy) but should only use
> > one if it can - otherwise it risks having twice as many keys as it would
> > otherwise need.
> 
> Why is this? Do we add keys in the case where the 'normal' upcall fails?

Yes.  The idmapper code defines a second key type and uses the key_type
struct's .request_key() op to do its own thing (ie. upcall to a running
rpc.idmapd process).  The idmapper code, however, calls request_key*() to make
use of this, and that will create a new key.

If only the legacy idmapper is available, you will end up with two keys added
to the keyring for each operation, one of the 'normal' type and one of the
'legacy' type.

The 'normal' key will be negative, it is true, but it still takes up a slot
until the garbage collector eats it, and can displace an otherwise good key.

Negative keys are added to the keyring for a time set by their timeout to
limit the rate of key lookups.

David

Re: [PATCH 1/2] NFS: Fix a number of bugs in the idmapper

[Myklebust, Trond]

T24gVHVlLCAyMDEyLTA3LTMxIGF0IDE1OjQyICswMTAwLCBEYXZpZCBIb3dlbGxzIHdyb3RlOg0K
PiBGaXggYSBudW1iZXIgb2YgYnVncyBpbiB0aGUgTkZTIGlkbWFwcGVyIGNvZGU6DQo+IA0KPiAg
KDEpIE9ubHkgcmVnaXN0ZXJlZCBrZXkgdHlwZXMgY2FuIGJlIHBhc3NlZCB0byB0aGUgY29yZSBr
ZXlzIGNvZGUsIHNvDQo+ICAgICAgcmVnaXN0ZXIgdGhlIGxlZ2FjeSBpZG1hcHBlciBrZXkgdHlw
ZS4NCj4gDQo+ICAgICAgVGhpcyBpcyBhIHJlcXVpcmVtZW50IGJlY2F1c2UgdGhlIHVucmVnaXN0
ZXIgZnVuY3Rpb24gY2xlYW5zIHVwIGtleXMNCj4gICAgICBiZWxvbmdpbmcgdG8gdGhhdCBrZXkg
dHlwZSBzbyB0aGF0IHRoZXJlIGFyZW4ndCBkYW5nbGluZyBwb2ludGVycyB0byB0aGUNCj4gICAg
ICBtb2R1bGUgbGVmdCBiZWhpbmQgLSBpbmNsdWRpbmcgdGhlIGtleS0+dHlwZSBwb2ludGVyLg0K
PiANCj4gICgyKSBSZW5hbWUgdGhlIGxlZ2FjeSBrZXkgdHlwZS4gIFlvdSBjYW4ndCBoYXZlIHR3
byBrZXkgdHlwZXMgd2l0aCB0aGUgc2FtZQ0KPiAgICAgIG5hbWUsIGFuZCAoMSkgd291bGQgb3Ro
ZXJ3aXNlIHJlcXVpcmUgdGhhdC4NCj4gDQo+ICAoMykgY29tcGxldGVfcmVxdWVzdF9rZXkoKSBt
dXN0IGJlIGNhbGxlZCBpbiB0aGUgZXJyb3IgcGF0aCBvZg0KPiAgICAgIG5mc19pZG1hcF9sZWdh
Y3lfdXBjYWxsKCkuDQo+IA0KPiAgKDQpIFRoZXJlIGlzIG9uZSBpZG1hcCBzdHJ1Y3QgZm9yIGVh
Y2ggbmZzX2NsaWVudCBzdHJ1Y3QuICBUaGlzIG1lYW5zIHRoYXQNCj4gICAgICBpZG1hcC0+aWRt
YXBfa2V5X2NvbnMgaXMgc2hhcmVkIHdpdGhvdXQgdGhlIHVzZSBvZiBhIGxvY2suICBUaGlzIGlz
IGENCj4gICAgICBwcm9ibGVtIGJlY2F1c2Uga2V5X2luc3RhbnRpYXRlX2FuZF9saW5rKCkgLSBh
cyBjYWxsZWQgaW5kaXJlY3RseSBieQ0KPiAgICAgIGlkbWFwX3BpcGVfZG93bmNhbGwoKSAtIHJl
bGVhc2VzIGFueW9uZSB3YWl0aW5nIGZvciB0aGUga2V5IHRvIGJlDQo+ICAgICAgaW5zdGFudGlh
dGVkLg0KPiANCj4gICAgICBXaGF0IGhhcHBlbnMgaXMgdGhhdCBpZG1hcF9waXBlX2Rvd25jYWxs
KCkgcnVubmluZyBpbiB0aGUgcnBjLmlkbWFwZA0KPiAgICAgIHRocmVhZCwgcmVsZWFzZXMgdGhl
IE5GUyBmaWxlc3lzdGVtIGluIHdoYXRldmVyIHRocmVhZCB0aGF0IGlzIHJ1bm5pbmcgaW4NCj4g
ICAgICB0byBjb250aW51ZS4gIFRoaXMgbWF5IHRoZW4gbWFrZSBhbm90aGVyIGlkbWFwcGVyIGNh
bGwsIG92ZXJ3cml0aW5nDQo+ICAgICAgaWRtYXBfa2V5X2NvbnMgYmVmb3JlIGlkbWFwX3BpcGVf
ZG93bmNhbGwoKSBnZXRzIHRoZSBjaGFuY2UgdG8gY2FsbA0KPiAgICAgIGNvbXBsZXRlX3JlcXVl
c3Rfa2V5KCkuDQo+IA0KPiAgICAgIEkgKnRoaW5rKiB0aGF0IHJlYWRpbmcgaWRtYXBfa2V5X2Nv
bnMgb25seSBvbmNlLCBiZWZvcmUNCj4gICAgICBrZXlfaW5zdGFudGlhdGVfYW5kX2xpbmsoKSBp
cyBjYWxsZWQsIGFuZCB0aGVuIGNhY2hpbmcgdGhlIHJlc3VsdCBpbiBhDQo+ICAgICAgdmFyaWFi
bGUgaXMgc3VmZmljaWVudC4NCj4gDQo+IEJ1ZyAoNCkgaXMgdGhlIGNhdXNlIG9mOg0KPiANCj4g
QlVHOiB1bmFibGUgdG8gaGFuZGxlIGtlcm5lbCBOVUxMIHBvaW50ZXIgZGVyZWZlcmVuY2UgYXQg
ICAgICAgICAgIChudWxsKQ0KPiBJUDogWzwgICAgICAgICAgKG51bGwpPl0gICAgICAgICAgIChu
dWxsKQ0KPiBQR0QgMA0KPiBPb3BzOiAwMDEwIFsjMV0gU01QDQo+IENQVSAxDQo+IE1vZHVsZXMg
bGlua2VkIGluOiBwcGRldiBwYXJwb3J0X3BjIGxwIHBhcnBvcnQgaXA2dGFibGVfZmlsdGVyIGlw
Nl90YWJsZXMgZWJ0YWJsZV9uYXQgZWJ0YWJsZXMgaXB0X01BU1FVRVJBREUgaXB0YWJsZV9uYXQg
bmZfbmF0IG5mX2Nvbm50cmFja19pcHY0IG5mX2RlZnJhZ19pcHY0IHh0X3N0YXRlIG5mX2Nvbm50
cmFjayBuZnMgZnNjYWNoZSB4dF9DSEVDS1NVTSBhdXRoX3JwY2dzcyBpcHRhYmxlX21hbmdsZSBu
ZnNfYWNsIGJyaWRnZSBzdHAgbGxjIGxvY2tkIGJlMmlzY3NpIGlzY3NpX2Jvb3Rfc3lzZnMgYm54
MmkgY25pYyB1aW8gY3hnYjRpIGN4Z2I0IGN4Z2IzaSBsaWJjeGdiaSBjeGdiMyBtZGlvIGliX2lz
ZXIgcmRtYV9jbSBpYl9jbSBpd19jbSBpYl9zYSBpYl9tYWQgaWJfY29yZSBpYl9hZGRyIGlzY3Np
X3RjcCBsaWJpc2NzaV90Y3AgbGliaXNjc2kgc2NzaV90cmFuc3BvcnRfaXNjc2kgc25kX2hkYV9j
b2RlY19yZWFsdGVrIHNuZF91c2JfYXVkaW8gc25kX2hkYV9pbnRlbCBzbmRfaGRhX2NvZGVjIHNu
ZF9zZXEgc25kX3BjbSBzbmRfaHdkZXAgc25kX3VzYm1pZGlfbGliIHNuZF9yYXdtaWRpIHNuZF90
aW1lciB1dmN2aWRlbyB2aWRlb2J1ZjJfY29yZSB2aWRlb2RldiBtZWRpYSB2aWRlb2J1ZjJfdm1h
bGxvYyBzbmRfc2VxX2RldmljZSB2aWRlb2J1ZjJfbWVtb3BzIGUxMDAwZSB2aG9zdF9uZXQgaVRD
T193ZHQgam95ZGV2IGNvcmV0ZW1wIHNuZCBzb3VuZGNvcmUgbWFjdnRhcCBtYWN2bGFuIGkyY19p
ODAxIHNuZF9wYWdlX2FsbG9jIHR1biBpVENPX3ZlbmRvcl9zdXBwb3J0IG1pY3JvY29kZSBrdm1f
aW50ZWwga3ZtIHN1bnJwYyBoaWRfbG9naXRlY2hfZGogdXNiX3N0b3JhZ2UgaTkxNSBkcm1fa21z
X2hlbHBlciBkcm0gaTJjX2FsZ29fYml0IGkyY19jb3JlIHZpZGVvIFtsYXN0IHVubG9hZGVkOiBz
Y3NpX3dhaXRfc2Nhbl0NCj4gUGlkOiAxMjI5LCBjb21tOiBycGMuaWRtYXBkIE5vdCB0YWludGVk
IDMuNC4yLTEuZmMxNi54ODZfNjQgIzEgR2F0ZXdheSBEWDQ3MTAtVUI4MDFBL0czM00wNUcxDQo+
IFJJUDogMDAxMDpbPDAwMDAwMDAwMDAwMDAwMDA+XSAgWzwgICAgICAgICAgKG51bGwpPl0gICAg
ICAgICAgIChudWxsKQ0KPiBSU1A6IDAwMTg6ZmZmZjg4MDFhMzY0NWQ0MCAgRUZMQUdTOiAwMDAx
MDI0Ng0KPiBSQVg6IGZmZmY4ODAwNzc3MDdlMzAgUkJYOiBmZmZmODgwMDc3NzA3ZjUwIFJDWDog
ZmZmZjg4MDFhMThjY2Q4MA0KPiBSRFg6IDAwMDAwMDAwMDAwMDAwMDYgUlNJOiBmZmZmODgwMWEz
NjQ1ZTc1IFJESTogZmZmZjg4MDA3NzcwN2Y1MA0KPiBSQlA6IGZmZmY4ODAxYTM2NDVkODggUjA4
OiBmZmZmODgwMWE0MzBmOWMwIFIwOTogZmZmZjg4MDFhMzY0NWRiMA0KPiBSMTA6IDAwMDAwMDAw
MDAwMDAwMGEgUjExOiAwMDAwMDAwMDAwMDAwMjQ2IFIxMjogZmZmZjg4MDFhMThjY2Q4MA0KPiBS
MTM6IGZmZmY4ODAxYTM2NDVlNzUgUjE0OiBmZmZmODgwMWE0MzBmOWMwIFIxNTogMDAwMDAwMDAw
MDAwMDAwNg0KPiBGUzogIDAwMDA3ZmI2ZmI1MWE3MDAoMDAwMCkgR1M6ZmZmZjg4MDFhZmM4MDAw
MCgwMDAwKSBrbmxHUzowMDAwMDAwMDAwMDAwMDAwDQo+IENTOiAgMDAxMCBEUzogMDAwMCBFUzog
MDAwMCBDUjA6IDAwMDAwMDAwODAwNTAwMzMNCj4gQ1IyOiAwMDAwMDAwMDAwMDAwMDAwIENSMzog
MDAwMDAwMDFhNDliMDAwMCBDUjQ6IDAwMDAwMDAwMDAwMDI3ZTANCj4gRFIwOiAwMDAwMDAwMDAw
MDAwMDAwIERSMTogMDAwMDAwMDAwMDAwMDAwMCBEUjI6IDAwMDAwMDAwMDAwMDAwMDANCj4gRFIz
OiAwMDAwMDAwMDAwMDAwMDAwIERSNjogMDAwMDAwMDBmZmZmMGZmMCBEUjc6IDAwMDAwMDAwMDAw
MDA0MDANCj4gUHJvY2VzcyBycGMuaWRtYXBkIChwaWQ6IDEyMjksIHRocmVhZGluZm8gZmZmZjg4
MDFhMzY0NDAwMCwgdGFzayBmZmZmODgwMWEzYmY5NzEwKQ0KPiBTdGFjazoNCj4gIGZmZmZmZmZm
ODEyNjA4NzggZmZmZjg4MDFhMzY0NWRiMCBmZmZmODgwMWEzNjQ1ZGIwIGZmZmY4ODAwNzc3MDdh
OTANCj4gIGZmZmY4ODAwNzc3MDdmNTAgZmZmZjg4MDFhMThjY2Q4MCAwMDAwMDAwMDAwMDAwMDA2
IGZmZmY4ODAxYTM2NDVlNzUNCj4gIGZmZmY4ODAxYTQzMGY5YzAgZmZmZjg4MDFhMzY0NWRkOCBm
ZmZmZmZmZjgxMjYwOTgzIGZmZmY4ODAxYTM2NDVkZTgNCj4gQ2FsbCBUcmFjZToNCj4gIFs8ZmZm
ZmZmZmY4MTI2MDg3OD5dID8gX19rZXlfaW5zdGFudGlhdGVfYW5kX2xpbmsrMHg1OC8weDEwMA0K
PiAgWzxmZmZmZmZmZjgxMjYwOTgzPl0ga2V5X2luc3RhbnRpYXRlX2FuZF9saW5rKzB4NjMvMHhh
MA0KPiAgWzxmZmZmZmZmZmEwNTcwNjJiPl0gaWRtYXBfcGlwZV9kb3duY2FsbCsweDFjYi8weDFl
MCBbbmZzXQ0KPiAgWzxmZmZmZmZmZmEwMTA3ZjU3Pl0gcnBjX3BpcGVfd3JpdGUrMHg2Ny8weDkw
IFtzdW5ycGNdDQo+ICBbPGZmZmZmZmZmODExN2Y4MzM+XSB2ZnNfd3JpdGUrMHhiMy8weDE4MA0K
PiAgWzxmZmZmZmZmZjgxMTdmYjVhPl0gc3lzX3dyaXRlKzB4NGEvMHg5MA0KPiAgWzxmZmZmZmZm
ZjgxNjAwMzI5Pl0gc3lzdGVtX2NhbGxfZmFzdHBhdGgrMHgxNi8weDFiDQo+IENvZGU6ICBCYWQg
UklQIHZhbHVlLg0KPiBSSVAgIFs8ICAgICAgICAgIChudWxsKT5dICAgICAgICAgICAobnVsbCkN
Cj4gIFJTUCA8ZmZmZjg4MDFhMzY0NWQ0MD4NCj4gQ1IyOiAwMDAwMDAwMDAwMDAwMDAwDQo+IA0K
PiBTaWduZWQtb2ZmLWJ5OiBEYXZpZCBIb3dlbGxzIDxkaG93ZWxsc0ByZWRoYXQuY29tPg0KPiBS
ZXZpZXdlZC1ieTogU3RldmUgRGlja3NvbiA8c3RldmVkQHJlZGhhdC5jb20+DQo+IC0tLQ0KDQpU
aGlzIHBhdGNoIHNob3VsZCBhbHJlYWR5IGJlIHByZXNlbnQgaW4gbmZzLWZvci1uZXh0IGFzIG9m
IHllc3RlcmRheS4NCihJJ3ZlIGJlZW4gb24gYSAyIHdlZWsgdmFjYXRpb24pLg0KDQotLSANClRy
b25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJv
bmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==

Re: [PATCH 1/2] NFS: Fix a number of bugs in the idmapper

[David Howells]

Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:

> This patch should already be present in nfs-for-next as of yesterday.
> (I've been on a 2 week vacation).

How about the second patch?  I posted an updated version of it just now.

Note that the second patch or something like it is likely to be necessary to
reduce the load on the keyring (you end up having duplicate keys of different
key types that can't be shared).

David

Re: [PATCH 1/2] NFS: Fix a number of bugs in the idmapper

[Myklebust, Trond]

T24gVHVlLCAyMDEyLTA3LTMxIGF0IDE1OjU2ICswMTAwLCBEYXZpZCBIb3dlbGxzIHdyb3RlOg0K
PiBNeWtsZWJ1c3QsIFRyb25kIDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4gd3JvdGU6DQo+
IA0KPiA+IFRoaXMgcGF0Y2ggc2hvdWxkIGFscmVhZHkgYmUgcHJlc2VudCBpbiBuZnMtZm9yLW5l
eHQgYXMgb2YgeWVzdGVyZGF5Lg0KPiA+IChJJ3ZlIGJlZW4gb24gYSAyIHdlZWsgdmFjYXRpb24p
Lg0KPiANCj4gSG93IGFib3V0IHRoZSBzZWNvbmQgcGF0Y2g/ICBJIHBvc3RlZCBhbiB1cGRhdGVk
IHZlcnNpb24gb2YgaXQganVzdCBub3cuDQo+IA0KPiBOb3RlIHRoYXQgdGhlIHNlY29uZCBwYXRj
aCBvciBzb21ldGhpbmcgbGlrZSBpdCBpcyBsaWtlbHkgdG8gYmUgbmVjZXNzYXJ5IHRvDQo+IHJl
ZHVjZSB0aGUgbG9hZCBvbiB0aGUga2V5cmluZyAoeW91IGVuZCB1cCBoYXZpbmcgZHVwbGljYXRl
IGtleXMgb2YgZGlmZmVyZW50DQo+IGtleSB0eXBlcyB0aGF0IGNhbid0IGJlIHNoYXJlZCkuDQoN
CldlIGRvbid0IGV4cGVjdCBwZW9wbGUgdG8gYmUgcnVubmluZyBib3RoIHR5cGVzIG9mIGlkbWFw
cGVyIGF0IHRoZSBzYW1lDQp0aW1lLiBOb3JtYWxseSwgdGhlIGRpc3RyaWJ1dGlvbiB3aWxsIHNl
dCB1cCBvbmUgb3IgdGhlIG90aGVyLiBIb3dldmVyDQppZiBib3RoIGlkbWFwcGVycyBkbyBoYXBw
ZW4gdG8gYmUgY29uZmlndXJlZCwgdGhlbiB3ZSB3YW50IHRvIGFsd2F5cw0KY2hvb3NlIHRoZSBu
b24tbGVnYWN5IHR5cGUuLi4NCg0KQ2hlZXJzDQogIFRyb25kDQo=

Re: [PATCH 1/2] NFS: Fix a number of bugs in the idmapper

[J. Bruce Fields]

On Tue, Jul 31, 2012 at 03:07:42PM +0000, Myklebust, Trond wrote:
> On Tue, 2012-07-31 at 15:56 +0100, David Howells wrote:
> > Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:
> > 
> > > This patch should already be present in nfs-for-next as of yesterday.
> > > (I've been on a 2 week vacation).
> > 
> > How about the second patch?  I posted an updated version of it just now.
> > 
> > Note that the second patch or something like it is likely to be necessary to
> > reduce the load on the keyring (you end up having duplicate keys of different
> > key types that can't be shared).
> 
> We don't expect people to be running both types of idmapper at the same
> time. Normally, the distribution will set up one or the other. However
> if both idmappers do happen to be configured, then we want to always
> choose the non-legacy type...

On a quick check....  It looks like F17 has /usr/sbin/nfsidmap, and also
runs rpc.idmapd for the server.  While rpc.idmap does have a "-S"
option, it's not using that.

But that should be easy to fix--I'll go look....

--b.

Re: [PATCH 1/2] NFS: Fix a number of bugs in the idmapper

[J. Bruce Fields]

On Tue, Jul 31, 2012 at 12:37:08PM -0400, bfields wrote:
> On Tue, Jul 31, 2012 at 03:07:42PM +0000, Myklebust, Trond wrote:
> > We don't expect people to be running both types of idmapper at the same
> > time. Normally, the distribution will set up one or the other. However
> > if both idmappers do happen to be configured, then we want to always
> > choose the non-legacy type...
> 
> On a quick check....  It looks like F17 has /usr/sbin/nfsidmap, and also
> runs rpc.idmapd for the server.  While rpc.idmap does have a "-S"
> option, it's not using that.
> 
> But that should be easy to fix--I'll go look....

Steve, this, or do we want to modify the default value of RPCIDMAPDARGS
in nfs.sysconfig?

--b.

diff --git a/nfs-idmap.service b/nfs-idmap.service
index 872ae09..93c1b3a 100644
--- a/nfs-idmap.service
+++ b/nfs-idmap.service
@@ -7,7 +7,7 @@ After=nfs-server.service
 Type=forking
 StandardError=syslog+console
 EnvironmentFile=-/etc/sysconfig/nfs
-ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
+ExecStart=/usr/sbin/rpc.idmapd -S $RPCIDMAPDARGS
 
 [Install]
 WantedBy=nfs.target
diff --git a/nfs.sysconfig b/nfs.sysconfig
index 2d33cf3..4baa0c3 100644
--- a/nfs.sysconfig
+++ b/nfs.sysconfig
@@ -24,7 +24,7 @@ RPCMOUNTDOPTS=""
 STATDARG=""
 #
 # Optional arguments passed to rpc.idmapd. See rpc.idmapd(8)
-RPCIDMAPDARGS=""
+RPCIDMAPDARGS="-S"
 #
 # Optional arguments passed to rpc.gssd. See rpc.gssd(8)
 RPCGSSDARGS=""

Re: [PATCH 1/2] NFS: Fix a number of bugs in the idmapper

[Steve Dickson]

On 07/31/2012 01:11 PM, J. Bruce Fields wrote:
> On Tue, Jul 31, 2012 at 12:37:08PM -0400, bfields wrote:
>> On Tue, Jul 31, 2012 at 03:07:42PM +0000, Myklebust, Trond wrote:
>>> We don't expect people to be running both types of idmapper at the same
>>> time. Normally, the distribution will set up one or the other. However
>>> if both idmappers do happen to be configured, then we want to always
>>> choose the non-legacy type...
>>
>> On a quick check....  It looks like F17 has /usr/sbin/nfsidmap, and also
>> runs rpc.idmapd for the server.  While rpc.idmap does have a "-S"
>> option, it's not using that.
>>
>> But that should be easy to fix--I'll go look....
> 
> Steve, this, or do we want to modify the default value of RPCIDMAPDARGS
> in nfs.sysconfig?
Actually that looks like a good idea.. I'll make that happen... but
it will be a Fedora only thing... 

steved.
> 
> --b.
> 
> diff --git a/nfs-idmap.service b/nfs-idmap.service
> index 872ae09..93c1b3a 100644
> --- a/nfs-idmap.service
> +++ b/nfs-idmap.service
> @@ -7,7 +7,7 @@ After=nfs-server.service
>  Type=forking
>  StandardError=syslog+console
>  EnvironmentFile=-/etc/sysconfig/nfs
> -ExecStart=/usr/sbin/rpc.idmapd $RPCIDMAPDARGS
> +ExecStart=/usr/sbin/rpc.idmapd -S $RPCIDMAPDARGS
>  
>  [Install]
>  WantedBy=nfs.target
> diff --git a/nfs.sysconfig b/nfs.sysconfig
> index 2d33cf3..4baa0c3 100644
> --- a/nfs.sysconfig
> +++ b/nfs.sysconfig
> @@ -24,7 +24,7 @@ RPCMOUNTDOPTS=""
>  STATDARG=""
>  #
>  # Optional arguments passed to rpc.idmapd. See rpc.idmapd(8)
> -RPCIDMAPDARGS=""
> +RPCIDMAPDARGS="-S"
>  #
>  # Optional arguments passed to rpc.gssd. See rpc.gssd(8)
>  RPCGSSDARGS=""
> --
> 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

[PATCH 1/2] NFS: Fix a number of bugs in the idmapper

[David Howells]

Fix a number of bugs in the NFS idmapper code:

 (1) Only registered key types can be passed to the core keys code, so
     register the legacy idmapper key type.

     This is a requirement because the unregister function cleans up keys
     belonging to that key type so that there aren't dangling pointers to the
     module left behind - including the key->type pointer.

 (2) Rename the legacy key type.  You can't have two key types with the same
     name, and (1) would otherwise require that.

 (3) complete_request_key() must be called in the error path of
     nfs_idmap_legacy_upcall().

 (4) There is one idmap struct for each nfs_client struct.  This means that
     idmap->idmap_key_cons is shared without the use of a lock.  This is a
     problem because key_instantiate_and_link() - as called indirectly by
     idmap_pipe_downcall() - releases anyone waiting for the key to be
     instantiated.

     What happens is that idmap_pipe_downcall() running in the rpc.idmapd
     thread, releases the NFS filesystem in whatever thread that is running in
     to continue.  This may then make another idmapper call, overwriting
     idmap_key_cons before idmap_pipe_downcall() gets the chance to call
     complete_request_key().

     I *think* that reading idmap_key_cons only once, before
     key_instantiate_and_link() is called, and then caching the result in a
     variable is sufficient.

Bug (4) is the cause of:

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<          (null)>]           (null)
PGD 0
Oops: 0010 [#1] SMP
CPU 1
Modules linked in: ppdev parport_pc lp parport ip6table_filter ip6_tables ebtable_nat ebtables ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack nfs fscache xt_CHECKSUM auth_rpcgss iptable_mangle nfs_acl bridge stp llc lockd be2iscsi iscsi_boot_sysfs bnx2i cnic uio cxgb4i cxgb4 cxgb3i libcxgbi cxgb3 mdio ib_iser rdma_cm ib_cm iw_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi snd_hda_codec_realtek snd_usb_audio snd_hda_intel snd_hda_codec snd_seq snd_pcm snd_hwdep snd_usbmidi_lib snd_rawmidi snd_timer uvcvideo videobuf2_core videodev media videobuf2_vmalloc snd_seq_device videobuf2_memops e1000e vhost_net iTCO_wdt joydev coretemp snd soundcore macvtap macvlan i2c_i801 snd_page_alloc tun iTCO_vendor_support microcode kvm_intel kvm sunrpc hid_logitech_dj usb_storage i915 drm_kms_helper drm i2c_algo_bit i2c_core video [last unloaded: scsi_wait_scan]
Pid: 1229, comm: rpc.idmapd Not tainted 3.4.2-1.fc16.x86_64 #1 Gateway DX4710-UB801A/G33M05G1
RIP: 0010:[<0000000000000000>]  [<          (null)>]           (null)
RSP: 0018:ffff8801a3645d40  EFLAGS: 00010246
RAX: ffff880077707e30 RBX: ffff880077707f50 RCX: ffff8801a18ccd80
RDX: 0000000000000006 RSI: ffff8801a3645e75 RDI: ffff880077707f50
RBP: ffff8801a3645d88 R08: ffff8801a430f9c0 R09: ffff8801a3645db0
R10: 000000000000000a R11: 0000000000000246 R12: ffff8801a18ccd80
R13: ffff8801a3645e75 R14: ffff8801a430f9c0 R15: 0000000000000006
FS:  00007fb6fb51a700(0000) GS:ffff8801afc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 00000001a49b0000 CR4: 00000000000027e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process rpc.idmapd (pid: 1229, threadinfo ffff8801a3644000, task ffff8801a3bf9710)
Stack:
 ffffffff81260878 ffff8801a3645db0 ffff8801a3645db0 ffff880077707a90
 ffff880077707f50 ffff8801a18ccd80 0000000000000006 ffff8801a3645e75
 ffff8801a430f9c0 ffff8801a3645dd8 ffffffff81260983 ffff8801a3645de8
Call Trace:
 [<ffffffff81260878>] ? __key_instantiate_and_link+0x58/0x100
 [<ffffffff81260983>] key_instantiate_and_link+0x63/0xa0
 [<ffffffffa057062b>] idmap_pipe_downcall+0x1cb/0x1e0 [nfs]
 [<ffffffffa0107f57>] rpc_pipe_write+0x67/0x90 [sunrpc]
 [<ffffffff8117f833>] vfs_write+0xb3/0x180
 [<ffffffff8117fb5a>] sys_write+0x4a/0x90
 [<ffffffff81600329>] system_call_fastpath+0x16/0x1b
Code:  Bad RIP value.
RIP  [<          (null)>]           (null)
 RSP <ffff8801a3645d40>
CR2: 0000000000000000

Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Steve Dickson <steved@redhat.com>
---

 fs/nfs/idmap.c |   26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index 864c51e..1b5058b 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -205,12 +205,18 @@ static int nfs_idmap_init_keyring(void)
 	if (ret < 0)
 		goto failed_put_key;
 
+	ret = register_key_type(&key_type_id_resolver_legacy);
+	if (ret < 0)
+		goto failed_reg_legacy;
+
 	set_bit(KEY_FLAG_ROOT_CAN_CLEAR, &keyring->flags);
 	cred->thread_keyring = keyring;
 	cred->jit_keyring = KEY_REQKEY_DEFL_THREAD_KEYRING;
 	id_resolver_cache = cred;
 	return 0;
 
+failed_reg_legacy:
+	unregister_key_type(&key_type_id_resolver);
 failed_put_key:
 	key_put(keyring);
 failed_put_cred:
@@ -222,6 +228,7 @@ static void nfs_idmap_quit_keyring(void)
 {
 	key_revoke(id_resolver_cache->thread_keyring);
 	unregister_key_type(&key_type_id_resolver);
+	unregister_key_type(&key_type_id_resolver_legacy);
 	put_cred(id_resolver_cache);
 }
 
@@ -385,7 +392,7 @@ static const struct rpc_pipe_ops idmap_upcall_ops = {
 };
 
 static struct key_type key_type_id_resolver_legacy = {
-	.name		= "id_resolver",
+	.name		= "id_legacy",
 	.instantiate	= user_instantiate,
 	.match		= user_match,
 	.revoke		= user_revoke,
@@ -674,6 +681,7 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons,
 	if (ret < 0)
 		goto out2;
 
+	BUG_ON(idmap->idmap_key_cons != NULL);
 	idmap->idmap_key_cons = cons;
 
 	ret = rpc_queue_upcall(idmap->idmap_pipe, msg);
@@ -687,8 +695,7 @@ out2:
 out1:
 	kfree(msg);
 out0:
-	key_revoke(cons->key);
-	key_revoke(cons->authkey);
+	complete_request_key(cons, ret);
 	return ret;
 }
 
@@ -722,11 +729,18 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
 {
 	struct rpc_inode *rpci = RPC_I(filp->f_path.dentry->d_inode);
 	struct idmap *idmap = (struct idmap *)rpci->private;
-	struct key_construction *cons = idmap->idmap_key_cons;
+	struct key_construction *cons;
 	struct idmap_msg im;
 	size_t namelen_in;
 	int ret;
 
+	/* If instantiation is successful, anyone waiting for key construction
+	 * will have been woken up and someone else may now have used
+	 * idmap_key_cons - so after this point we may no longer touch it.
+	 */
+	cons = ACCESS_ONCE(idmap->idmap_key_cons);
+	idmap->idmap_key_cons = NULL;
+
 	if (mlen != sizeof(im)) {
 		ret = -ENOSPC;
 		goto out;
@@ -739,7 +753,7 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
 
 	if (!(im.im_status & IDMAP_STATUS_SUCCESS)) {
 		ret = mlen;
-		complete_request_key(idmap->idmap_key_cons, -ENOKEY);
+		complete_request_key(cons, -ENOKEY);
 		goto out_incomplete;
 	}
 
@@ -756,7 +770,7 @@ idmap_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
 	}
 
 out:
-	complete_request_key(idmap->idmap_key_cons, ret);
+	complete_request_key(cons, ret);
 out_incomplete:
 	return ret;
 }