nfsd: change nfs4_client_to_reclaim() to allocate data

The calling convention for nfs4_client_to_reclaim() is clumsy in that
the caller needs to free memory if the function fails.  It is much
cleaner if the function frees its own memory.

This patch changes nfs4_client_to_reclaim() to re-allocate the .data
fields to be stored in the newly allocated struct nfs4_client_reclaim,
and to free everything on failure.

__cld_pipe_inprogress_downcall() needs to allocate the data anyway to
copy it from user-space, so now that data is allocated twice.  I think
that is a small price to pay for a cleaner interface.

Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index e9d09541161cb370f13605c34c7d0f87ec3aed8c..b1005abcb9035b2cf743200808a251b00af7e3f4 100644 (file)
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -147,24 +147,13 @@ legacy_recdir_name_error(struct nfs4_client *clp, int error)
 
 static void
 __nfsd4_create_reclaim_record_grace(struct nfs4_client *clp,
-               const char *dname, int len, struct nfsd_net *nn)
+                                   char *dname, struct nfsd_net *nn)
 {
-       struct xdr_netobj name;
+       struct xdr_netobj name = { .len = strlen(dname), .data = dname };
        struct xdr_netobj princhash = { .len = 0, .data = NULL };
        struct nfs4_client_reclaim *crp;
 
-       name.data = kmemdup(dname, len, GFP_KERNEL);
-       if (!name.data) {
-               dprintk("%s: failed to allocate memory for name.data!\n",
-                       __func__);
-               return;
-       }
-       name.len = len;
        crp = nfs4_client_to_reclaim(name, princhash, nn);
-       if (!crp) {
-               kfree(name.data);
-               return;
-       }
        crp->cr_clp = clp;
 }
 
@@ -223,8 +212,7 @@ out_unlock:
        inode_unlock(d_inode(dir));
        if (status == 0) {
                if (nn->in_grace)
-                       __nfsd4_create_reclaim_record_grace(clp, dname,
-                                       HEXDIR_LEN, nn);
+                       __nfsd4_create_reclaim_record_grace(clp, dname, nn);
                vfs_fsync(nn->rec_file, 0);
        } else {
                printk(KERN_ERR "NFSD: failed to write recovery record"
@@ -461,7 +449,7 @@ out:
 static int
 load_recdir(struct dentry *parent, char *cname, struct nfsd_net *nn)
 {
-       struct xdr_netobj name;
+       struct xdr_netobj name = { .len = HEXDIR_LEN, .data = cname };
        struct xdr_netobj princhash = { .len = 0, .data = NULL };
 
        if (strlen(cname) != HEXDIR_LEN - 1) {
@@ -470,16 +458,7 @@ load_recdir(struct dentry *parent, char *cname, struct nfsd_net *nn)
                /* Keep trying; maybe the others are OK: */
                return 0;
        }
-       name.data = kstrdup(cname, GFP_KERNEL);
-       if (!name.data) {
-               dprintk("%s: failed to allocate memory for name.data!\n",
-                       __func__);
-               goto out;
-       }
-       name.len = HEXDIR_LEN;
-       if (!nfs4_client_to_reclaim(name, princhash, nn))
-               kfree(name.data);
-out:
+       nfs4_client_to_reclaim(name, princhash, nn);
        return 0;
 }
 
@@ -777,6 +756,8 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
 {
        uint8_t cmd, princhashlen;
        struct xdr_netobj name, princhash = { .len = 0, .data = NULL };
+       char *namecopy __free(kfree) = NULL;
+       char *princhashcopy __free(kfree) = NULL;
        uint16_t namelen;
 
        if (get_user(cmd, &cmsg->cm_cmd)) {
@@ -794,19 +775,19 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
                                dprintk("%s: invalid namelen (%u)", __func__, namelen);
                                return -EINVAL;
                        }
-                       name.data = memdup_user(&ci->cc_name.cn_id, namelen);
-                       if (IS_ERR(name.data))
-                               return PTR_ERR(name.data);
+                       namecopy = memdup_user(&ci->cc_name.cn_id, namelen);
+                       if (IS_ERR(namecopy))
+                               return PTR_ERR(namecopy);
+                       name.data = namecopy;
                        name.len = namelen;
                        get_user(princhashlen, &ci->cc_princhash.cp_len);
                        if (princhashlen > 0) {
-                               princhash.data = memdup_user(
-                                               &ci->cc_princhash.cp_data,
-                                               princhashlen);
-                               if (IS_ERR(princhash.data)) {
-                                       kfree(name.data);
-                                       return PTR_ERR(princhash.data);
-                               }
+                               princhashcopy = memdup_user(
+                                       &ci->cc_princhash.cp_data,
+                                       princhashlen);
+                               if (IS_ERR(princhashcopy))
+                                       return PTR_ERR(princhashcopy);
+                               princhash.data = princhashcopy;
                                princhash.len = princhashlen;
                        } else
                                princhash.len = 0;
@@ -820,9 +801,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
                                dprintk("%s: invalid namelen (%u)", __func__, namelen);
                                return -EINVAL;
                        }
-                       name.data = memdup_user(&cnm->cn_id, namelen);
-                       if (IS_ERR(name.data))
-                               return PTR_ERR(name.data);
+                       namecopy = memdup_user(&cnm->cn_id, namelen);
+                       if (IS_ERR(namecopy))
+                               return PTR_ERR(namecopy);
+                       name.data = namecopy;
                        name.len = namelen;
                }
 #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
@@ -830,15 +812,12 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
                        struct cld_net *cn = nn->cld_net;
 
                        name.len = name.len - 5;
-                       memmove(name.data, name.data + 5, name.len);
+                       name.data = name.data + 5;
                        cn->cn_has_legacy = true;
                }
 #endif
