xfs
[Top] [All Lists]

Re: [PATCH] metadump: obfuscate symlinks by path component

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH] metadump: obfuscate symlinks by path component
From: Alex Elder <elder@xxxxxxxxxxx>
Date: Fri, 14 Dec 2012 13:55:30 -0600
In-reply-to: <50CA9545.10105@xxxxxxxxxxx>
References: <4F83B844.3060508@xxxxxxxxxx> <50CA9545.10105@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0
On 12/13/2012 08:56 PM, Eric Sandeen wrote:
> On 4/9/12 11:34 PM, Eric Sandeen wrote:
>> xfs_metadump currently obfuscates entire symlinks without regard
>> to path components; this can lead to a corrupt image when restoring
>> a metadump containing extremely long symlinks:
>>
>> Phase 3 - for each AG...
>>         - scan and clear agi unlinked lists...
>>         - process known inodes and perform inode discovery...
>>         - agno = 0
>> component of symlink in inode 145 too long
>> problem with symbolic link in inode 145
>> cleared inode 145
>> ... <more trail of woe>
>>
>> Fix this by consolidating symlink obfuscation into a new
>> function which obfuscates one path component at a time.
>>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> 
> ping? :)

You know, these things take time.

What you have looks good to me, but I rewrote it, below. :)

Even if you don't change anything...

Reviewed-by: Alex Elder <elder@xxxxxxxxxxx>

>> ---
>>
>> Yes, I need to do an xfstest for this.  metadumping a filesystem
>> after fsstressing it works well.
>>
>> diff --git a/db/metadump.c b/db/metadump.c
>> index c5ffddb..9f15d9e 100644
>> --- a/db/metadump.c
>> +++ b/db/metadump.c
>> @@ -956,6 +956,40 @@ obfuscate_sf_dir(
>>  }
>>  
>>  static void
>> +obfuscate_path_components(
>> +    char                    *buf,
>> +    __uint64_t              len)
>> +{
>> +    uchar_t                 *comp;
>> +    xfs_dahash_t            hash;
>> +
>> +    comp = (uchar_t *)buf;
>> +    while (comp < (uchar_t *)buf + len) {
>> +            char    *slash;
>> +            int     namelen;
>> +

Maybe instead of handling it later, you could check for
leading and duplicate slashes here:

                while (*comp == '/')
                        comp++;

In fact, maybe it could be more like:

                while (*comp == '/')
                        comp++;
                slash = strchrnul((char *) comp, '/');
                namlen = slash - comp;
                if (!namelen)
                        return;
                hash = libxfs_da_hashname(comp, namelen);
                obfuscate_name(hash, namelen, comp);
                comp += namelen + 1;

Note that these all assume the original buffer is NUL-terminated
(yours did too).

>> +            /* find slash at end of this component */
>> +            slash = strchr((char *)comp, '/');
>> +            if (!slash) {
>> +                    /* last (or single) component */
>> +                    namelen = strlen((char *)comp);
>> +                    hash = libxfs_da_hashname(comp, namelen);
>> +                    obfuscate_name(hash, namelen, comp);
>> +                    break;
>> +            }
>> +            namelen = slash - (char *)comp;
>> +            /* handle leading or consecutive slashes */
>> +            if (!namelen) {
>> +                    comp++;
>> +                    continue;
>> +            }
>> +            hash = libxfs_da_hashname(comp, namelen);
>> +            obfuscate_name(hash, namelen, comp);
>> +            comp += namelen + 1;
>> +    }
>> +}
>> +
>> +static void
>>  obfuscate_sf_symlink(
>>      xfs_dinode_t            *dip)
>>  {
>> @@ -971,8 +1005,7 @@ obfuscate_sf_symlink(
>>      }
>>  
>>      buf = (char *)XFS_DFORK_DPTR(dip);
>> -    while (len > 0)
>> -            buf[--len] = random() % 127 + 1;
>> +    obfuscate_path_components(buf, len);
>>  }
>>  
>>  static void
>> @@ -1176,11 +1209,8 @@ obfuscate_symlink_blocks(
>>      char                    *block,
>>      xfs_dfilblks_t          count)
>>  {
>> -    int                     i;
>> -
>>      count <<= mp->m_sb.sb_blocklog;
>> -    for (i = 0; i < count; i++)
>> -            block[i] = random() % 127 + 1;
>> +    obfuscate_path_components(block, count);

This is interesting.  Is there any guarantee that the hashed
result here will match the hashed target file?  I don't think
there is, necessarily (mostly if the target is subject to a
hash collision).  It's a corner case though so it can most
likely be ignored.

>>  }
>>  
>>  #define MAX_REMOTE_VALS             4095
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@xxxxxxxxxxx
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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