[PATCH] nfs: allow nfs4_label_free to ignore a PTR_ERR value

[PATCH] nfs: allow nfs4_label_free to ignore a PTR_ERR value

[Jeff Layton]

nfs4_label_alloc is a little odd in that a NULL return means "label is
not needed" and a PTR_ERR return means an actual error. In at least one
place however (nfs_lookup_revalidate) we can end up passing an error
value to nfs4_label_free, which will likely lead to an oops.

We could fix that one caller, but I think just allowing the free to
accept and ignore PTR_ERR values is probably appropriate given how the
allocation works.

Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/nfs/internal.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9b372b845f6a..44bc298f6216 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -312,13 +312,13 @@ nfs4_label_copy(struct nfs4_label *dst, struct nfs4_label *src)
 
 	return dst;
 }
+
 static inline void nfs4_label_free(struct nfs4_label *label)
 {
-	if (label) {
-		kfree(label->label);
-		kfree(label);
-	}
-	return;
+	if (IS_ERR_OR_NULL(label))
+		return;
+	kfree(label->label);
+	kfree(label);
 }
 
 static inline void nfs_zap_label_cache_locked(struct nfs_inode *nfsi)
-- 
2.4.3

Re: [PATCH] nfs: allow nfs4_label_free to ignore a PTR_ERR value

[Anna Schumaker]

Hi Jeff,


On 08/11/2015 01:13 PM, Jeff Layton wrote:
> nfs4_label_alloc is a little odd in that a NULL return means "label is
> not needed" and a PTR_ERR return means an actual error. In at least one
> place however (nfs_lookup_revalidate) we can end up passing an error
> value to nfs4_label_free, which will likely lead to an oops.
> 
> We could fix that one caller, but I think just allowing the free to
> accept and ignore PTR_ERR values is probably appropriate given how the
> allocation works.

This makes sense, but I wonder if there are other functions that may-or-may-not allocate a structure without it being considered an error?  Does nfs4_label_alloc() match what other functions do?

Thanks,
Anna

> 
> Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> ---
>  fs/nfs/internal.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 9b372b845f6a..44bc298f6216 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -312,13 +312,13 @@ nfs4_label_copy(struct nfs4_label *dst, struct nfs4_label *src)
>  
>  	return dst;
>  }
> +
>  static inline void nfs4_label_free(struct nfs4_label *label)
>  {
> -	if (label) {
> -		kfree(label->label);
> -		kfree(label);
> -	}
> -	return;
> +	if (IS_ERR_OR_NULL(label))
> +		return;
> +	kfree(label->label);
> +	kfree(label);
>  }
>  
>  static inline void nfs_zap_label_cache_locked(struct nfs_inode *nfsi)
> 

Re: [PATCH] nfs: allow nfs4_label_free to ignore a PTR_ERR value

[Jeff Layton]

On Tue, 11 Aug 2015 13:32:05 -0400
Anna Schumaker <Anna.Schumaker@netapp.com> wrote:

> Hi Jeff,
> 
> 
> On 08/11/2015 01:13 PM, Jeff Layton wrote:
> > nfs4_label_alloc is a little odd in that a NULL return means "label is
> > not needed" and a PTR_ERR return means an actual error. In at least one
> > place however (nfs_lookup_revalidate) we can end up passing an error
> > value to nfs4_label_free, which will likely lead to an oops.
> > 
> > We could fix that one caller, but I think just allowing the free to
> > accept and ignore PTR_ERR values is probably appropriate given how the
> > allocation works.
> 
> This makes sense, but I wonder if there are other functions that may-or-may-not allocate a structure without it being considered an error?  Does nfs4_label_alloc() match what other functions do?
> 
> Thanks,
> Anna
> 

AFAICT, label allocation is really a special case, but I didn't do a
survey of the code to look for similar idioms.

> > 
> > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> > ---
> >  fs/nfs/internal.h | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > index 9b372b845f6a..44bc298f6216 100644
> > --- a/fs/nfs/internal.h
> > +++ b/fs/nfs/internal.h
> > @@ -312,13 +312,13 @@ nfs4_label_copy(struct nfs4_label *dst, struct nfs4_label *src)
> >  
> >  	return dst;
> >  }
> > +
> >  static inline void nfs4_label_free(struct nfs4_label *label)
> >  {
> > -	if (label) {
> > -		kfree(label->label);
> > -		kfree(label);
> > -	}
> > -	return;
> > +	if (IS_ERR_OR_NULL(label))
> > +		return;
> > +	kfree(label->label);
> > +	kfree(label);
> >  }
> >  
> >  static inline void nfs_zap_label_cache_locked(struct nfs_inode *nfsi)
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@poochiereds.net>