-               if (!nfs4_client_to_reclaim(name, princhash, nn)) {
-                       kfree(name.data);
-                       kfree(princhash.data);
+               if (!nfs4_client_to_reclaim(name, princhash, nn))
                        return -EFAULT;
-               }
                return nn->client_tracking_ops->msglen;
        }
        return -EFAULT;

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8a6960500217412d1aa5d82102ecf6888ff2f27c..af7a20ded1cac166a079bd41cb0413b74c0cbd56 100644 (file)
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8801,9 +8801,6 @@ nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn)
 
 /*
  * failure => all reset bets are off, nfserr_no_grace...
- *
- * The caller is responsible for freeing name.data if NULL is returned (it
- * will be freed in nfs4_remove_reclaim_record in the normal case).
  */
 struct nfs4_client_reclaim *
 nfs4_client_to_reclaim(struct xdr_netobj name, struct xdr_netobj princhash,
@@ -8812,6 +8809,22 @@ nfs4_client_to_reclaim(struct xdr_netobj name, struct xdr_netobj princhash,
        unsigned int strhashval;
        struct nfs4_client_reclaim *crp;
 
+       name.data = kmemdup(name.data, name.len, GFP_KERNEL);
+       if (!name.data) {
+               dprintk("%s: failed to allocate memory for name.data!\n",
+                       __func__);
+               return NULL;
+       }
+       if (princhash.len) {
+               princhash.data = kmemdup(princhash.data, princhash.len, GFP_KERNEL);
+               if (!princhash.data) {
+                       dprintk("%s: failed to allocate memory for princhash.data!\n",
+                               __func__);
+                       kfree(name.data);
+                       return NULL;
+               }
+       } else
+               princhash.data = NULL;
        crp = alloc_reclaim();
        if (crp) {
                strhashval = clientstr_hashval(name);
@@ -8823,6 +8836,9 @@ nfs4_client_to_reclaim(struct xdr_netobj name, struct xdr_netobj princhash,
                crp->cr_princhash.len = princhash.len;
                crp->cr_clp = NULL;
                nn->reclaim_str_hashtbl_size++;
+       } else {
+               kfree(name.data);
+               kfree(princhash.data);
        }
        return crp;
 }