[PATCH v2 0/2] pnfs: fix a crash when hitting Ctrl+C during LAYOUTGET

[PATCH v2 0/2] pnfs: fix a crash when hitting Ctrl+C during LAYOUTGET

[Idan Kedar]

While working on object layout, we have encountered a general protection fault
in xdr_shrink_bufhead when killing a process performing a lot of reads.

full trace:

[ 1353.258729] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 1353.259109] CPU 0 
[ 1353.259109] Modules linked in:[ 1353.259109]  objlayoutdriver exofs libore osd libosd iscsi_tcp netconsole nfs nfsd lockd fscache auth_rpcgss nfs_acl sunrpc e1000 rtc_cmos serio_raw microcode [last unloaded: scsi_wait_scan]

[ 1353.259109] Pid: 4, comm: kworker/0:0 Not tainted 3.5.0-nfsobj #147 innotek GmbH VirtualBox
[ 1353.259109] RIP: 0010:[<ffffffff8132cf2d>]  [<ffffffff8132cf2d>] memcpy+0xd/0x110
[ 1353.259109] RSP: 0018:ffff88003d6cdab8  EFLAGS: 00010202
[ 1353.259109] RAX: ffff88003b51928c RBX: ffff88003b51928c RCX: 000000000000000d
[ 1353.259109] RDX: 0000000000000004 RSI: 0005080000000004 RDI: ffff88003b51928c
[ 1353.259109] RBP: ffff88003d6cdb00 R08: ffff88003a9d0748 R09: 0000000000000001
[ 1353.259109] R10: 0000000000000000 R11: 0000000000000001 R12: 000000000000006c
[ 1353.259109] R13: 0000000000000004 R14: 000000000000006c R15: ffff88003d6cc000
[ 1353.259109] FS:  0000000000000000(0000) GS:ffff88003e200000(0000) knlGS:0000000000000000
[ 1353.259109] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1353.259109] CR2: 0000003ce4076770 CR3: 000000003772c000 CR4: 00000000000006f0
[ 1353.259109] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1353.259109] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 1353.259109] Process kworker/0:0 (pid: 4, threadinfo ffff88003d6cc000, task ffff88003d6d0000)
[ 1353.259109] Stack:
[ 1353.259109]  ffffffffa0056e17 ffff88003d6cdfd8 ffff88003bebf8c8 ffff88003d6cdae0
[ 1353.259109]  ffff88003bebc390 0000000000000ffc 0000000000021000 0000000000021000
[ 1353.259109]  ffff88003b518284 ffff88003d6cdb70 ffffffffa005779f ffff88003e3d4a80
[ 1353.259109] Call Trace:
[ 1353.259109]  [<ffffffffa0056e17>] ? _copy_from_pages+0xa7/0xe0 [sunrpc]
[ 1353.259109]  [<ffffffffa005779f>] xdr_shrink_bufhead+0x7f/0x270 [sunrpc]
[ 1353.259109]  [<ffffffffa00579e2>] xdr_read_pages+0x42/0x150 [sunrpc]
[ 1353.259109]  [<ffffffffa01668a4>] nfs4_xdr_dec_layoutget+0x174/0x180 [nfs]
[ 1353.259109]  [<ffffffffa0166730>] ? decode_getfh+0x120/0x120 [nfs]
[ 1353.259109]  [<ffffffffa0166730>] ? decode_getfh+0x120/0x120 [nfs]
[ 1353.259109]  [<ffffffffa004de05>] rpcauth_unwrap_resp+0x65/0x70 [sunrpc]
[ 1353.259109]  [<ffffffffa0043f47>] call_decode+0x377/0x470 [sunrpc]
[ 1353.259109]  [<ffffffff810c216d>] ? trace_hardirqs_off+0xd/0x10
[ 1353.259109]  [<ffffffffa0043bd0>] ? call_status+0x210/0x210 [sunrpc]
[ 1353.259109]  [<ffffffffa0043bd0>] ? call_status+0x210/0x210 [sunrpc]
[ 1353.259109]  [<ffffffffa004bb14>] __rpc_execute+0x64/0x2b0 [sunrpc]
[ 1353.259109]  [<ffffffffa004bd60>] ? __rpc_execute+0x2b0/0x2b0 [sunrpc]
[ 1353.259109]  [<ffffffffa004bd75>] rpc_async_schedule+0x15/0x20 [sunrpc]
[ 1353.259109]  [<ffffffff81084294>] process_one_work+0x1a4/0x4f0
[ 1353.259109]  [<ffffffff81084231>] ? process_one_work+0x141/0x4f0
[ 1353.259109]  [<ffffffff81085fc2>] worker_thread+0x162/0x350
[ 1353.259109]  [<ffffffff81085e60>] ? manage_workers.clone.19+0x240/0x240
[ 1353.259109]  [<ffffffff8108b5c6>] kthread+0xc6/0xd0
[ 1353.259109]  [<ffffffff81097426>] ? finish_task_switch+0x46/0xf0
[ 1353.259109]  [<ffffffff817a91b4>] kernel_thread_helper+0x4/0x10
[ 1353.259109]  [<ffffffff8179f9f0>] ? retint_restore_args+0x13/0x13
[ 1353.259109]  [<ffffffff8108b500>] ? __init_kthread_worker+0x70/0x70
[ 1353.259109]  [<ffffffff817a91b0>] ? gs_change+0x13/0x13
[ 1353.259109] Code: 43 50 88 43 4e 48 83 c4 08 5b c9 c3 66 90 e8 eb fb ff ff eb e6 90 90 90 90 90 90 90 90 90 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 20 4c 8b 06 4c 8b 4e 08 4c 8b 56 10 4c 
[ 1353.259109] RIP  [<ffffffff8132cf2d>] memcpy+0xd/0x110
[ 1353.259109]  RSP <ffff88003d6cdab8>
[ 1353.328569] ---[ end trace a48bf911452c4212 ]---

