2015-08-11 17:14:07

by Jeff Layton

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

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 <[email protected]>
---
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

2015-08-11 17:32:23

by Anna Schumaker

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

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 <[email protected]>
> ---
> 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)
>

2015-08-11 18:45:31

by Jeff Layton

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

On Tue, 11 Aug 2015 13:32:05 -0400
Anna Schumaker <[email protected]> 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 <[email protected]>
> > ---
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


--
Jeff Layton <[email protected]>