Re: [PATCH v5 4/5] vfio: Add vfio_copy_user_data()

[Yi Liu <yi.l.liu@intel.com>]

On 2024/11/15 02:19, Alex Williamson wrote:
> On Wed, 13 Nov 2024 15:22:02 +0800
> Yi Liu <yi.l.liu@intel.com> wrote:
> 
>> On 2024/11/12 21:52, Alex Williamson wrote:
>>
>>>>>> +{
>>>>>> +	unsigned long xend = minsz;
>>>>>> +	struct user_header {
>>>>>> +		u32 argsz;
>>>>>> +		u32 flags;
>>>>>> +	} *header;
>>>>>> +	unsigned long flags;
>>>>>> +	u32 flag;
>>>>>> +
>>>>>> +	if (copy_from_user(buffer, arg, minsz))
>>>>>> +		return -EFAULT;
>>>>>> +
>>>>>> +	header = (struct user_header *)buffer;
>>>>>> +	if (header->argsz < minsz)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	if (header->flags & ~flags_mask)
>>>>>> +		return -EINVAL;
>>>>>
>>>>> I'm already wrestling with whether this is an over engineered solution
>>>>> to remove a couple dozen lines of mostly duplicate logic between attach
>>>>> and detach, but a couple points that could make it more versatile:
>>>>>
>>>>> (1) Test xend_array here:
>>>>>
>>>>> 	if (!xend_array)
>>>>> 		return 0;
>>>>
>>>> Perhaps we should return error if the header->flags has any bit set. Such
>>>> cases require a valid xend_array.
>>>
>>> I don't think that's true.  For example if we want to drop this into
>>> existing cases where the structure size has not expanded and flags are
>>> used for other things, I don't think we want the overhead of declaring
>>> an xend_array.
>>
>> I see. My thought was sticking with using it in the cases that have
>> extended fields. Given that would it be better to return minsz as you
>> suggested to return ssize_t to caller.
> 
> If the xend_array is NULL, then yes it would do the copy, validate
> argsz and flags, and return minsz.

yes.

>>>>> (2) Return ssize_t/-errno for the caller to know the resulting copy
>>>>> size.
>>>>>       
>>>>>> +
>>>>>> +	/* Loop each set flag to decide the xend */
>>>>>> +	flags = header->flags;
>>>>>> +	for_each_set_bit(flag, &flags, BITS_PER_TYPE(u32)) {
>>>>>> +		if (xend_array[flag] > xend)
>>>>>> +			xend = xend_array[flag];
>>>>>
>>>>> Can we craft a BUILD_BUG in the wrapper to test that xend_array is at
>>>>> least long enough to match the highest bit in flags?  Thanks,
>>>>
>>>> yes. I would add a BUILD_BUG like the below.
>>>>
>>>> BUILD_BUG_ON(ARRAY_SIZE(_xend_array) < ilog2(_flags_mask));
>>>
>>> So this would need to account that _xend_array can be NULL regardless
>>> of _flags_mask.  Thanks,
>> yes, but I encounter a problem to account it. The below failed as when
>> the _xend_array is a null pointer. It's due to the usage of ARRAY_SIZE
>> macro. If it's not doable, perhaps we can have two wrappers, one for
>> copying user data with array, this should enforce the array num check
>> with flags. While, the another one is for copying user data without
>> array, no array num check. How about your opinion?
>>
>> BUILD_BUG_ON((_xend_array != NULL) && (ARRAY_SIZE(_xend_array) <
>> ilog2(_flags_mask)));
>>
>> Compiling fail snippet:
>>
>> In file included from <command-line>:
>> ./include/linux/array_size.h:11:38: warning: division ‘sizeof (long
>> unsigned int *) / sizeof (long unsigned int)’ does not compute the number
>> of array elements [-Wsizeof-pointer-div]
>>      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
>> __must_be_array(arr))
>>         |                                      ^
>> ././include/linux/compiler_types.h:497:23: note: in definition of macro
>> ‘__compiletime_assert’
>>     497 |                 if (!(condition))
>>        \
>>         |                       ^~~~~~~~~
>> ././include/linux/compiler_types.h:517:9: note: in expansion of macro
>> ‘_compiletime_assert’
>>     517 |         _compiletime_assert(condition, msg, __compiletime_assert_,
>> __COUNTER__)
>>         |         ^~~~~~~~~~~~~~~~~~~
>> ./include/linux/build_bug.h:39:37: note: in expansion of macro
>> ‘compiletime_assert’
>>      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> 
> TBH, I think this whole generalization is stalling the series.  We're
> de-duplicating 20-ish lines of code implemented by adjacent functions
> with something more complicated, I think mostly to formalize the
> methodology of using flags to expand the ioctl data structure, which
> has been our plan all along.  If it only addresses the duplication in
> these two functions, the added complexity isn't that compelling, but
> expanding it to be used more broadly is introducing scope creep.
> 
> Given the momentum on the iommufd side, if this series is intended to
> be v6.13 material, we should probably defer this generalization to a
> follow-on series where we can evaluate a more broadly used helper.

sure. I'm open about it. I may drop it in next series, and make this
generalization as a follow-up patch. :)

-- 
Regards,
Yi Liu