xfsprogs: fix inode crash in xfs_repair

Adding the lost+found in phase 6 could allocate an inode from a new inode chunk. That newly created chunk was not around in the scan phase, and is not in the avl tree which will result in a NULL dereference.

This patch adds the newly created inode chunk and inodes as if found in the scan phase.

Metadata dump available for future tests.

Signed-off-by Mark Tinguely <[email protected]>

---
 repair/incore_ino.c |    2 +-
 repair/phase6.c     |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

Index: b/repair/incore_ino.c
===================================================================

repair/incore_ino.c +1 -1

@@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec,
 	return(0LL);
 }
 
-static void
+void
 alloc_ex_data(ino_tree_node_t *irec)
 {
 	parent_list_t 	*ptbl;

Index: b/repair/phase6.c
===================================================================

Quoted 34 lines · Mark Tinguely

Adding the lost+found in phase 6 could allocate an inode from a new inode chunk. That newly created chunk was not around in the scan phase, and is not in the avl tree which will result in a NULL dereference.

This patch adds the newly created inode chunk and inodes as if found in the scan phase.

Metadata dump available for future tests.

Signed-off-by Mark Tinguely<[email protected]>

---
  repair/incore_ino.c |    2 +-
  repair/phase6.c     |   15 +++++++++++++++
  2 files changed, 16 insertions(+), 1 deletion(-)

Index: b/repair/incore_ino.c
===================================================================

repair/incore_ino.c +1 -1

@@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec,
  	return(0LL);
  }
 
