Skip to contentSkip to conversation

GFS2: Fix potential NULL dereference in gfs2_alloc_inode

PATCH New Andrew Price 4 participants 6 messages Mar 2, 2015 – Mar 3, 2015 GFS2 file system

lore
Share & tools
lore

Andrew Price

author
#0 lore raw

PATCH 1/1 GFS2: Fix potential NULL dereference in gfs2_alloc_inode

Return NULL when ip is NULL instead of dereferencing it.

Signed-off-by Andrew Price <[email protected]>
---
 fs/gfs2/super.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
fs/gfs2/super.c +7 -6
index 1666382..37c59ee 100644
@@ -1628,12 +1628,13 @@ static struct inode *gfs2_alloc_inode(struct super_block *sb)
 	struct gfs2_inode *ip;
 
 	ip = kmem_cache_alloc(gfs2_inode_cachep, GFP_KERNEL);
-	if (ip) {
-		ip->i_flags = 0;
-		ip->i_gl = NULL;
-		ip->i_rgd = NULL;
-		ip->i_res = NULL;
-	}
+	if (!ip)
+		return NULL;
+
+	ip->i_flags = 0;
+	ip->i_gl = NULL;
+	ip->i_rgd = NULL;
+	ip->i_res = NULL;
 	return &ip->i_inode;
 }
Signature · 1 line
1.9.3
#1 lore raw

PATCH 1/1 GFS2: Fix potential NULL dereference in gfs2_alloc_inode

Hi,

Quoted 21 lines · Andrew Price

Return NULL when ip is NULL instead of dereferencing it.

Signed-off-by Andrew Price <[email protected]>
---
  fs/gfs2/super.c | 13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)
fs/gfs2/super.c +0 -6
index 1666382..37c59ee 100644
@@ -1628,12 +1628,13 @@ static struct inode *gfs2_alloc_inode(struct super_block *sb)
  	struct gfs2_inode *ip;
 
  	ip = kmem_cache_alloc(gfs2_inode_cachep, GFP_KERNEL);
-	if (ip) {
-		ip->i_flags = 0;
-		ip->i_gl = NULL;
-		ip->i_rgd = NULL;
-		ip->i_res = NULL;
-	}

fs/gfs2/super.c static struct inode *gfs2_alloc_inode(struct super_block *sb) · line 1640

Quoted 10 lines
fs/gfs2/super.c +7 -0
+	if (!ip)
+		return NULL;
+
+	ip->i_flags = 0;
+	ip->i_gl = NULL;
+	ip->i_rgd = NULL;
+	ip->i_res = NULL;
  	return &ip->i_inode;
  }

I'm not sure that I see the problem here... it should just return NULL if ip is NULL, since ip->i_inode is the first element of ip,

Steve.

author
#2 lore raw

PATCH 1/1 GFS2: Fix potential NULL dereference in gfs2_alloc_inode

Quoted 28 lines · Steven Whitehouse

Hi,

Quoted 25 lines · Andrew Price

Return NULL when ip is NULL instead of dereferencing it.

Signed-off-by Andrew Price <[email protected]>
---
  fs/gfs2/super.c | 13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)
fs/gfs2/super.c +4 -6
index 1666382..37c59ee 100644
@@ -1628,12 +1628,13 @@ static struct inode *gfs2_alloc_inode(struct
 super_block *sb)
      struct gfs2_inode *ip;
      ip = kmem_cache_alloc(gfs2_inode_cachep, GFP_KERNEL);
-    if (ip) {
-        ip->i_flags = 0;
-        ip->i_gl = NULL;
-        ip->i_rgd = NULL;
-        ip->i_res = NULL;
-    }
+    if (!ip)
+        return NULL;
+
+    ip->i_flags = 0;
Quoted 8 lines

fs/gfs2/super.c static struct inode *gfs2_alloc_inode(struct super_block *sb) · line 1639

Quoted 5 lines
fs/gfs2/super.c +3 -0
+    ip->i_gl = NULL;
+    ip->i_rgd = NULL;
+    ip->i_res = NULL;
      return &ip->i_inode;
  }

I'm not sure that I see the problem here... it should just return NULL if ip is NULL, since ip->i_inode is the first element of ip,

Ah, so it is. Self-NACK then.

Andy

maintainer
#3 lore raw

PATCH 1/1 GFS2: Fix potential NULL dereference in gfs2_alloc_inode

Quoted 5 lines · Andrew Price
Quoted 3 lines · Steven Whitehouse

Hi,

On 02/03/15 16:15, Andrew Price wrote:

Ah, so it is. Self-NACK then.

The patch itself is still worth it though, it's not self-evident ip and &ip->i_inode are the same.

Andreas

author
#4 lore raw

PATCH 1/1 GFS2: Improve readability of gfs2_alloc_inode

Returning &ip->i_inode when ip is NULL is safe as i_inode is the first member in struct gfs2_inode, but that's not immediately obvious. Reorganize gfs2_alloc_inode to avoid any doubt.

Signed-off-by Andrew Price <[email protected]>

---

Re-sending with a more appropriate commit log based on Andreas' comments.

 fs/gfs2/super.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
fs/gfs2/super.c +7 -6
index 1666382..37c59ee 100644
@@ -1628,12 +1628,13 @@ static struct inode *gfs2_alloc_inode(struct super_block *sb)
 	struct gfs2_inode *ip;
 
 	ip = kmem_cache_alloc(gfs2_inode_cachep, GFP_KERNEL);
-	if (ip) {
-		ip->i_flags = 0;
-		ip->i_gl = NULL;
-		ip->i_rgd = NULL;
-		ip->i_res = NULL;
-	}
+	if (!ip)
+		return NULL;
+
+	ip->i_flags = 0;
+	ip->i_gl = NULL;
+	ip->i_rgd = NULL;
+	ip->i_res = NULL;
 	return &ip->i_inode;
 }
Signature · 1 line
1.9.3
#5 lore raw

PATCH 1/1 GFS2: Improve readability of gfs2_alloc_inode

----- Original Message -----

Quoted 38 lines

Returning &ip->i_inode when ip is NULL is safe as i_inode is the first member in struct gfs2_inode, but that's not immediately obvious. Reorganize gfs2_alloc_inode to avoid any doubt.

Signed-off-by Andrew Price <[email protected]>

---

Re-sending with a more appropriate commit log based on Andreas' comments.

 fs/gfs2/super.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
fs/gfs2/super.c +7 -7
index 1666382..37c59ee 100644
@@ -1628,12 +1628,13 @@ static struct inode *gfs2_alloc_inode(struct
 super_block *sb)
 	struct gfs2_inode *ip;
 
 	ip = kmem_cache_alloc(gfs2_inode_cachep, GFP_KERNEL);
-	if (ip) {
-		ip->i_flags = 0;
-		ip->i_gl = NULL;
-		ip->i_rgd = NULL;
-		ip->i_res = NULL;
-	}
+	if (!ip)
+		return NULL;
+
+	ip->i_flags = 0;
+	ip->i_gl = NULL;
+	ip->i_rgd = NULL;
+	ip->i_res = NULL;
 	return &ip->i_inode;
 }
 
--
Quoted 1 line

1.9.3

ACK,

Bob Peterson
Red Hat File Systems