[1/3] NFS: Clear key construction data if the idmap upcall fails

Commit Message

Bryan Schumaker Aug. 9, 2012, 6:05 p.m. UTC

From: Bryan Schumaker <[e-mail]>

idmap_pipe_downcall already clears this field if the upcall succeeds,
but if it fails (rpc.idmapd isn't running) the field will still be set
on the next call triggering a BUG_ON().  This patch tries to handle all
possible ways that the upcall could fail and clear the idmap key data
for each one.

Signed-off-by: Bryan Schumaker <[e-mail]>
---
 fs/nfs/idmap.c            | 29 ++++++++++++++++++++++++++---
 include/linux/nfs_idmap.h |  1 +
 2 files changed, 27 insertions(+), 3 deletions(-)

Comments

William Dauchy Aug. 10, 2012, 10:09 a.m. UTC | #1

On Thu, Aug 9, 2012 at 8:05 PM,  <[e-mail]> wrote:
> idmap_pipe_downcall already clears this field if the upcall succeeds,
> but if it fails (rpc.idmapd isn't running) the field will still be set
> on the next call triggering a BUG_ON().  This patch tries to handle all
> possible ways that the upcall could fail and clear the idmap key data
> for each one.

strange, I was also getting the BUG_ON even with an idmap running.

I tested this patch on top of a 3.4.8 kernel; before the patch, I was
unable to mount a nfsv4 mount correctly. I think this should also go
in stable.

> Signed-off-by: Bryan Schumaker <[e-mail]>

Tested-by: William Dauchy <[e-mail]>
Cc: [e-mail]

William Dauchy Aug. 16, 2012, 1:45 p.m. UTC | #2

Hi Bryan,

On Fri, Aug 10, 2012 at 12:09 PM, William Dauchy <[e-mail]> wrote:
> strange, I was also getting the BUG_ON even with an idmap running.

On another setup, this patch is breaking the legacy mapping.
rpc.idmapd is running but it can't map any user. I'm trying to see the
difference with the previous test I made.

William Dauchy Aug. 16, 2012, 3:07 p.m. UTC | #3

On Thu, Aug 16, 2012 at 3:45 PM, William Dauchy <[e-mail]> wrote:
> On another setup, this patch is breaking the legacy mapping.
> rpc.idmapd is running but it can't map any user. I'm trying to see the
> difference with the previous test I made.

my second test is on 64 bits (the previous was on 32 bits).
I'm also getting many errors from the userland:
rpc.idmapd: nfscb: write(/var/lib/nfs/rpc_pipefs/nfs/clnt4/idmap): No
space left on device

I assume this is because of adding `im_private` in `struct idmap_msg`
since removing the field resolves the issue.
I tried to find a wrong sizeof regarding `struct idmap_msg` but didn’t
found any thing at the moment.

Bryan Schumaker Aug. 16, 2012, 3:21 p.m. UTC | #4

On 08/16/2012 11:07 AM, William Dauchy wrote:
> On Thu, Aug 16, 2012 at 3:45 PM, William Dauchy <[e-mail]> wrote:
>> On another setup, this patch is breaking the legacy mapping.
>> rpc.idmapd is running but it can't map any user. I'm trying to see the
>> difference with the previous test I made.
> 
> my second test is on 64 bits (the previous was on 32 bits).
> I'm also getting many errors from the userland:
> rpc.idmapd: nfscb: write(/var/lib/nfs/rpc_pipefs/nfs/clnt4/idmap): No
> space left on device
> 
> I assume this is because of adding `im_private` in `struct idmap_msg`
> since removing the field resolves the issue.
> I tried to find a wrong sizeof regarding `struct idmap_msg` but didn’t
> found any thing at the moment.
> 

I was afraid im_private would cause problems, but I wouldn't expect "No space left on device".  You did double check the output of `df`, right? :).

I'll play around with it soon to see what I can find.  Thanks for finding this!

- Bryan

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

William Dauchy Aug. 16, 2012, 3:34 p.m. UTC | #5

On Thu, Aug 16, 2012 at 5:21 PM, Bryan Schumaker <[e-mail]> wrote:
> I was afraid im_private would cause problems, but I wouldn't expect "No space left on device".  You did double check the output of `df`, right? :).

:)
yes.

> I'll play around with it soon to see what I can find.  Thanks for finding this!

Thanks. I'm also testing some stuff to find a possible solution.

Patch

diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index 66d0e85..c2b4004 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -324,6 +324,7 @@  static ssize_t nfs_idmap_get_key(const char *name, size_t namelen,
 		ret = nfs_idmap_request_key(&key_type_id_resolver_legacy,
 					    name, namelen, type, data,
 					    data_size, idmap);
+		idmap->idmap_key_cons = NULL;
 		mutex_unlock(&idmap->idmap_mutex);
 	}
 	return ret;
@@ -380,11 +381,13 @@  static const match_table_t nfs_idmap_tokens = {
 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_release_pipe(struct inode *);
 static void idmap_pipe_destroy_msg(struct rpc_pipe_msg *);
 
 static const struct rpc_pipe_ops idmap_upcall_ops = {
 	.upcall		= rpc_pipe_generic_upcall,
 	.downcall	= idmap_pipe_downcall,
+	.release_pipe	= idmap_release_pipe,
 	.destroy_msg	= idmap_pipe_destroy_msg,
 };
 
@@ -616,7 +619,8 @@  void nfs_idmap_quit(void)
 	nfs_idmap_quit_keyring();
 }
 
-static int nfs_idmap_prepare_message(char *desc, struct idmap_msg *im,
+static int nfs_idmap_prepare_message(char *desc, struct idmap *idmap,
+				     struct idmap_msg *im,
 				     struct rpc_pipe_msg *msg)
 {
 	substring_t substr;
@@ -626,6 +630,7 @@  static int nfs_idmap_prepare_message(char *desc, struct idmap_msg *im,
 	memset(msg, 0, sizeof(*msg));
 
 	im->im_type = IDMAP_TYPE_GROUP;
+	im->im_private = idmap;
 	token = match_token(desc, nfs_idmap_tokens, &substr);
 
 	switch (token) {
@@ -674,7 +679,7 @@  static int nfs_idmap_legacy_upcall(struct key_construction *cons,
 	if (!im)
 		goto out1;
 
-	ret = nfs_idmap_prepare_message(key->description, im, msg);
+	ret = nfs_idmap_prepare_message(key->description, idmap, im, msg);
 	if (ret < 0)
 		goto out2;
 
@@ -683,10 +688,12 @@  static int nfs_idmap_legacy_upcall(struct key_construction *cons,
 
 	ret = rpc_queue_upcall(idmap->idmap_pipe, msg);
 	if (ret < 0)
-		goto out2;
+		goto out3;
 
 	return ret;
 
+out3:
+	idmap->idmap_key_cons = NULL;
 out2:
 	kfree(im);
 out1:
@@ -775,11 +782,27 @@  out_incomplete:
 static void
 idmap_pipe_destroy_msg(struct rpc_pipe_msg *msg)
 {
+	struct idmap_msg *im = msg->data;
+	struct idmap *idmap = (struct idmap *)im->im_private;
+	struct key_construction *cons;
+	if (msg->errno) {
+		cons = ACCESS_ONCE(idmap->idmap_key_cons);
+		idmap->idmap_key_cons = NULL;
+		complete_request_key(cons, msg->errno);
+	}
 	/* Free memory allocated in nfs_idmap_legacy_upcall() */
 	kfree(msg->data);
 	kfree(msg);
 }
 
+static void
+idmap_release_pipe(struct inode *inode)
+{
+	struct rpc_inode *rpci = RPC_I(inode);
+	struct idmap *idmap = (struct idmap *)rpci->private;
+	idmap->idmap_key_cons = NULL;
+}
+
 int nfs_map_name_to_uid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
 {
 	struct idmap *idmap = server->nfs_client->cl_idmap;
diff --git a/include/linux/nfs_idmap.h b/include/linux/nfs_idmap.h
index ece91c5..8a645c7 100644
--- a/include/linux/nfs_idmap.h
+++ b/include/linux/nfs_idmap.h
@@ -59,6 +59,7 @@  struct idmap_msg {
 	char  im_name[IDMAP_NAMESZ];
 	__u32 im_id;
 	__u8  im_status;
+	void *im_private;
 };
 
 #ifdef __KERNEL__