[RFC: 2.6 patch] include/linux/nfsd/nfsfh.h: fix a NULL

[RFC: 2.6 patch] include/linux/nfsd/nfsfh.h: fix a NULL

[Adrian Bunk]

dereference
Reply-To: 
Fcc: =sent-mail

When we know fhp->fh_dentry is NULL, a code path where it's being 
dereferenced isn't a good choice.

Spotted by the coverity checker.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6/include/linux/nfsd/nfsfh.h.old	2006-11-12 14:13:34.000000000 +0100
+++ linux-2.6/include/linux/nfsd/nfsfh.h	2006-11-12 14:13:49.000000000 +0100
@@ -330,8 +330,7 @@ fh_unlock(struct svc_fh *fhp)
 {
 	if (!fhp->fh_dentry)
 		printk(KERN_ERR "fh_unlock: fh not verified!\n");
-
-	if (fhp->fh_locked) {
+	else if (fhp->fh_locked) {
 		fill_post_wcc(fhp);
 		mutex_unlock(&fhp->fh_dentry->d_inode->i_mutex);
 		fhp->fh_locked = 0;

Re: [NFS] [RFC: 2.6 patch] include/linux/nfsd/nfsfh.h: fix a NULL

[Trond Myklebust]

On Sun, 2006-11-12 at 14:17 +0100, Adrian Bunk wrote:
> dereference
> Reply-To: 
> Fcc: =sent-mail
> 
> When we know fhp->fh_dentry is NULL, a code path where it's being 
> dereferenced isn't a good choice.
> 
> Spotted by the coverity checker.
> 
> Signed-off-by: Adrian Bunk <bunk@stusta.de>
> 
> --- linux-2.6/include/linux/nfsd/nfsfh.h.old	2006-11-12 14:13:34.000000000 +0100
> +++ linux-2.6/include/linux/nfsd/nfsfh.h	2006-11-12 14:13:49.000000000 +0100
> @@ -330,8 +330,7 @@ fh_unlock(struct svc_fh *fhp)
>  {
>  	if (!fhp->fh_dentry)
>  		printk(KERN_ERR "fh_unlock: fh not verified!\n");
> -
> -	if (fhp->fh_locked) {
> +	else if (fhp->fh_locked) {
>  		fill_post_wcc(fhp);
>  		mutex_unlock(&fhp->fh_dentry->d_inode->i_mutex);
>  		fhp->fh_locked = 0;
> 

This issue has come up on lkml before. Please just convert that check
for fhp->fh_dentry into a BUG_ON().

Trond

[2.6 patch] include/linux/nfsd/nfsfh.h:fh_unlock(): change an error to a BUG_ON()

[Adrian Bunk]

On Sun, Nov 12, 2006 at 10:58:47AM -0500, Trond Myklebust wrote:
> On Sun, 2006-11-12 at 14:17 +0100, Adrian Bunk wrote:
> > dereference
> > Reply-To: 
> > Fcc: =sent-mail
> > 
> > When we know fhp->fh_dentry is NULL, a code path where it's being 
> > dereferenced isn't a good choice.
> > 
> > Spotted by the coverity checker.
> > 
> > Signed-off-by: Adrian Bunk <bunk@stusta.de>
> > 
> > --- linux-2.6/include/linux/nfsd/nfsfh.h.old	2006-11-12 14:13:34.000000000 +0100
> > +++ linux-2.6/include/linux/nfsd/nfsfh.h	2006-11-12 14:13:49.000000000 +0100
> > @@ -330,8 +330,7 @@ fh_unlock(struct svc_fh *fhp)
> >  {
> >  	if (!fhp->fh_dentry)
> >  		printk(KERN_ERR "fh_unlock: fh not verified!\n");
> > -
> > -	if (fhp->fh_locked) {
> > +	else if (fhp->fh_locked) {
> >  		fill_post_wcc(fhp);
> >  		mutex_unlock(&fhp->fh_dentry->d_inode->i_mutex);
> >  		fhp->fh_locked = 0;
> > 
> 
> This issue has come up on lkml before. Please just convert that check
> for fhp->fh_dentry into a BUG_ON().

Patch below.

> Trond

cu
Adrian


<--  snip  -->


!fhp->fh_dentry is a condition that mustn't ever happen (and we might 
run into additional trouble later).

Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6/include/linux/nfsd/nfsfh.h.old	2006-11-12 18:37:43.000000000 +0100
+++ linux-2.6/include/linux/nfsd/nfsfh.h	2006-11-12 18:38:03.000000000 +0100
@@ -328,8 +328,7 @@
 static inline void
 fh_unlock(struct svc_fh *fhp)
 {
-	if (!fhp->fh_dentry)
-		printk(KERN_ERR "fh_unlock: fh not verified!\n");
+	BUG_ON(!fhp->fh_dentry);
 
 	if (fhp->fh_locked) {
 		fill_post_wcc(fhp);

Re: [2.6 patch] include/linux/nfsd/nfsfh.h:fh_unlock(): change an error to a BUG_ON()

[Neil Brown]

On Sunday November 12, bunk@stusta.de wrote:
> > 
> > This issue has come up on lkml before. Please just convert that check
> > for fhp->fh_dentry into a BUG_ON().
> 
> Patch below.
> 

Thanks.  I might that the opportunity to do a bit more cleaning up
here and suggest this version .

NeilBrown

---------------------------
Subject: Replace some warning ins nfsfh.h with BUG_ON or WARN_ON

A couple of the warning will be followed by an Oops if they ever fire,
so may as well be BUG_ON.  Another isn't obviously fatal but has never
been known to fire, so make it a WARN_ON.

Cc: Adrian Bunk <bunk@stusta.de>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./include/linux/nfsd/nfsfh.h |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff .prev/include/linux/nfsd/nfsfh.h ./include/linux/nfsd/nfsfh.h
--- .prev/include/linux/nfsd/nfsfh.h	2006-11-13 15:44:24.000000000 +1100
+++ ./include/linux/nfsd/nfsfh.h	2006-11-13 15:47:02.000000000 +1100
@@ -217,11 +217,7 @@ void	fh_put(struct svc_fh *);
 static __inline__ struct svc_fh *
 fh_copy(struct svc_fh *dst, struct svc_fh *src)
 {
-	if (src->fh_dentry || src->fh_locked) {
-		struct dentry *dentry = src->fh_dentry;
-		printk(KERN_ERR "fh_copy: copying %s/%s, already verified!\n",
-			dentry->d_parent->d_name.name, dentry->d_name.name);
-	}
+	WARN_ON(src->fh_dentry || src->fh_locked);
 			
 	*dst = *src;
 	return dst;
@@ -300,10 +296,8 @@ fh_lock_nested(struct svc_fh *fhp, unsig
 	dfprintk(FILEOP, "nfsd: fh_lock(%s) locked = %d\n",
 			SVCFH_fmt(fhp), fhp->fh_locked);
 
-	if (!fhp->fh_dentry) {
-		printk(KERN_ERR "fh_lock: fh not verified!\n");
-		return;
-	}
+	BUG_ON(!dentry);
+
 	if (fhp->fh_locked) {
 		printk(KERN_WARNING "fh_lock: %s/%s already locked!\n",
 			dentry->d_parent->d_name.name, dentry->d_name.name);
@@ -328,8 +322,7 @@ fh_lock(struct svc_fh *fhp)
 static inline void
 fh_unlock(struct svc_fh *fhp)
 {
-	if (!fhp->fh_dentry)
-		printk(KERN_ERR "fh_unlock: fh not verified!\n");
+	BUG_ON(!fhp->fh_dentry);
 
 	if (fhp->fh_locked) {
 		fill_post_wcc(fhp);