Message-ID: <110b2d4a-a33a-7002-1176-73dc85e1de27@linaro.org>
Date:   Mon, 6 Mar 2023 15:55:18 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH 2/3] ext4: fsmap: Consolidate fsmap_head checks
To:     "Darrick J. Wong" <djwong@kernel.org>
Cc:     tytso@mit.edu, darrick.wong@oracle.com, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        joneslee@google.com
References: <20230222131211.3898066-1-tudor.ambarus@linaro.org>
 <20230222131211.3898066-3-tudor.ambarus@linaro.org>
 <20230304025644.GA1637890@frogsfrogsfrogs>
Content-Language: en-US
From:   Tudor Ambarus <tudor.ambarus@linaro.org>
In-Reply-To: <20230304025644.GA1637890@frogsfrogsfrogs>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi, Darrick,

Thanks for taking the time to review this patch.

On 3/4/23 02:56, Darrick J. Wong wrote:
> On Wed, Feb 22, 2023 at 01:12:10PM +0000, Tudor Ambarus wrote:
>> Sanity checks should be done the soonest possible to avoid superfluous
>> computations when user provides wrong data. Gather all the checks on
>> user provided data in a single method and call it immediately after
>> copying the data from user.
> 
> This patch changes the validation criteria, moves chunks of code around,

The validation criteria remains the same, there's no functional change
in the code.

> and constifies parameters all at once.  And all you say here is that
> you're moving validation code up in the sequence!

My apologies, I should have mentioned something about the
constification. I chose to do the validation over const data because
the data should not be changed at validation time, otherwise one may end
with nasty implications on the sequence of validation. The const change
deserved at least a comment if not a dedicated patch, I agree.

> 
> Also, how does moving callsites around improve things?  Do the fstests

You don't waste CPU cycles in case the validation fails later on in the
code. Every initialization that is done before the last validation check
is superfluous in case the validation fails. Also, having the validation 
scattered around copies of user data and in different methods is harder
to follow. What I did was to gather all validation checks in a single
method and call it the soonest possible. IMO this makes the code cleaner
and easier to understand.

> still pass?

Yes, please check the cover letter at:
https://lore.kernel.org/linux-ext4/20230222131211.3898066-1-tudor.ambarus@linaro.org/

All the available ext4 fsmap tests passed after this patch set. I tested
ext4/{027, 028, 029}.

