Re: possible bug/oops in nfs_pageio_add_request (2.6.22-rc2)?

[Trond Myklebust <trond.myklebust@fys.uio.no>]

On Wed, 2007-05-23 at 21:16 -0400, Erez Zadok wrote:
> I've hit a NULL ptr deref on desc->pg_error below, triggered when mounting a
> stackable file system on top of nfsv3:
> 
> // from file: nfs/pagelist.c
> int nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
> 			   struct nfs_page *req)
> {
> 	while (!nfs_pageio_do_add_request(desc, req)) {
> 		nfs_pageio_doio(desc);
> 		if (desc->pg_error < 0)
> 
> Scenario:
> 
> 2.6.22-rc2 with Unionfs 2.0 (release u2 for 2.6.22-rc2, which includes mmap
> support).
> 
> I mount unionfs on top of nfs (v3).  I have one file in the nfs branch.  I
> run a simple program through the union which mmap's the file, changes the
> first byte of the file, calls msync(), and then closes.  This causes
> unionfs_writepage to be invoked, which in turn calls the lower file system's
> ->writepage, here nfs_writepage.
> 
> The 'wbc' that's passed to unionfs_writepage from the VFS has this:
> 
>     wbc->for_writepages = 1
>     wbc->fs_private = NULL
> 
> If you follow the logic, then nfs_writepage calls nfs_writepage_locked,
> passing the same wbc.  nfs_writepage_locked does this:
> 
> 	if (wbc->for_writepages)
> 		pgio = wbc->fs_private;
> 	else {
> 		nfs_pageio_init_write(&mypgio, inode, wb_priority(wbc));
> 		pgio = &mypgio;
> 	}
> 
> which means that pgio is set to NULL from the caller's wbc.  Then
> nfs_writepage_locked calls nfs_page_async_flush, passing it this pgio
> (NULL).  nfs_page_async_flush invokes nfs_pageio_add_request, passing it
> this NULL pgio.  Inside nfs_pageio_add_request the NULL is being
> dereferenced as desc->pg_error and we get an oops.
> 
> As a workaround, in unionfs_writepage I tried this before calling the lower
> file system's ->writepage (which was nfs_writepage):
> 
> 	struct writeback_control lower_wbc;
> 	memcpy(&lower_wbc, wbc, sizeof(struct writeback_control));
> 	if (lower_wbc.for_writepages && !lower_wbc.fs_private) {
> 		printk("unionfs: setting wbc.for_writepages to 0\n");
> 		lower_wbc.for_writepages = 0;
> 	}
> 
> Then I passed &lower_wbc to the lower file system's writepage method
> (nfs_writepage).  It works; no oops, and the file in question was sync'ed to
> the backing f/s too.  But I'm not sure if it's the correct workaround and
> whether it'd break things for other non-NFS file systems.
> 
> It's possible that I'm doing something wrong in unionfs's mmap code, which
> indirectly results in a malformed wbc structure being passed to unionfs (by
> malformed I mean that wbc->fs_private is NULL and wbc->for_writepages is set
> to 1).  If such a wbc can be created by any other means and passed to NFS,
> then nfs probably will continue to oops even w/o unionfs.
> 
> FWIW, I tried a similar scenario with eCryptfs (another stackable f/s in
> 2.6.22-rc2) on top of NFSv3, and got the same oops (sorry, Mike :-)
> 
> Any pointers would be appreciated.

If this is truly a call to ->writepages() by the VFS (as opposed to a
call to ->writepage()) then why is unionfs' writepages() failing to call
the underlying writepages method of the host filesystem: in this case
nfs_writepages()?

Trond