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

[Jan Kara <jack@suse.cz>]

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