[PATCH] xfs: Fix file type directory corruption for btree directories

[PATCH] xfs: Fix file type directory corruption for btree directories

[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.

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

Reported-by: Giacomo Comes <comes@naic.edu>
Signed-off-by: Jan Kara <jack@suse.com>
---
 fs/xfs/libxfs/xfs_dir2_node.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
index 41b80d3d3877..1006710a7c92 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -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;
 	}
-- 
2.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Fix file type directory corruption for btree directories

[Jan Kara]

On Fri 21-08-15 19:55:22, Jan Kara wrote:
> 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 <comes@naic.edu>
> Signed-off-by: Jan Kara <jack@suse.com>

BTW, this is a stable material. Forgot to add CC...

								Honza

> ---
>  fs/xfs/libxfs/xfs_dir2_node.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> index 41b80d3d3877..1006710a7c92 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -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;
>  	}
> -- 
> 2.1.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Fix file type directory corruption for btree directories

[Dave Chinner]

On Fri, Aug 21, 2015 at 07:55:22PM +0200, Jan Kara wrote:
> 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 <comes@naic.edu>
> Signed-off-by: Jan Kara <jack@suse.com>

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?

i.e. I'm looking for something that can be put into a regression
test, because vlealry we don't cover this case right now....

> ---
>  fs/xfs/libxfs/xfs_dir2_node.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> index 41b80d3d3877..1006710a7c92 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -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;
>  	}

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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Fix file type directory corruption for btree directories

[Jan Kara]

On Sat 22-08-15 09:11:54, Dave Chinner wrote:
> On Fri, Aug 21, 2015 at 07:55:22PM +0200, Jan Kara wrote:
> > 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 <comes@naic.edu>
> > Signed-off-by: Jan Kara <jack@suse.com>
> 
> 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.

								Honza

> > ---
> >  fs/xfs/libxfs/xfs_dir2_node.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> > index 41b80d3d3877..1006710a7c92 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_node.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> > @@ -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;
> >  	}
> 
> 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.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: Fix file type directory corruption for btree directories

[Dave Chinner]

On Mon, Aug 24, 2015 at 11:26:37AM +0200, Jan Kara wrote:
> On Sat 22-08-15 09:11:54, Dave Chinner wrote:
> > On Fri, Aug 21, 2015 at 07:55:22PM +0200, Jan Kara wrote:
> > > 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 <comes@naic.edu>
> > > Signed-off-by: Jan Kara <jack@suse.com>
> > 
> > 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.

I don't think there's anything specific that tests ftype after
rename. It might be worth writing a specific test for it that steps
through different directory formats and checks that ftype swaps with
rename corectly as we move through shortform, block, leaf and node
formats (i.e. as the directory gets larger).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs