xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfs_repair: do not check symlink component lengths
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Thu, 18 Dec 2014 10:12:37 -0500
Cc: xfs-oss <xfs@xxxxxxxxxxx>, Andy Grimm <agrimm@xxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <548215D6.5020804@xxxxxxxxxxx>
References: <548215D6.5020804@xxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
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@xxxxxxxxxx>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

> 
> 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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>