> 
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>   fs/ext4/fsmap.c | 52 ++++++++++++++++++++++++++++++++++++-------------
>>   fs/ext4/fsmap.h |  3 +++
>>   fs/ext4/ioctl.c | 17 +++-------------
>>   3 files changed, 44 insertions(+), 28 deletions(-)
>>
>> diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
>> index b5289378a761..a27d9f0967b7 100644
>> --- a/fs/ext4/fsmap.c
>> +++ b/fs/ext4/fsmap.c
>> @@ -9,6 +9,7 @@
>>   #include "fsmap.h"
>>   #include "mballoc.h"
>>   #include <linux/sort.h>
>> +#include <linux/string.h>
>>   #include <linux/list_sort.h>
>>   #include <trace/events/ext4.h>
>>   
>> @@ -571,7 +572,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
>>   
>>   /* Do we recognize the device? */
>>   static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
>> -					  struct ext4_fsmap *fm)
>> +					  const struct fsmap *fm)
>>   {
>>   	if (fm->fmr_device == 0 || fm->fmr_device == UINT_MAX ||
>>   	    fm->fmr_device == new_encode_dev(sb->s_bdev->bd_dev))
>> @@ -583,17 +584,19 @@ static bool ext4_getfsmap_is_valid_device(struct super_block *sb,
>>   }
>>   
>>   /* Ensure that the low key is less than the high key. */
>> -static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
>> -				     struct ext4_fsmap *high_key)
>> +static bool ext4_getfsmap_check_keys(const struct fsmap *low_key,
>> +				     const struct fsmap *high_key)
>>   {
>> +	u64 l_fmr_phys = low_key->fmr_physical + low_key->fmr_length;
>> +
>>   	if (low_key->fmr_device > high_key->fmr_device)
>>   		return false;
>>   	if (low_key->fmr_device < high_key->fmr_device)
>>   		return true;
>>   
>> -	if (low_key->fmr_physical > high_key->fmr_physical)
>> +	if (l_fmr_phys > high_key->fmr_physical)
>>   		return false;
>> -	if (low_key->fmr_physical < high_key->fmr_physical)
>> +	if (l_fmr_phys < high_key->fmr_physical)
> 
> Why are you changing the comparison here?

So that I preserve the validation check that was done before this patch.

In the code there are 3 representations of the key on which we currently
do validations:
1/  the ones from struct fsmap_head head; -> contains the data copied
from the user
2/ the ones from struct ext4_fsmap_head xhead; -> ext4 internal
representation of the fsmap
3/ dkeys - local keys used to query the device. These are 2/ but with 
the low key bumped by fmr_length.

As you correctly identified below, ext4_getfsmap_check_keys() validated
dkeys[0] and fmh_keys[1], so a combination of 2/ and 3/, whereas now I 
use it to validate directly the data copied from user, thus the data 
from 1/. In order to do that and at the same time to preserve the logic, 
I had to introduce a local variable, l_fmr_phys, and bump the low key by
fmr_length.

As you see, now instead of scattering the checks on data from 1/, 2/ and 
3/, I do the checks only on the user provided data, thus 1/.
> 
>>   		return true;
>>   
>>   	if (low_key->fmr_owner > high_key->fmr_owner)
>> @@ -604,6 +607,36 @@ static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key,
>>   	return false;
>>   }
>>   
>> +int ext4_fsmap_check_head(struct super_block *sb,
>> +			  const struct fsmap_head *head)
>> +{
>> +	const struct fsmap *l = &head->fmh_keys[0];
>> +	const struct fsmap *h = &head->fmh_keys[1];
>> +
>> +	if (memchr_inv(head->fmh_reserved, 0, sizeof(head->fmh_reserved)) ||
>> +	    memchr_inv(l->fmr_reserved, 0, sizeof(l->fmr_reserved)) ||
>> +	    memchr_inv(h->fmr_reserved, 0, sizeof(h->fmr_reserved)))
>> +		return -EINVAL;
>> +	/*
>> +	 * ext4 doesn't report file extents at all, so the only valid
>> +	 * file offsets are the magic ones (all zeroes or all ones).
>> +	 */
>> +	if (l->fmr_offset || (h->fmr_offset != 0 && h->fmr_offset != -1ULL))
>> +		return -EINVAL;
>> +
>> +	if (head->fmh_iflags & ~FMH_IF_VALID)
>> +		return -EINVAL;
>> +
>> +	if (!ext4_getfsmap_is_valid_device(sb, l) ||
>> +	    !ext4_getfsmap_is_valid_device(sb, h))
>> +		return -EINVAL;
>> +
>> +	if (!ext4_getfsmap_check_keys(l, h))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>   #define EXT4_GETFSMAP_DEVS	2
>>   /*
>>    * Get filesystem's extents as described in head, and format for
>> @@ -635,12 +668,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
>>   	int i;
>>   	int error = 0;
>>   
>> -	if (head->fmh_iflags & ~FMH_IF_VALID)
>> -		return -EINVAL;
>> -	if (!ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[0]) ||
>> -	    !ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[1]))
>> -		return -EINVAL;
>> -
>>   	head->fmh_entries = 0;
>>   
>>   	/* Set up our device handlers. */
>> @@ -673,9 +700,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head,
>>   	dkeys[0].fmr_length = 0;
>>   	memset(&dkeys[1], 0xFF, sizeof(struct ext4_fsmap));
>>   
>> -	if (!ext4_getfsmap_check_keys(dkeys, &head->fmh_keys[1]))
>> -		return -EINVAL;
> 
> And why is it ok to turn validation of dkeys[0] vs. fmh_keys[1] into a
> validation of fmh_keys[0..1] ?  I guess that's why check_keys now adds
> the low key physical offset and length?

Yes, you're correct. It's okay to validate directly on the data copied
from user because 2/ and 3/ are just copies of 1/.

> 
> But why not leave the key checks the where they are, since it's
> dkeys[0..1] that get passed around the implementations?
> 

I hope I made it clear at this point. Waiting for your reply.

Cheers,
ta