diff mbox series

[v3,1/1] NFSv4.2 COPY do not allocate memory under the lock

Message ID 20181116190151.50707-1-olga.kornievskaia@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/1] NFSv4.2 COPY do not allocate memory under the lock | expand

Commit Message

Olga Kornievskaia Nov. 16, 2018, 7:01 p.m. UTC 
From: Olga Kornievskaia <kolga@netapp.com>

Bruce pointed out that we shouldn't allocate memory while holding
a lock in the nfs4_callback_offload() and handle_async_copy()
that deal with a racing CB_OFFLOAD and reply to COPY case.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/callback_proc.c | 22 +++++++++++-----------
 fs/nfs/nfs42proc.c     | 21 ++++++++++++---------
 2 files changed, 23 insertions(+), 20 deletions(-)

Comments

kernel test robot Nov. 20, 2018, 5:13 a.m. UTC | #1 
Hi Olga,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on nfs/linux-next]
[also build test WARNING on v4.20-rc3 next-20181119]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Olga-Kornievskaia/NFSv4-2-COPY-do-not-allocate-memory-under-the-lock/20181119-202855
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   include/linux/slab.h:332:43: warning: dubious: x & !y
   include/linux/slab.h:332:43: warning: dubious: x & !y
   include/linux/slab.h:332:43: warning: dubious: x & !y
   include/linux/slab.h:332:43: warning: dubious: x & !y
>> fs/nfs/nfs42proc.c:147:28: warning: context imbalance in 'handle_async_copy' - unexpected unlock

vim +/handle_async_copy +147 fs/nfs/nfs42proc.c

   133	
   134	static int handle_async_copy(struct nfs42_copy_res *res,
   135				     struct nfs_server *server,
   136				     struct file *src,
   137				     struct file *dst,
   138				     nfs4_stateid *src_stateid)
   139	{
   140		struct nfs4_copy_state *copy, *tmp_copy;
   141		int status = NFS4_OK;
   142		bool found_pending = false;
   143		struct nfs_open_context *ctx = nfs_file_open_context(dst);
   144	
   145		copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
   146		if (!copy) {
 > 147			spin_unlock(&server->nfs_client->cl_lock);
   148			return -ENOMEM;
   149		}
   150	
   151		spin_lock(&server->nfs_client->cl_lock);
   152		list_for_each_entry(tmp_copy, &server->nfs_client->pending_cb_stateids,
   153					copies) {
   154			if (memcmp(&res->write_res.stateid, &tmp_copy->stateid,
   155					NFS4_STATEID_SIZE))
   156				continue;
   157			found_pending = true;
   158			list_del(&tmp_copy->copies);
   159			break;
   160		}
   161		if (found_pending) {
   162			spin_unlock(&server->nfs_client->cl_lock);
   163			kfree(copy);
   164			copy = tmp_copy;
   165			goto out;
   166		}
   167	
   168		memcpy(&copy->stateid, &res->write_res.stateid, NFS4_STATEID_SIZE);
   169		init_completion(&copy->completion);
   170		copy->parent_state = ctx->state;
   171	
   172		list_add_tail(&copy->copies, &server->ss_copies);
   173		spin_unlock(&server->nfs_client->cl_lock);
   174	
   175		status = wait_for_completion_interruptible(&copy->completion);
   176		spin_lock(&server->nfs_client->cl_lock);
   177		list_del_init(&copy->copies);
   178		spin_unlock(&server->nfs_client->cl_lock);
   179		if (status == -ERESTARTSYS) {
   180			goto out_cancel;
   181		} else if (copy->flags) {
   182			status = -EAGAIN;
   183			goto out_cancel;
   184		}
   185	out:
   186		res->write_res.count = copy->count;
   187		memcpy(&res->write_res.verifier, &copy->verf, sizeof(copy->verf));
   188		status = -copy->error;
   189	
   190		kfree(copy);
   191		return status;
   192	out_cancel:
   193		nfs42_do_offload_cancel_async(dst, &copy->stateid);
   194		kfree(copy);
   195		return status;
   196	}
   197	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox series

