NFS: possible NULL pointer deref in nfs_sillyrename()

PATCH In review Jesper Juhl 2 participants 4 messages Aug 16, 2006 – Sep 19, 2006

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Jesper Juhl

author

2006-08-16 22:21 UTC #0 lore raw

PATCH 1/1 NFS: possible NULL pointer deref in nfs_sillyrename()

The coverity checker spotted this as bug #1013.

If we get a NULL dentry->d_inode, then regardless of NFS_PARANOIA or no NFS_PARANOIA, then if
if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
turns out to be false we'll end up dereferencing that NULL d_inode in two places below.

And since the check for "(!dentry->d_inode)" even exists (although inside #ifdef NFS_PARANOIA) I take that to mean that this is a possibility.
And the fact that we check

    if (dentry->d_flags & DCACHE_NFSFS_RENAMED)

must also mean that there are cases where the check could fail.

So, we can get in trouble here :

1) as an arg to sprintf() :
sprintf(silly, ".nfs%*.*lx",
i_inosize, i_inosize, dentry->d_inode->i_ino);

2) or we pass it to nfs_inode_return_delegation() which then dereferences it. nfs_inode_return_delegation(dentry->d_inode);

I propose the following patch to handle this.

Compile tested only.

If this patch is somehow incorrect I'd appreciate an explanation of why :)

Signed-off-by Jesper Juhl <[email protected]>

---

 fs/nfs/dir.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

fs/nfs/dir.c +6 -6

@@ -1299,15 +1299,15 @@ static int nfs_sillyrename(struct inode 
 		atomic_read(&dentry->d_count));
 	nfs_inc_stats(dir, NFSIOS_SILLYRENAME);
 
-#ifdef NFS_PARANOIA
-if (!dentry->d_inode)
-printk("NFS: silly-renaming %s/%s, negative dentry??\n",
-dentry->d_parent->d_name.name, dentry->d_name.name);
-#endif
+	error = -EBUSY;
+	if (!dentry->d_inode) {
+		printk("NFS: silly-renaming %s/%s, negative dentry??\n",
+			dentry->d_parent->d_name.name, dentry->d_name.name);
+		goto out;
+	}
 	/*
 	 * We don't allow a dentry to be silly-renamed twice.
 	 */
-	error = -EBUSY;
 	if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
 		goto out;

Trond Myklebust

2006-08-17 00:16 UTC #1 lore raw

Quoted 11 lines · Jesper Juhl

The coverity checker spotted this as bug #1013.
If we get a NULL dentry->d_inode, then regardless of NFS_PARANOIA or no NFS_PARANOIA, then if
if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
turns out to be false we'll end up dereferencing that NULL d_inode in two places below.
And since the check for "(!dentry->d_inode)" even exists (although inside #ifdef NFS_PARANOIA) I take that to mean that this is a possibility.

Sorry, but it isn't possible. See the checks in may_delete() (which is called before ->unlink()) and nfs_rename().

IOW: Feel free to kill the NFS_PARANOIA crap. It looks like legacy code from a debugging session about a decade or so ago.

Cheers,
Trond

Jesper Juhl

author

2006-08-17 10:05 UTC #2 lore raw

Quoted 9 lines · Trond Myklebust

Quoted 8 lines · Jesper Juhl
The coverity checker spotted this as bug #1013.
If we get a NULL dentry->d_inode, then regardless of NFS_PARANOIA or no NFS_PARANOIA, then if
if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
turns out to be false we'll end up dereferencing that NULL d_inode in two places below.

Quoted 7 lines

Quoted 3 lines
And since the check for "(!dentry->d_inode)" even exists (although inside #ifdef NFS_PARANOIA) I take that to mean that this is a possibility.
Sorry, but it isn't possible. See the checks in may_delete() (which is called before ->unlink()) and nfs_rename().

Thanks, that was useful info.

Quoted 3 lines

IOW: Feel free to kill the NFS_PARANOIA crap. It looks like legacy code from a debugging session about a decade or so ago.

Sure thing, I'll cook up a patch to do that.

Signature · 12 lines

Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Jesper Juhl

author

2006-09-19 00:25 UTC #3 lore raw

Kill NFS_PARANOIA (was: Re: [PATCH] NFS: possible NULL pointer deref in nfs_sillyrename() )

Quoted 1 line · Jesper Juhl

On 17/08/06, Trond Myklebust <[email protected]> wrote:

[...]

Quoted 6 lines

Quoted 4 lines
IOW: Feel free to kill the NFS_PARANOIA crap. It looks like legacy code from a debugging session about a decade or so ago.
Sure thing, I'll cook up a patch to do that.

How about something like this:

Remove obsolete NFS_PARANOIA

Signed-off-by Jesper Juhl <[email protected]>

---

 fs/nfs/dir.c      |   22 ++--------------------
 fs/nfs/inode.c    |   11 +----------
 fs/nfs/nfs2xdr.c  |    1 -
 fs/nfs/pagelist.c |    7 ++-----
 4 files changed, 5 insertions(+), 36 deletions(-)

fs/nfs/dir.c +2 ~~-20~~

diff -upr linux-2.6.18-rc7-git2-orig/fs/nfs/dir.c linux-2.6.18-rc7-git2/fs/nfs/dir.c
@@ -36,7 +36,6 @@
 #include "delegation.h"
 #include "iostat.h"
 
-#define NFS_PARANOIA 1
 /* #define NFS_DEBUG_VERBOSE 1 */
 
 static int nfs_opendir(struct inode *, struct file *);

@@ -1299,11 +1298,7 @@ static int nfs_sillyrename(struct inode 
 		atomic_read(&dentry->d_count));
 	nfs_inc_stats(dir, NFSIOS_SILLYRENAME);
 
-#ifdef NFS_PARANOIA
-if (!dentry->d_inode)
-printk("NFS: silly-renaming %s/%s, negative dentry??\n",
-dentry->d_parent->d_name.name, dentry->d_name.name);
-#endif
+	BUG_ON(!dentry->d_inode);
 	/*
 	 * We don't allow a dentry to be silly-renamed twice.
 	 */

@@ -1452,11 +1447,6 @@ nfs_symlink(struct inode *dir, struct de
 	dfprintk(VFS, "NFS: symlink(%s/%ld, %s, %s)\n", dir->i_sb->s_id,
 		dir->i_ino, dentry->d_name.name, symname);
 
-#ifdef NFS_PARANOIA
-if (dentry->d_inode)
-printk("nfs_proc_symlink: %s/%s not negative!\n",
-dentry->d_parent->d_name.name, dentry->d_name.name);
-#endif
 	/*
 	 * Fill in the sattr for the call.
  	 * Note: SunOS 4.1.2 crashes if the mode isn't initialized!

@@ -1584,16 +1574,8 @@ static int nfs_rename(struct inode *old_
 			new_inode = NULL;
 			/* instantiate the replacement target */
 			d_instantiate(new_dentry, NULL);
-		} else if (atomic_read(&new_dentry->d_count) > 1) {
-		/* dentry still busy? */
-#ifdef NFS_PARANOIA
-			printk("nfs_rename: target %s/%s busy, d_count=%d\n",
-			       new_dentry->d_parent->d_name.name,
-			       new_dentry->d_name.name,
-			       atomic_read(&new_dentry->d_count));
-#endif
+		} else if (atomic_read(&new_dentry->d_count) > 1)
 			goto out;
-		}
 	} else
 		new_inode->i_nlink--;