to reproduce:
mount an object-based pNFS file system. we used exofs as the MDS. assume the
mount point is /mnt/pnfs, run:

cp -r /bin /mnt/pnfs
cd /mnt/pnfs
while true; do
        rm -rf bin2
        echo 3 > /proc/sys/vm/drop_caches
        cp -r bin bin2 &
        sleep 1
        kill -s int $!
done

on my setup it crashed after a couple of minutes, sometimes a couple of seconds.
your mileage may vary.

differences from v1:
* fold 3rd patch into first one.
* use kcalloc() instead of kzalloc().
* functions renamed to better fit the convention.

Idan Kedar (2):
  pnfs: defer release of pages in layoutget
  pnfs: nfs4_proc_layoutget returns void

 fs/nfs/nfs4proc.c |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/nfs/pnfs.c     |   39 +---------------------------------
 fs/nfs/pnfs.h     |    2 +-
 3 files changed, 60 insertions(+), 42 deletions(-)

-- 
1.7.6.5

[PATCH v2 1/2] pnfs: defer release of pages in layoutget

[Idan Kedar]

we have encountered a bug whereby reading a lot of files (copying
fedora's /bin) from a pNFS mount and hitting Ctrl+C in the middle caused
a general protection fault in xdr_shrink_bufhead. this function is
called when decoding the response from LAYOUTGET. the decoding is done
by a worker thread, and the caller of LAYOUTGET waits for the worker
thread to complete.

hitting Ctrl+C caused the synchronous wait to end and the next thing the
caller does is to free the pages, so when the worker thread calls
xdr_shrink_bufhead, the pages are gone. therefore, the cleanup of these
pages has been moved to nfs4_layoutget_release.

Signed-off-by: Idan Kedar <idank@tonian.com>
Signed-off-by: Benny Halevy <bhalevy@tonian.com>
---
 fs/nfs/nfs4proc.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/nfs/pnfs.c     |   39 +-----------------------------------
 fs/nfs/pnfs.h     |    2 +-
 3 files changed, 58 insertions(+), 40 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 15fc7e4..1a1776b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6164,11 +6164,58 @@ static void nfs4_layoutget_done(struct rpc_task *task, void *calldata)
 	dprintk("<-- %s\n", __func__);
 }
 