Patch

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index fa515d5..48b2e90 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -686,20 +686,24 @@  __be32 nfs4_callback_offload(void *data, void *dummy,
 {
 	struct cb_offloadargs *args = data;
 	struct nfs_server *server;
-	struct nfs4_copy_state *copy;
+	struct nfs4_copy_state *copy, *tmp_copy;
 	bool found = false;
 
+	copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
+	if (!copy)
+		return htonl(NFS4ERR_SERVERFAULT);
+
 	spin_lock(&cps->clp->cl_lock);
 	rcu_read_lock();
 	list_for_each_entry_rcu(server, &cps->clp->cl_superblocks,
 				client_link) {
-		list_for_each_entry(copy, &server->ss_copies, copies) {
+		list_for_each_entry(tmp_copy, &server->ss_copies, copies) {
 			if (memcmp(args->coa_stateid.other,
-					copy->stateid.other,
+					tmp_copy->stateid.other,
 					sizeof(args->coa_stateid.other)))
 				continue;
-			nfs4_copy_cb_args(copy, args);
-			complete(&copy->completion);
+			nfs4_copy_cb_args(tmp_copy, args);
+			complete(&tmp_copy->completion);
 			found = true;
 			goto out;
 		}
@@ -707,15 +711,11 @@  __be32 nfs4_callback_offload(void *data, void *dummy,
 out:
 	rcu_read_unlock();
 	if (!found) {
-		copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
-		if (!copy) {
-			spin_unlock(&cps->clp->cl_lock);
-			return htonl(NFS4ERR_SERVERFAULT);
-		}
 		memcpy(&copy->stateid, &args->coa_stateid, NFS4_STATEID_SIZE);
 		nfs4_copy_cb_args(copy, args);
 		list_add_tail(&copy->copies, &cps->clp->pending_cb_stateids);
-	}
+	} else
+		kfree(copy);
 	spin_unlock(&cps->clp->cl_lock);
 
 	return 0;
diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
index c7c2ffa..7f2b716 100644
--- a/fs/nfs/nfs42proc.c
+++ b/fs/nfs/nfs42proc.c
@@ -158,31 +158,34 @@  static int handle_async_copy(struct nfs42_copy_res *res,
 			     struct file *dst,
 			     nfs4_stateid *src_stateid)
 {
-	struct nfs4_copy_state *copy;
+	struct nfs4_copy_state *copy, *tmp_copy;
 	int status = NFS4_OK;
 	bool found_pending = false;
 	struct nfs_open_context *ctx = nfs_file_open_context(dst);
 
+	copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
+	if (!copy) {
+		spin_unlock(&server->nfs_client->cl_lock);
+		return -ENOMEM;
+	}
+
 	spin_lock(&server->nfs_client->cl_lock);
-	list_for_each_entry(copy, &server->nfs_client->pending_cb_stateids,
+	list_for_each_entry(tmp_copy, &server->nfs_client->pending_cb_stateids,
 				copies) {
-		if (memcmp(&res->write_res.stateid, &copy->stateid,
+		if (memcmp(&res->write_res.stateid, &tmp_copy->stateid,
 				NFS4_STATEID_SIZE))
 			continue;
 		found_pending = true;
-		list_del(&copy->copies);
+		list_del(&tmp_copy->copies);
 		break;
 	}
 	if (found_pending) {
 		spin_unlock(&server->nfs_client->cl_lock);
+		kfree(copy);
+		copy = tmp_copy;
 		goto out;
 	}
 
-	copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
-	if (!copy) {
-		spin_unlock(&server->nfs_client->cl_lock);
-		return -ENOMEM;
-	}
 	memcpy(&copy->stateid, &res->write_res.stateid, NFS4_STATEID_SIZE);
 	init_completion(&copy->completion);
 	copy->parent_state = ctx->state;