Re: [PATCH] xfs_repair: do not check symlink component lengths

[Brian Foster <bfoster@redhat.com>]

On Fri, Dec 05, 2014 at 02:30:14PM -0600, Eric Sandeen wrote:
> As reported by Andy Grimm,
> 
> # ln -s $( python -c 'print "a" * 260' ) /mnt/foo
> 
> will succeed on xfs, but then xfs_repair will complain:
> 
> component of symlink in inode 131 too long
> problem with symbolic link in inode 131
> would have cleared inode 131
> 
> The kernel checks the total length of the symlink on both read
> and write, but does not look at component paths.
> 
> Looking around the kernel, no other filesystem checks component
> lengths, nor does the vfs.  And as Andy points out, the target
> could even be on a different filesystem, with different limitations.
> 

Interesting, but we do enforce dentry name limits, yes? At least, I
can't create such a component on an XFS fs (haven't dug into where that
is enforced).

> And having a "too-long" component doesn't even seem like something
> likely to stem from disk corruption anyway, so I'm not sure why repair
> should care.
> 

My guess would be the intent is based on the above, where we can't
create such a long component name and thus a component that exceeds the
limits must be "invalid."

That said, a broken symlink is generally valid and not the same as
corruption, so I think I agree that this is somewhat misplaced
functionality...

> Therefore I propose removing the component length checks from xfs_repair.
> 
> Andy Grimm <agrimm@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 38a6562..73e4b9e 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -1333,7 +1333,7 @@ process_symlink(
>  	xfs_dinode_t	*dino,
>  	blkmap_t 	*blkmap)
>  {
> -	char			*symlink, *cptr;
> +	char			*symlink;
>  	char			data[MAXPATHLEN];
>  
>  	/*
> @@ -1380,31 +1380,6 @@ _("found illegal null character in symlink inode %" PRIu64 "\n"),
>  		return(1);
>  	}
>  
> -	/*
> -	 * check for any component being too long
> -	 */
> -	if (be64_to_cpu(dino->di_size) >= MAXNAMELEN)  {
> -		cptr = strchr(symlink, '/');
> -
> -		while (cptr != NULL)  {
> -			if (cptr - symlink >= MAXNAMELEN)  {
> -				do_warn(
> -_("component of symlink in inode %" PRIu64 " too long\n"),
> -					lino);
> -				return(1);
> -			}
> -			symlink = cptr + 1;
> -			cptr = strchr(symlink, '/');
> -		}
> -
> -		if (strlen(symlink) >= MAXNAMELEN)  {
> -			do_warn(
> -_("component of symlink in inode %" PRIu64 " too long\n"),
> -				lino);
> -			return(1);
> -		}
> -	}
> -
>  	return(0);
>  }
>  
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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