+static size_t max_response_pages(struct nfs_server *server)
+{
+	u32 max_resp_sz = server->nfs_client->cl_session->fc_attrs.max_resp_sz;
+	return nfs_page_array_len(0, max_resp_sz);
+}
+
+static void nfs4_free_pages(struct page **pages, size_t size)
+{
+	int i;
+
+	if (!pages)
+		return;
+
+	for (i = 0; i < size; i++) {
+		if (!pages[i])
+			break;
+		__free_page(pages[i]);
+	}
+	kfree(pages);
+}
+
+static struct page **nfs4_alloc_pages(size_t size, gfp_t gfp_flags)
+{
+	struct page **pages;
+	int i;
+
+	pages = kcalloc(size, sizeof(struct page *), gfp_flags);
+	if (!pages) {
+		dprintk("%s: can't alloc array of %zu pages\n", __func__, size);
+		return NULL;
+	}
+
+	for (i = 0; i < size; i++) {
+		pages[i] = alloc_page(gfp_flags);
+		if (!pages[i]) {
+			dprintk("%s: failed to allocate page\n", __func__);
+			nfs4_free_pages(pages, size);
+			return NULL;
+		}
+	}
+
+	return pages;
+}
+
 static void nfs4_layoutget_release(void *calldata)
 {
 	struct nfs4_layoutget *lgp = calldata;
+	struct nfs_server *server = NFS_SERVER(lgp->args.inode);
+	size_t max_pages = max_response_pages(server);
 
 	dprintk("--> %s\n", __func__);
+	nfs4_free_pages(lgp->args.layout.pages, max_pages);
 	put_nfs_open_context(lgp->args.ctx);
 	kfree(calldata);
 	dprintk("<-- %s\n", __func__);
@@ -6180,9 +6227,10 @@ static const struct rpc_call_ops nfs4_layoutget_call_ops = {
 	.rpc_release = nfs4_layoutget_release,
 };
 
-int nfs4_proc_layoutget(struct nfs4_layoutget *lgp)
+int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
 {
 	struct nfs_server *server = NFS_SERVER(lgp->args.inode);
+	size_t max_pages = max_response_pages(server);
 	struct rpc_task *task;
 	struct rpc_message msg = {
 		.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_LAYOUTGET],
@@ -6200,6 +6248,13 @@ int nfs4_proc_layoutget(struct nfs4_layoutget *lgp)
 
 	dprintk("--> %s\n", __func__);
 
+	lgp->args.layout.pages = nfs4_alloc_pages(max_pages, gfp_flags);
+	if (!lgp->args.layout.pages) {
+		nfs4_layoutget_release(lgp);
+		return -ENOMEM;
+	}
+	lgp->args.layout.pglen = max_pages * PAGE_SIZE;
+
 	lgp->res.layoutp = &lgp->args.layout;
 	lgp->res.seq_res.sr_slot = NULL;
 	nfs41_init_sequence(&lgp->args.seq_args, &lgp->res.seq_res, 0);
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index bbc49ca..8229a0e 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -583,9 +583,6 @@ send_layoutget(struct pnfs_layout_hdr *lo,
 	struct nfs_server *server = NFS_SERVER(ino);
 	struct nfs4_layoutget *lgp;
 	struct pnfs_layout_segment *lseg = NULL;
-	struct page **pages = NULL;
-	int i;
-	u32 max_resp_sz, max_pages;
 
 	dprintk("--> %s\n", __func__);
 
@@ -594,20 +591,6 @@ send_layoutget(struct pnfs_layout_hdr *lo,
 	if (lgp == NULL)
 		return NULL;
 
-	/* allocate pages for xdr post processing */
-	max_resp_sz = server->nfs_client->cl_session->fc_attrs.max_resp_sz;
-	max_pages = nfs_page_array_len(0, max_resp_sz);
-
-	pages = kcalloc(max_pages, sizeof(struct page *), gfp_flags);
-	if (!pages)
-		goto out_err_free;
-
-	for (i = 0; i < max_pages; i++) {
-		pages[i] = alloc_page(gfp_flags);
-		if (!pages[i])
-			goto out_err_free;
-	}
-
 	lgp->args.minlength = PAGE_CACHE_SIZE;
 	if (lgp->args.minlength > range->length)
 		lgp->args.minlength = range->length;
@@ -616,39 +599,19 @@ send_layoutget(struct pnfs_layout_hdr *lo,
 	lgp->args.type = server->pnfs_curr_ld->id;
 	lgp->args.inode = ino;
 	lgp->args.ctx = get_nfs_open_context(ctx);
-	lgp->args.layout.pages = pages;
-	lgp->args.layout.pglen = max_pages * PAGE_SIZE;
 	lgp->lsegpp = &lseg;
 	lgp->gfp_flags = gfp_flags;
 
 	/* Synchronously retrieve layout information from server and
 	 * store in lseg.
 	 */
-	nfs4_proc_layoutget(lgp);
+	nfs4_proc_layoutget(lgp, gfp_flags);
 	if (!lseg) {
 		/* remember that LAYOUTGET failed and suspend trying */
 		set_bit(lo_fail_bit(range->iomode), &lo->plh_flags);
 	}
 
-	/* free xdr pages */
-	for (i = 0; i < max_pages; i++)
-		__free_page(pages[i]);
-	kfree(pages);
-
 	return lseg;
-
-out_err_free:
-	/* free any allocated xdr pages, lgp as it's not used */
-	if (pages) {
-		for (i = 0; i < max_pages; i++) {
-			if (!pages[i])
-				break;
-			__free_page(pages[i]);
-		}
-		kfree(pages);
-	}
-	kfree(lgp);
-	return NULL;
 }
 
 /* Initiates a LAYOUTRETURN(FILE) */
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 64f90d8..9a31ff3 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -171,7 +171,7 @@ extern int nfs4_proc_getdevicelist(struct nfs_server *server,
 				   struct pnfs_devicelist *devlist);
 extern int nfs4_proc_getdeviceinfo(struct nfs_server *server,
 				   struct pnfs_device *dev);
-extern int nfs4_proc_layoutget(struct nfs4_layoutget *lgp);
+extern int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags);
 extern int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp);
 
 /* pnfs.c */
-- 
1.7.6.5

[PATCH v2 2/2] pnfs: nfs4_proc_layoutget returns void

[Idan Kedar]

since the only user of nfs4_proc_layoutget is send_layoutget, which
ignores its return value, there is no reason to return any value.

Signed-off-by: Idan Kedar <idank@tonian.com>
Signed-off-by: Benny Halevy <bhalevy@tonian.com>
---
 fs/nfs/nfs4proc.c |    8 ++++----
 fs/nfs/pnfs.h     |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1a1776b..a87216d 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6227,7 +6227,7 @@ static const struct rpc_call_ops nfs4_layoutget_call_ops = {
 	.rpc_release = nfs4_layoutget_release,
 };
 
-int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
+void nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
 {
 	struct nfs_server *server = NFS_SERVER(lgp->args.inode);
 	size_t max_pages = max_response_pages(server);
@@ -6251,7 +6251,7 @@ int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
 	lgp->args.layout.pages = nfs4_alloc_pages(max_pages, gfp_flags);
 	if (!lgp->args.layout.pages) {
 		nfs4_layoutget_release(lgp);
-		return -ENOMEM;
+		return;
 	}
 	lgp->args.layout.pglen = max_pages * PAGE_SIZE;
 
@@ -6260,7 +6260,7 @@ int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
 	nfs41_init_sequence(&lgp->args.seq_args, &lgp->res.seq_res, 0);
 	task = rpc_run_task(&task_setup_data);
 	if (IS_ERR(task))
-		return PTR_ERR(task);
+		return;
 	status = nfs4_wait_for_completion_rpc_task(task);
 	if (status == 0)
 		status = task->tk_status;
@@ -6268,7 +6268,7 @@ int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
 		status = pnfs_layout_process(lgp);
 	rpc_put_task(task);
 	dprintk("<-- %s status=%d\n", __func__, status);
-	return status;
+	return;
 }
 
 static void
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 9a31ff3..eda42b9 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -171,7 +171,7 @@ extern int nfs4_proc_getdevicelist(struct nfs_server *server,
 				   struct pnfs_devicelist *devlist);
 extern int nfs4_proc_getdeviceinfo(struct nfs_server *server,
 				   struct pnfs_device *dev);
-extern int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags);
+extern void nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags);
 extern int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp);
 
 /* pnfs.c */
-- 
1.7.6.5