fs/nfs/inode.c +1 ~~-10~~

diff -upr linux-2.6.18-rc7-git2-orig/fs/nfs/inode.c linux-2.6.18-rc7-git2/fs/nfs/inode.c
@@ -48,7 +48,6 @@
 #include "internal.h"
 
 #define NFSDBG_FACILITY		NFSDBG_VFS
-#define NFS_PARANOIA 1
 
 static void nfs_invalidate_inode(struct inode *);
 static int nfs_update_inode(struct inode *, struct nfs_fattr *);

@@ -895,7 +894,7 @@ static int nfs_update_inode(struct inode
 	 * Make sure the inode's type hasn't changed.
 	 */
 	if ((inode->i_mode & S_IFMT) != (fattr->mode & S_IFMT))
-		goto out_changed;
+		goto out_err;
 
 	server = NFS_SERVER(inode);
 	/* Update the fsid if and only if this is the root directory */

@@ -1005,14 +1004,6 @@ static int nfs_update_inode(struct inode
 		nfsi->cache_validity |= invalid;
 
 	return 0;
- out_changed:
-	/*
-	 * Big trouble! The inode has become a different object.
-	 */
-#ifdef NFS_PARANOIA
-	printk(KERN_DEBUG "%s: inode %ld mode changed, %07o to %07o\n",
-			__FUNCTION__, inode->i_ino, inode->i_mode, fattr->mode);
-#endif
  out_err:
 	/*
 	 * No need to worry about unhashing the dentry, as the

fs/nfs/nfs2xdr.c +0 -1

diff -upr linux-2.6.18-rc7-git2-orig/fs/nfs/nfs2xdr.c linux-2.6.18-rc7-git2/fs/nfs/nfs2xdr.c
@@ -26,7 +26,6 @@
 #include "internal.h"
 
 #define NFSDBG_FACILITY		NFSDBG_XDR
-/* #define NFS_PARANOIA 1 */
 
 /* Mapping from NFS error code to "errno" error code. */
 #define errno_NFSERR_IO		EIO

fs/nfs/pagelist.c +2 -5

diff -upr linux-2.6.18-rc7-git2-orig/fs/nfs/pagelist.c linux-2.6.18-rc7-git2/fs/nfs/pagelist.c
@@ -18,7 +18,6 @@
 #include <linux/nfs_fs.h>
 #include <linux/nfs_mount.h>
 
-#define NFS_PARANOIA 1
 
 static kmem_cache_t *nfs_page_cachep;

@@ -171,10 +170,8 @@ nfs_release_request(struct nfs_page *req
 	if (!atomic_dec_and_test(&req->wb_count))
 		return;
 
-#ifdef NFS_PARANOIA
-	BUG_ON (!list_empty(&req->wb_list));
-	BUG_ON (NFS_WBACK_BUSY(req));
-#endif
+	BUG_ON(!list_empty(&req->wb_list));
+	BUG_ON(NFS_WBACK_BUSY(req));
 
 	/* Release struct file or cached credential */
 	nfs_clear_request(req);

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs