nfs: kstrdup() memory handling

Commit Message

Sanidhya Kashyap March 21, 2015, 5:07 p.m. UTC

The following patch tries to correctly handle the memory failure even
for kstrdup() during memory pressure. Therefore, putting extra check
for ENOMEM in places where the kstrdup() has not been validated. Further
more checks have been added which called the functions
nfs4_init_nonuniform_client_string(), nfs4_init_uniform_client_string()
and _nfs4_proc_exchange_id().

Signed-off-by: Sanidhya Kashyap <[e-mail]>
---
 fs/nfs/nfs4client.c |  9 ++++++++-
 fs/nfs/nfs4proc.c   | 30 ++++++++++++++++++++++++------
 2 files changed, 32 insertions(+), 7 deletions(-)

Comments

Christoph Hellwig March 25, 2015, 8:40 a.m. UTC | #1

On Sat, Mar 21, 2015 at 01:07:28PM -0400, Sanidhya Kashyap wrote:
> index 86d6214..08bf1f1 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -1217,8 +1217,15 @@ int nfs4_update_server(struct nfs_server *server, const char *hostname,
>  		goto out;
>  	}
>  
> -	if (server->nfs_client->cl_hostname == NULL)
> +	if (server->nfs_client->cl_hostname == NULL) {
>  		server->nfs_client->cl_hostname = kstrdup(hostname, GFP_KERNEL);
> +		if (!server->nfs_client->cl_hostname) {
> +			error = -ENOMEM;
> +			dprintk("<-- %s(): not enough memory %d\n",
> +				__func__, error);
> +			goto out;
> +		}
> +	}
>  	nfs_server_insert_lists(server);

The function alays ensures to call nfs_server_insert_lists on each exit
path, and you add one that doesn't.

>  
>  	error = nfs_probe_destination(server);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 627f37c..2d8b408 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4974,6 +4974,9 @@ nfs4_init_nonuniform_client_string(struct nfs_client *clp,
>  							RPC_DISPLAY_PROTO));
>  	rcu_read_unlock();
>  	clp->cl_owner_id = kstrdup(buf, GFP_KERNEL);
> +	if (!clp->cl_owner_id)
> +		return -ENOMEM;
> +
>  	return result;
>  }
>  
> @@ -4998,6 +5001,9 @@ nfs4_init_uniform_client_string(struct nfs_client *clp,
>  				clp->rpc_ops->version, clp->cl_minorversion,
>  				nodename);
>  	clp->cl_owner_id = kstrdup(buf, GFP_KERNEL);
> +	if (!clp->cl_owner_id)
> +		return -ENOMEM;
> +

Both these use a scnprintf + kstrdup pattern thast should be replaced by
kasprintf.  But I still don't understand why both function get away with
just returning the caller supplied buf in one case, but need a dynamic
allocation in the other case.
--
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

Patch

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 86d6214..08bf1f1 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -1217,8 +1217,15 @@  int nfs4_update_server(struct nfs_server *server, const char *hostname,
 		goto out;
 	}
 
-	if (server->nfs_client->cl_hostname == NULL)
+	if (server->nfs_client->cl_hostname == NULL) {
 		server->nfs_client->cl_hostname = kstrdup(hostname, GFP_KERNEL);
+		if (!server->nfs_client->cl_hostname) {
+			error = -ENOMEM;
+			dprintk("<-- %s(): not enough memory %d\n",
+				__func__, error);
+			goto out;
+		}
+	}
 	nfs_server_insert_lists(server);
 
 	error = nfs_probe_destination(server);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 627f37c..2d8b408 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4974,6 +4974,9 @@  nfs4_init_nonuniform_client_string(struct nfs_client *clp,
 							RPC_DISPLAY_PROTO));
 	rcu_read_unlock();
 	clp->cl_owner_id = kstrdup(buf, GFP_KERNEL);
+	if (!clp->cl_owner_id)
+		return -ENOMEM;
+
 	return result;
 }
 
@@ -4998,6 +5001,9 @@  nfs4_init_uniform_client_string(struct nfs_client *clp,
 				clp->rpc_ops->version, clp->cl_minorversion,
 				nodename);
 	clp->cl_owner_id = kstrdup(buf, GFP_KERNEL);
+	if (!clp->cl_owner_id)
+		return -ENOMEM;
+
 	return result;
 }
 
@@ -5062,19 +5068,24 @@  int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,
 		.flags = RPC_TASK_TIMEOUT,
 	};
 	int status;
+	int ret;
 
 	/* nfs_client_id4 */
 	nfs4_init_boot_verifier(clp, &sc_verifier);
 	if (test_bit(NFS_CS_MIGRATION, &clp->cl_flags))
-		setclientid.sc_name_len =
-				nfs4_init_uniform_client_string(clp,
+		ret = nfs4_init_uniform_client_string(clp,
 						setclientid.sc_name,
 						sizeof(setclientid.sc_name));
 	else
-		setclientid.sc_name_len =
-				nfs4_init_nonuniform_client_string(clp,
+		ret = nfs4_init_nonuniform_client_string(clp,
 						setclientid.sc_name,
 						sizeof(setclientid.sc_name));
+	if (ret < 0) {
+		status = ret;
+		goto out;
+	}
+	setclientid.sc_name_len = ret;
+
 	/* cb_client4 */
 	setclientid.sc_netid_len =
 				nfs4_init_callback_netid(clp,
@@ -6837,6 +6848,7 @@  static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,
 		0
 	};
 	int status;
+	int ret;
 	struct rpc_message msg = {
 		.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_EXCHANGE_ID],
 		.rpc_argp = &args,
@@ -6845,8 +6857,14 @@  static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,
 	};
 
 	nfs4_init_boot_verifier(clp, &verifier);
-	args.id_len = nfs4_init_uniform_client_string(clp, args.id,
-							sizeof(args.id));
+	ret = nfs4_init_uniform_client_string(clp, args.id,
+						sizeof(args.id));
+	if (ret < 0) {
+		status = ret;
+		goto out;
+	}
+	args.id_len = ret;
+
 	dprintk("NFS call  exchange_id auth=%s, '%.*s'\n",
 		clp->cl_rpcclient->cl_auth->au_ops->au_name,
 		args.id_len, args.id);