Skip to contentSkip to conversation

nfs: allow nfs4_label_free to ignore a PTR_ERR value

PATCH In review Jeff Layton 2 participants 3 messages Aug 11, 2015

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Jeff Layton

author
#0 lore raw

PATCH 1/1 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(-)
fs/nfs/internal.h +5 -5
index 9b372b845f6a..44bc298f6216 100644
@@ -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)
Signature · 1 line
2.4.3
#1 lore raw

Hi Jeff,

Quoted 8 lines · 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.

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

Quoted 30 lines
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/internal.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
fs/nfs/internal.h +5 -5
index 9b372b845f6a..44bc298f6216 100644
@@ -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)
author
#2 lore raw
Quoted 9 lines · On Tue, 11 Aug 2015 13:32:05 -0400 Anna Schumaker <[email protected]>…

Hi Jeff,

Quoted 5 lines · 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.

Quoted 9 lines
Quoted 3 lines

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.

Quoted 35 lines
Quoted 20 lines
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/internal.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
fs/nfs/internal.h +1 -2
index 9b372b845f6a..44bc298f6216 100644
@@ -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);

fs/nfs/internal.h nfs4_label_copy(struct nfs4_label *dst, struct nfs4_label *src) · line 324

Quoted 10 lines
fs/nfs/internal.h +4 -3
-		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

Signature · 1 line
Jeff Layton <[email protected]>