xfs: Fix file type directory corruption for btree directories

Users have occasionally reported that file type for some directory entries is wrong. This mostly happened after updating libraries some libraries. After some debugging the problem was traced down to xfs_dir2_node_replace(). The function uses args->filetype as a file type to store in the replaced directory entry however it also calls xfs_da3_node_lookup_int() which will store file type of the current directory entry in args->filetype. Thus we fail to change file type of a directory entry to a proper type.

Fix the problem by storing new file type in a local variable before calling xfs_da3_node_lookup_int().

Reported-by Giacomo Comes <[email protected]>

Signed-off-by Jan Kara <[email protected]>

---
 fs/xfs/libxfs/xfs_dir2_node.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/xfs/libxfs/xfs_dir2_node.c +7 -1

index 41b80d3d3877..1006710a7c92 100644
@@ -2132,6 +2132,7 @@ xfs_dir2_node_replace(
 	int			error;		/* error return value */
 	int			i;		/* btree level */
 	xfs_ino_t		inum;		/* new inode number */
+	int			ftype;		/* new file type */
 	xfs_dir2_leaf_t		*leaf;		/* leaf structure */
 	xfs_dir2_leaf_entry_t	*lep;		/* leaf entry being changed */
 	int			rval;		/* internal return value */

@@ -2145,7 +2146,12 @@ xfs_dir2_node_replace(
 	state = xfs_da_state_alloc();
 	state->args = args;
 	state->mp = args->dp->i_mount;
+	/*
+	 * We have to save new inode number and ftype since
+	 * xfs_da3_node_lookup_int() is going to overwrite them
+	 */
 	inum = args->inumber;
+	ftype = args->filetype;
 	/*
 	 * Lookup the entry to change in the btree.
 	 */

@@ -2183,7 +2189,7 @@ xfs_dir2_node_replace(
 		 * Fill in the new inode number and log the entry.
 		 */
 		dep->inumber = cpu_to_be64(inum);
-		args->dp->d_ops->data_put_ftype(dep, args->filetype);
+		args->dp->d_ops->data_put_ftype(dep, ftype);
 		xfs_dir2_data_log_entry(args, state->extrablk.bp, dep);
 		rval = 0;
 	}

Signature · 1 line

2.1.4

Users have occasionally reported that file type for some directory entries is wrong. This mostly happened after updating libraries some libraries. After some debugging the problem was traced down to xfs_dir2_node_replace(). The function uses args->filetype as a file type to store in the replaced directory entry however it also calls xfs_da3_node_lookup_int() which will store file type of the current directory entry in args->filetype. Thus we fail to change file type of a directory entry to a proper type.

Fix the problem by storing new file type in a local variable before calling xfs_da3_node_lookup_int().

Reported-by Giacomo Comes <[email protected]>

Signed-off-by Jan Kara <[email protected]>

---
 fs/xfs/libxfs/xfs_dir2_node.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/xfs/libxfs/xfs_dir2_node.c +6 -0

index 41b80d3d3877..1006710a7c92 100644
@@ -2132,6 +2132,7 @@ xfs_dir2_node_replace(
 	int			error;		/* error return value */
 	int			i;		/* btree level */
 	xfs_ino_t		inum;		/* new inode number */
+	int			ftype;		/* new file type */
 	xfs_dir2_leaf_t		*leaf;		/* leaf structure */
 	xfs_dir2_leaf_entry_t	*lep;		/* leaf entry being changed */
 	int			rval;		/* internal return value */

@@ -2145,7 +2146,12 @@ xfs_dir2_node_replace(
 	state = xfs_da_state_alloc();
 	state->args = args;
 	state->mp = args->dp->i_mount;
+	/*
+	 * We have to save new inode number and ftype since
+	 * xfs_da3_node_lookup_int() is going to overwrite them
+	 */
 	inum = args->inumber;
+	ftype = args->filetype;
 	/*
 	 * Lookup the entry to change in the btree.
 	 */

Quoted 9 lines · Jan Kara

Users have occasionally reported that file type for some directory entries is wrong. This mostly happened after updating libraries some libraries. After some debugging the problem was traced down to xfs_dir2_node_replace(). The function uses args->filetype as a file type to store in the replaced directory entry however it also calls xfs_da3_node_lookup_int() which will store file type of the current directory entry in args->filetype. Thus we fail to change file type of a directory entry to a proper type.

Quoted 5 lines

Fix the problem by storing new file type in a local variable before calling xfs_da3_node_lookup_int().

Reported-by Giacomo Comes <[email protected]>

Signed-off-by Jan Kara <[email protected]>

So this is being triggered by a rename() operation on a large directory? node format is the optimised form form for large dirs, so I suspect that's why only few people see this. Can you see if you can write a reproducer for it baseed on a large single directory and renaming two files of different types (e.g. BLKDEV over REG) to see if the cause is that simple?

Quoted 30 lines

---
 fs/xfs/libxfs/xfs_dir2_node.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/xfs/libxfs/xfs_dir2_node.c +6 -0

index 41b80d3d3877..1006710a7c92 100644
@@ -2132,6 +2132,7 @@ xfs_dir2_node_replace(
 	int			error;		/* error return value */
 	int			i;		/* btree level */
 	xfs_ino_t		inum;		/* new inode number */
+	int			ftype;		/* new file type */
 	xfs_dir2_leaf_t		*leaf;		/* leaf structure */
 	xfs_dir2_leaf_entry_t	*lep;		/* leaf entry being changed */
 	int			rval;		/* internal return value */

@@ -2145,7 +2146,12 @@ xfs_dir2_node_replace(
 	state = xfs_da_state_alloc();
 	state->args = args;
 	state->mp = args->dp->i_mount;
+	/*
+	 * We have to save new inode number and ftype since
+	 * xfs_da3_node_lookup_int() is going to overwrite them
+	 */
 	inum = args->inumber;
+	ftype = args->filetype;
 	/*
 	 * Lookup the entry to change in the btree.
 	 */

fs/xfs/libxfs/xfs_dir2_node.c xfs_dir2_node_replace( · line 2195

Quoted 8 lines

fs/xfs/libxfs/xfs_dir2_node.c +1 -1

@@ -2183,7 +2189,7 @@ xfs_dir2_node_replace(
 		 * Fill in the new inode number and log the entry.
 		 */
 		dep->inumber = cpu_to_be64(inum);
-		args->dp->d_ops->data_put_ftype(dep, args->filetype);
+		args->dp->d_ops->data_put_ftype(dep, ftype);
 		xfs_dir2_data_log_entry(args, state->extrablk.bp, dep);
 		rval = 0;
 	}

The change looks sane (i'll add whitespace around the comments when i commit) and I'll also add the stable cc. I would like to have a reproducer to test it, though ;)

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 16 lines · Dave Chinner

Quoted 9 lines · Jan Kara

Users have occasionally reported that file type for some directory entries is wrong. This mostly happened after updating libraries some libraries. After some debugging the problem was traced down to xfs_dir2_node_replace(). The function uses args->filetype as a file type to store in the replaced directory entry however it also calls xfs_da3_node_lookup_int() which will store file type of the current directory entry in args->filetype. Thus we fail to change file type of a directory entry to a proper type.

Quoted 5 lines

Fix the problem by storing new file type in a local variable before calling xfs_da3_node_lookup_int().

Reported-by Giacomo Comes <[email protected]>

Signed-off-by Jan Kara <[email protected]>

Quoted 6 lines

So this is being triggered by a rename() operation on a large directory? node format is the optimised form form for large dirs, so I suspect that's why only few people see this. Can you see if you can write a reproducer for it baseed on a large single directory and renaming two files of different types (e.g. BLKDEV over REG) to see if the cause is that simple?

Yes, I've tried and for a large enough directory renaming symlink over a regular file is all that is needed to corrupt the file type in the directory. Should I write a dedicated test for this or is there something that already excercises directory code? I know about fsstress runs but those would be hard to tweak to trigger this I guess.