xfs
[Top] [All Lists]

Re: [PATCH 2/4] xfs: reject completely bogus remount options

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] xfs: reject completely bogus remount options
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Sun, 13 Oct 2013 21:42:37 -0500
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131013215208.GE5663@dastard>
References: <52584C8A.1060808@xxxxxxxxxx> <52584D56.7090902@xxxxxxxxxxx> <20131013215208.GE5663@dastard>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/20130801 Thunderbird/17.0.8
On 10/13/13 4:52 PM, Dave Chinner wrote:
> On Fri, Oct 11, 2013 at 02:11:18PM -0500, Eric Sandeen wrote:
>> There's a long comment about handling non-remountable
>> options in xfs_fs_remount, but nothing addresses the case
>> of completely bogus mount options at remount time, which
>> can lead to some severe strangeness:
>>
>> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
>> # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
>> # grep sdb4 /etc/mtab
>> /dev/sdb4 /mnt/test2 xfs 
>> rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption
>>  0 0
>>
>> This is a bit of a hack, but we can re-use xfs_parseargs()
>> with a dummy mount struct to just vet all of the remount
>> options which were passed in.  With this, we get a saner
>> result:
>>
>> [44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or 
>> missing value
> 
> ext4? Really? :)


uhhh ;)

>> +++ b/fs/xfs/xfs_super.c
>> @@ -1202,11 +1202,25 @@ xfs_fs_remount(
>>      int                     *flags,
>>      char                    *options)
>>  {
>> -    struct xfs_mount        *mp = XFS_M(sb);
>> +    struct xfs_mount        *mp = XFS_M(sb), *dummy_mp;
>>      substring_t             args[MAX_OPT_ARGS];
>>      char                    *p;
>>      int                     error;
>>  
>> +    /*
>> +     * Check all the mount options presented to be sure
>> +     * there's nothing too crazy in there.  Non-remountable
>> +     * but valid options are a different issue.
>> +     */
>> +    dummy_mp = kmem_zalloc(sizeof(*dummy_mp), KM_MAYFAIL);
>> +    if (dummy_mp) {
>> +            dummy_mp->m_super = sb;
>> +            error = xfs_parseargs(dummy_mp, options);
>> +            kfree(dummy_mp);
>> +            if (error)
>> +                    return -error;
> 
> This, at minimum, leaks dummy_mp->m_fsname, and it will leak other
> strings that are also kstrdup()d by xfs_parseargs().

nnngh.  Forgot about that side effect, sorry.  Dammit.

Think it's still worth doing this if I handle freeing them all up?

-Eric

> Cheers,
> 
> Dave.
> 

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