-static void
+void
  alloc_ex_data(ino_tree_node_t *irec)
  {
  	parent_list_t 	*ptbl;

Index: b/repair/phase6.c
===================================================================

repair/phase6.c mk_orphanage(xfs_mount_t *mp) · line 935

Quoted 6 lines

repair/phase6.c +3 -0

@@ -930,6 +930,21 @@ mk_orphanage(xfs_mount_t *mp)
  	irec = find_inode_rec(mp,
  			XFS_INO_TO_AGNO(mp, ino),
  			XFS_INO_TO_AGINO(mp, ino));
+
+	if (irec == NULL&&  XFS_INO_TO_AGNO(mp, ino)<  mp->m_sb.sb_agcount&&
+	    ip != NULL&&  ip->i_d.di_magic == XFS_DINODE_MAGIC) {

I don't understand this check.

We've already dereferenced ip several lines above to increment the link count and get the inode number stored in ino, so the ip != NULL is unnecessary.

We've just allocated the inode, so why would the magic number be wrong? And why would the inode number lie in a non-existent allocation group?

repair/phase6.c mk_orphanage(xfs_mount_t *mp) · line 939

Quoted 3 lines

repair/phase6.c +3 -0

+		/*
+		 * add the newly allocated inode chunk to the avl tree.
+		 */

I can see from the code we are allocating and irec, inserting it into the AVL tree and marking all the inodes in the chunk as free. The comment should explain *why* we need to do this.

Cheers,

Dave.

Quoted 25 lines · Dave Chinner

Quoted 24 lines · Mark Tinguely

Adding the lost+found in phase 6 could allocate an inode from a new inode chunk. That newly created chunk was not around in the scan phase, and is not in the avl tree which will result in a NULL dereference.

This patch adds the newly created inode chunk and inodes as if found in the scan phase.

Metadata dump available for future tests.

Signed-off-by Mark Tinguely<[email protected]>

---
 repair/incore_ino.c |    2 +-
 repair/phase6.c     |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

Index: b/repair/incore_ino.c
===================================================================

repair/incore_ino.c +0 -0

@@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec,
 	return(0LL);
 }
 

Quoted 8 lines

Quoted 8 lines

repair/incore_ino.c +1 -1

-static void
+void
 alloc_ex_data(ino_tree_node_t *irec)
 {
 	parent_list_t 	*ptbl;

Index: b/repair/phase6.c
===================================================================
--- a/repair/phase6.c

Quoted 8 lines

repair/incore_ino.c mk_orphanage(xfs_mount_t *mp) · line 935

Quoted 8 lines

+++ b/repair/phase6.c

repair/incore_ino.c +3 -0

@@ -930,6 +930,21 @@ mk_orphanage(xfs_mount_t *mp)
 	irec = find_inode_rec(mp,
 			XFS_INO_TO_AGNO(mp, ino),
 			XFS_INO_TO_AGINO(mp, ino));
+
+	if (irec == NULL&&  XFS_INO_TO_AGNO(mp, ino)<  mp->m_sb.sb_agcount&&
+	    ip != NULL&&  ip->i_d.di_magic == XFS_DINODE_MAGIC) {

Quoted 6 lines

I don't understand this check.

We've already dereferenced ip several lines above to increment the link count and get the inode number stored in ino, so the ip != NULL is unnecessary.

Quoted 4 lines

We've just allocated the inode, so why would the magic number be wrong? And why would the inode number lie in a non-existent allocation group?

just being being paranoid.

Quoted 37 lines · Mark Tinguely

Quoted 36 lines · Dave Chinner

Quoted 35 lines · Mark Tinguely

Adding the lost+found in phase 6 could allocate an inode from a new inode chunk. That newly created chunk was not around in the scan phase, and is not in the avl tree which will result in a NULL dereference.

This patch adds the newly created inode chunk and inodes as if found in the scan phase.

Metadata dump available for future tests.

Signed-off-by Mark Tinguely<[email protected]>

---
  repair/incore_ino.c |    2 +-
  repair/phase6.c     |   15 +++++++++++++++
  2 files changed, 16 insertions(+), 1 deletion(-)

Index: b/repair/incore_ino.c
===================================================================

repair/incore_ino.c +1 -1

@@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec,
  	return(0LL);
  }
 
-static void
+void
  alloc_ex_data(ino_tree_node_t *irec)
  {
  	parent_list_t 	*ptbl;

Index: b/repair/phase6.c
===================================================================

repair/phase6.c +0 -0

@@ -930,6 +930,21 @@ mk_orphanage(xfs_mount_t *mp)
  	irec = find_inode_rec(mp,

Quoted 5 lines

Quoted 5 lines

repair/phase6.c mk_orphanage(xfs_mount_t *mp)

Quoted 5 lines

repair/phase6.c +3 -0

  			XFS_INO_TO_AGNO(mp, ino),
  			XFS_INO_TO_AGINO(mp, ino));
+
+	if (irec == NULL&&   XFS_INO_TO_AGNO(mp, ino)<   mp->m_sb.sb_agcount&&
+	    ip != NULL&&   ip->i_d.di_magic == XFS_DINODE_MAGIC) {

BTW, Mark, you're mailer is doing weird things to whitespace in code when it's quoting quoted code.

Quoted 7 lines

Quoted 7 lines

I don't understand this check.

We've already dereferenced ip several lines above to increment the link count and get the inode number stored in ino, so the ip != NULL is unnecessary.

Quoted 6 lines

Quoted 4 lines

We've just allocated the inode, so why would the magic number be wrong? And why would the inode number lie in a non-existent allocation group?

just being being paranoid.

It's the same code as in the kernel - if is that broken that it can't tell it didn't allocate a real inode, then we've got bigger problems. We design the code to return an error when it fails so we don't have to robustly check every possible error condition at every call site. So really the only check needed is "if (!irec) {...}"

Cheers,

Dave.

Quoted 14 lines · Dave Chinner

Quoted 12 lines · Mark Tinguely

Quoted 10 lines

I don't understand this check.

We've already dereferenced ip several lines above to increment the link count and get the inode number stored in ino, so the ip != NULL is unnecessary.

We've just allocated the inode, so why would the magic number be wrong? And why would the inode number lie in a non-existent allocation group?

just being being paranoid.

Quoted 5 lines

It's the same code as in the kernel - if is that broken that it can't tell it didn't allocate a real inode, then we've got bigger problems. We design the code to return an error when it fails so we don't have to robustly check every possible error condition at every call site. So really the only check needed is "if (!irec) {...}"

The inode number check came from find_inode_rec(ag, agino) (repair/incore.h). A bad inode number will also return a NULL irec.