Re: [PATCH v2 02/15] slab: add struct kmem_cache_args

[Christian Brauner <brauner@kernel.org>]

On Wed, Sep 04, 2024 at 06:22:45PM GMT, Vlastimil Babka wrote:
> 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.

Sure. So can you fold your suggestion above and the small diff below
into the translation layer patch?

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 30000dcf0736..3c12d87825e3 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -255,9 +255,14 @@ struct kmem_cache *__kmem_cache_create_args(const char *name,
                                            slab_flags_t flags)
 {
        struct kmem_cache *s = NULL;
+       struct kmem_cache_args kmem_args = {};
        const char *cache_name;
        int err;

+       /* If no custom arguments are requested just assume the default values. */
+       if (!args)
+               args = &kmem_args;
+
 #ifdef CONFIG_SLUB_DEBUG
        /*
         * If no slab_debug was enabled globally, the static key is not yet