On 9/4/24 18:16, Mike Rapoport wrote:
> On Wed, Sep 04, 2024 at 05:49:15PM +0200, Vlastimil Babka wrote:
>> On 9/4/24 17:16, Mike Rapoport wrote:
>> > On Tue, Sep 03, 2024 at 04:20:43PM +0200, Christian Brauner wrote:
>> >> @@ -275,7 +285,7 @@ do_kmem_cache_create_usercopy(const char *name,
>> >>
>> >> mutex_lock(&slab_mutex);
>> >>
>> >> - err = kmem_cache_sanity_check(name, size);
>> >> + err = kmem_cache_sanity_check(name, object_size);
>> >> if (err) {
>> >> goto out_unlock;
>> >> }
>> >> @@ -296,12 +306,14 @@ do_kmem_cache_create_usercopy(const char *name,
>> >>
>> >> /* Fail closed on bad usersize of useroffset values. */
>> >> if (!IS_ENABLED(CONFIG_HARDENED_USERCOPY) ||
>> >> - WARN_ON(!usersize && useroffset) ||
>> >> - WARN_ON(size < usersize || size - usersize < useroffset))
>> >> - usersize = useroffset = 0;
>> >> -
>> >> - if (!usersize)
>> >> - s = __kmem_cache_alias(name, size, align, flags, ctor);
>> >> + WARN_ON(!args->usersize && args->useroffset) ||
>> >> + WARN_ON(object_size < args->usersize ||
>> >> + object_size - args->usersize < args->useroffset))
>> >> + args->usersize = args->useroffset = 0;
>> >> +
>> >> + if (!args->usersize)
>> >> + s = __kmem_cache_alias(name, object_size, args->align, flags,
>> >> + args->ctor);
>> >
>> > Sorry I missed it in the previous review, but nothing guaranties that
>> > nobody will call kmem_cache_create_args with args != NULL.
>> >
>> > I think there should be a check for args != NULL and a substitution of args
>> > with defaults if it actually was NULL.
>>
>> Hm there might be a bigger problem with this? If we wanted to do a
>> (non-flag-day) conversion to the new kmem_cache_create() for some callers
>> that need none of the extra args, passing NULL wouldn't work for the
>> _Generic((__args) looking for "struct kmem_cache_args *" as NULL is not of
>> that type, right?
>>
>> I tried and it really errors out.
>
> How about
>
> #define kmem_cache_create(__name, __object_size, __args, ...) \
> _Generic((__args), \
> struct kmem_cache_args *: __kmem_cache_create_args, \
> void *: __kmem_cache_create_args, \
> default: __kmem_cache_create)(__name, __object_size, __args, __VA_ARGS__)
Seems to work. I'd agree with the "if NULL, use a static default" direction
then. It just seems like a more user-friendly API to me.
