Re: [PATCH] nfs: clear_commit_release incorrectly handle truncated page

Trond Myklebust
2010-02-02T19:54:26Z

On Tue, 2010-02-02 at 20:09 +0300, Dmitry Monakhov wrote:
> Trond Myklebust <trond.myklebust@fys.uio.no> writes:
>
> > On Tue, 2010-02-02 at 19:47 +0300, Dmitry Monakhov wrote:
> >> Trond Myklebust <trond.myklebust@fys.uio.no> writes:
> >> > Hmm.... There is a known problem with a reference leak in
> >> > nfs_wb_page_cancel() (I've queued up a fix for 2.6.33 in the 'bugfixes'
> >> > branch of my git tree already). What happens when you apply the
> >> > following patch?
> >> The not helps, still get the same oops(log follows).
> >> Have you tried my testcase?
> >>
> >> BUG: unable to handle kernel NULL pointer dereference at 00000040
> >> IP: [<f80d415f>] nfs_clear_request_commit+0x3f/0xb0 [nfs]
> >> *pde = 00000000
> >> Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
> >> last sysfs file: /sys/devices/platform/thinkpad_acpi/leds/tpacpi::thinkvantage/uevent
> >> Modules linked in: binfmt_misc quota_v2 quota_tree nfsd exportfs nfs lockd sunrpc iwl3945 thinkpad_acpi psmouse led_class serio_raw iwlcore nvram raid1 raid0 linear e1000e
> >>
> >> Pid: 1035, comm: nfsiod Not tainted 2.6.33-rc6 #60 2623DDU/2623DDU
> >> EIP: 0060:[<f80d415f>] EFLAGS: 00010296 CPU: 0
> >> EIP is at nfs_clear_request_commit+0x3f/0xb0 [nfs]
> >> EAX: 00000000 EBX: c2561d80 ECX: c06d3700 EDX: 00000014
> >> ESI: f69916c0 EDI: f80dab58 EBP: f6724ef4 ESP: f6724ee8
> >> DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> >> Process nfsiod (pid: 1035, ti=f6724000 task=f69dda90 task.ti=f6724000)
> >> Stack:
> >> c04df67d f69d7440 f69916c0 f6724f34 f80d4258 f6724f44 00001b0e 00000000
> >> <0> ffff799c 00000400 f505f000 94c0042a f69917e0 f69917e8 00000000 f69916c0
> >> <0> f69916c4 f69916c0 f80dab58 f6724f3c f8075703 f6724f58 f8075871 f6724f60
> >> Call Trace:
> >> [<c04df67d>] ? schedule+0x3ad/0xa30
> >> [<f80d4258>] ? nfs_commit_release+0x88/0x1a0 [nfs]
> >> [<f8075703>] ? rpc_release_calldata+0x13/0x20 [sunrpc]
> >> [<f8075871>] ? rpc_free_task+0x41/0x70 [sunrpc]
> >> [<c01acc4c>] ? probe_workqueue_execution+0x8c/0xd0
> >> [<f8075940>] ? rpc_async_release+0x10/0x20 [sunrpc]
> >> [<c015834d>] ? worker_thread+0x10d/0x210
> >> [<f8075930>] ? rpc_async_release+0x0/0x20 [sunrpc]
> >> [<c015bb10>] ? autoremove_wake_function+0x0/0x50
> >> [<c0158240>] ? worker_thread+0x0/0x210
> >> [<c015b724>] ? kthread+0x74/0x80
> >> [<c015b6b0>] ? kthread+0x0/0x80
> >> [<c0103546>] ? kernel_thread_helper+0x6/0x10
> >> Code: f0 0f ba 70 28 01 19 d2 31 c0 85 d2 75 0e 8b 5d f8 8b 75 fc 89 ec 5d c3 8d 74 26 00 89 d8 ba 10 00 00 00 e8 74 0e 10 c8 8b 43 10 <8b> 70 40 9c 5b fa e8 26 67 0d c8 8d 46 30 b9 ff ff ff ff 0f bd
> >> EIP: [<f80d415f>] nfs_clear_request_commit+0x3f/0xb0 [nfs] SS:ESP 0068:f6724ee8
> >> CR2: 0000000000000040
> >> ---[ end trace 4bf8ee9d233ce744 ]---
> >
> > Yep. Looking more carefully at your test case, I don't see how
> > truncate_inode_page() can be involved at all. You are extending the file
> > using lseek(), not truncate(). So something else must be at work here.
> open(,O_TRUNC,)
> do_filp_open()
> handle_truncate()
> do_truncate()
> Yess this is craziness to run concurrent tasks which do:
> open(,O_TRUNC,); mmap();
> But initially i've done this by occasion and this result in OOps :)
> >
> > I'll see if I can reproduce it.
> >

OK. I haven't been able to reproduce your bug yet, but I think I see
what is happening.

Your 'kill -9' will occasionally hit nfs_wb_page_cancel() and cause it
to fail. When _that_ happens, then all hell breaks loose, because
mapping->a_ops->invalidatepage() is not allowed to fail.

Ugh... I don't think there much of an alternative to making
nfs_wait_on_request() uninterruptible. On the plus side, that does make
the behaviour of the NFS writeback code consistent with that of the VFS
layer (i.e. wait_on_page_writeback()).

So here goes...

Trond
----------------------------------------------------------------------------------------------
NFS: Fix an Oops when truncating a file

From: Trond Myklebust <Trond.Myklebust@netapp.com>

The VM/VFS does not allow mapping->a_ops->invalidatepage() to fail.
Unfortunately, nfs_wb_page_cancel() may fail if a fatal signal occurs.
Since the NFS code assumes that the page stays mapped for as long as the
writeback is active, we can end up Oopsing (among other things).

The only safe fix here is to convert nfs_wait_on_request(), so as to make
it uninterruptible (as is already the case with wait_on_page_writeback()).

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

fs/nfs/pagelist.c | 17 +++++++++--------
1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index e297593..a12c45b 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -176,6 +176,12 @@ void nfs_release_request(struct nfs_page *req)
kref_put(&req->wb_kref, nfs_free_request);
}

+static int nfs_wait_bit_uninterruptible(void *word)
+{
+ io_schedule();
+ return 0;
+}
+
/**
* nfs_wait_on_request - Wait for a request to complete.
* @req: request to wait upon.
@@ -186,14 +192,9 @@ void nfs_release_request(struct nfs_page *req)
int
nfs_wait_on_request(struct nfs_page *req)
{
- int ret = 0;
-
- if (!test_bit(PG_BUSY, &req->wb_flags))
- goto out;
- ret = out_of_line_wait_on_bit(&req->wb_flags, PG_BUSY,
- nfs_wait_bit_killable, TASK_KILLABLE);
-out:
- return ret;
+ return wait_on_bit(&req->wb_flags, PG_BUSY,
+ nfs_wait_bit_uninterruptible,
+ TASK_UNINTERRUPTIBLE);
}

/**