On 2024/11/15 02:19, Alex Williamson wrote:
On Wed, 13 Nov 2024 15:22:02 +0800
Yi Liu <yi.l.liu@xxxxxxxxx> 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
