| Message ID | 20240903-work-kmem_cache_args-v2-2-76f97e9a4560@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | slab: add struct kmem_cache_args | expand |
On Tue, Sep 03, 2024 at 04:20:43PM +0200, Christian Brauner wrote: > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > include/linux/slab.h | 21 ++++++++++++++++ > mm/slab_common.c | 67 +++++++++++++++++++++++++++++++++++++++------------- > 2 files changed, 72 insertions(+), 16 deletions(-) > > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 5b2da2cf31a8..79d8c8bca4a4 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -240,6 +240,27 @@ struct mem_cgroup; > */ > bool slab_is_available(void); > > +/** > + * @align: The required alignment for the objects. > + * @useroffset: Usercopy region offset > + * @usersize: Usercopy region size > + * @freeptr_offset: Custom offset for the free pointer in RCU caches > + * @use_freeptr_offset: Whether a @freeptr_offset is used > + * @ctor: A constructor for the objects. > + */ > +struct kmem_cache_args { > + unsigned int align; > + unsigned int useroffset; > + unsigned int usersize; > + unsigned int freeptr_offset; > + bool use_freeptr_offset; > + void (*ctor)(void *); > +}; > + > +struct kmem_cache *__kmem_cache_create_args(const char *name, > + unsigned int object_size, > + struct kmem_cache_args *args, > + slab_flags_t flags); I'd name it __kmem_cache_create() and then s/kmem_cache_create/_kmem_cache_create/ in patch 12. Other than that Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org> > struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, > unsigned int align, slab_flags_t flags, > void (*ctor)(void *));
On 9/3/24 16:20, Christian Brauner wrote: You could describe that it's to hold less common args and there's __kmem_cache_create_args() that takes it, and do_kmem_cache_create_usercopy() is converted to it? Otherwise LGTM. > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > include/linux/slab.h | 21 ++++++++++++++++ > mm/slab_common.c | 67 +++++++++++++++++++++++++++++++++++++++------------- > 2 files changed, 72 insertions(+), 16 deletions(-) > > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 5b2da2cf31a8..79d8c8bca4a4 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -240,6 +240,27 @@ struct mem_cgroup; > */ > bool slab_is_available(void); > > +/** > + * @align: The required alignment for the objects. > + * @useroffset: Usercopy region offset > + * @usersize: Usercopy region size > + * @freeptr_offset: Custom offset for the free pointer in RCU caches > + * @use_freeptr_offset: Whether a @freeptr_offset is used > + * @ctor: A constructor for the objects. > + */ > +struct kmem_cache_args { > + unsigned int align; > + unsigned int useroffset; > + unsigned int usersize; > + unsigned int freeptr_offset; > + bool use_freeptr_offset; > + void (*ctor)(void *); > +}; > + > +struct kmem_cache *__kmem_cache_create_args(const char *name, > + unsigned int object_size, > + struct kmem_cache_args *args, > + slab_flags_t flags); > struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, > unsigned int align, slab_flags_t flags, > void (*ctor)(void *)); > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 91e0e36e4379..0f13c045b8d1 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -248,14 +248,24 @@ static struct kmem_cache *create_cache(const char *name, > return ERR_PTR(err); > } > > -static struct kmem_cache * > -do_kmem_cache_create_usercopy(const char *name, > - unsigned int size, unsigned int freeptr_offset, > - unsigned int align, slab_flags_t flags, > - unsigned int useroffset, unsigned int usersize, > - void (*ctor)(void *)) > +/** > + * __kmem_cache_create_args - Create a kmem cache > + * @name: A string which is used in /proc/slabinfo to identify this cache. > + * @object_size: The size of objects to be created in this cache. > + * @args: Arguments for the cache creation (see struct kmem_cache_args). > + * @flags: See %SLAB_* flags for an explanation of individual @flags. > + * > + * Cannot be called within a interrupt, but can be interrupted. > + * > + * Return: a pointer to the cache on success, NULL on failure. > + */ > +struct kmem_cache *__kmem_cache_create_args(const char *name, > + unsigned int object_size, > + struct kmem_cache_args *args, > + slab_flags_t flags) > { > struct kmem_cache *s = NULL; > + unsigned int freeptr_offset = UINT_MAX; > const char *cache_name; > int err; > > @@ -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); > if (s) > goto out_unlock; > > @@ -311,9 +323,11 @@ do_kmem_cache_create_usercopy(const char *name, > goto out_unlock; > } > > - s = create_cache(cache_name, size, freeptr_offset, > - calculate_alignment(flags, align, size), > - flags, useroffset, usersize, ctor); > + if (args->use_freeptr_offset) > + freeptr_offset = args->freeptr_offset; > + s = create_cache(cache_name, object_size, freeptr_offset, > + calculate_alignment(flags, args->align, object_size), > + flags, args->useroffset, args->usersize, args->ctor); > if (IS_ERR(s)) { > err = PTR_ERR(s); > kfree_const(cache_name); > @@ -335,6 +349,27 @@ do_kmem_cache_create_usercopy(const char *name, > } > return s; > } > +EXPORT_SYMBOL(__kmem_cache_create_args); > + > +static struct kmem_cache * > +do_kmem_cache_create_usercopy(const char *name, > + unsigned int size, unsigned int freeptr_offset, > + unsigned int align, slab_flags_t flags, > + unsigned int useroffset, unsigned int usersize, > + void (*ctor)(void *)) > +{ > + struct kmem_cache_args kmem_args = { > + .align = align, > + .use_freeptr_offset = freeptr_offset != UINT_MAX, > + .freeptr_offset = freeptr_offset, > + .useroffset = useroffset, > + .usersize = usersize, > + .ctor = ctor, > + }; > + > + return __kmem_cache_create_args(name, size, &kmem_args, flags); > +} > + > > /** > * kmem_cache_create_usercopy - Create a cache with a region suitable >
On Wed, Sep 04, 2024 at 10:13:11AM GMT, Vlastimil Babka wrote: > On 9/3/24 16:20, Christian Brauner wrote: > > You could describe that it's to hold less common args and there's > __kmem_cache_create_args() that takes it, and > do_kmem_cache_create_usercopy() is converted to it? Otherwise LGTM. Hm, I seem to have dropped the contents of the commit message when I split it into multiple individual commits. I've now added: "Currently we have multiple kmem_cache_create*() variants that take up to seven separate parameters with one of the functions having to grow an eigth parameter in the future to handle both usercopy and a custom freelist pointer. Add a struct kmem_cache_args structure and move less common parameters into it. Core parameters such as name, object size, and flags continue to be passed separately. Add a new function __kmem_cache_create_args() that takes a struct kmem_cache_args pointer and port do_kmem_cache_create_usercopy() over to it. In follow-up patches we will port the other kmem_cache_create*() variants over to it as well." to the work.kmem_cache_args branch. > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > --- > > include/linux/slab.h | 21 ++++++++++++++++ > > mm/slab_common.c | 67 +++++++++++++++++++++++++++++++++++++++------------- > > 2 files changed, 72 insertions(+), 16 deletions(-) > > > > diff --git a/include/linux/slab.h b/include/linux/slab.h > > index 5b2da2cf31a8..79d8c8bca4a4 100644 > > --- a/include/linux/slab.h > > +++ b/include/linux/slab.h > > @@ -240,6 +240,27 @@ struct mem_cgroup; > > */ > > bool slab_is_available(void); > > > > +/** > > + * @align: The required alignment for the objects. > > + * @useroffset: Usercopy region offset > > + * @usersize: Usercopy region size > > + * @freeptr_offset: Custom offset for the free pointer in RCU caches > > + * @use_freeptr_offset: Whether a @freeptr_offset is used > > + * @ctor: A constructor for the objects. > > + */ > > +struct kmem_cache_args { > > + unsigned int align; > > + unsigned int useroffset; > > + unsigned int usersize; > > + unsigned int freeptr_offset; > > + bool use_freeptr_offset; > > + void (*ctor)(void *); > > +}; > > + > > +struct kmem_cache *__kmem_cache_create_args(const char *name, > > + unsigned int object_size, > > + struct kmem_cache_args *args, > > + slab_flags_t flags); > > struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, > > unsigned int align, slab_flags_t flags, > > void (*ctor)(void *)); > > diff --git a/mm/slab_common.c b/mm/slab_common.c > > index 91e0e36e4379..0f13c045b8d1 100644 > > --- a/mm/slab_common.c > > +++ b/mm/slab_common.c > > @@ -248,14 +248,24 @@ static struct kmem_cache *create_cache(const char *name, > > return ERR_PTR(err); > > } > > > > -static struct kmem_cache * > > -do_kmem_cache_create_usercopy(const char *name, > > - unsigned int size, unsigned int freeptr_offset, > > - unsigned int align, slab_flags_t flags, > > - unsigned int useroffset, unsigned int usersize, > > - void (*ctor)(void *)) > > +/** > > + * __kmem_cache_create_args - Create a kmem cache > > + * @name: A string which is used in /proc/slabinfo to identify this cache. > > + * @object_size: The size of objects to be created in this cache. > > + * @args: Arguments for the cache creation (see struct kmem_cache_args). > > + * @flags: See %SLAB_* flags for an explanation of individual @flags. > > + * > > + * Cannot be called within a interrupt, but can be interrupted. > > + * > > + * Return: a pointer to the cache on success, NULL on failure. > > + */ > > +struct kmem_cache *__kmem_cache_create_args(const char *name, > > + unsigned int object_size, > > + struct kmem_cache_args *args, > > + slab_flags_t flags) > > { > > struct kmem_cache *s = NULL; > > + unsigned int freeptr_offset = UINT_MAX; > > const char *cache_name; > > int err; > > > > @@ -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); > > if (s) > > goto out_unlock; > > > > @@ -311,9 +323,11 @@ do_kmem_cache_create_usercopy(const char *name, > > goto out_unlock; > > } > > > > - s = create_cache(cache_name, size, freeptr_offset, > > - calculate_alignment(flags, align, size), > > - flags, useroffset, usersize, ctor); > > + if (args->use_freeptr_offset) > > + freeptr_offset = args->freeptr_offset; > > + s = create_cache(cache_name, object_size, freeptr_offset, > > + calculate_alignment(flags, args->align, object_size), > > + flags, args->useroffset, args->usersize, args->ctor); > > if (IS_ERR(s)) { > > err = PTR_ERR(s); > > kfree_const(cache_name); > > @@ -335,6 +349,27 @@ do_kmem_cache_create_usercopy(const char *name, > > } > > return s; > > } > > +EXPORT_SYMBOL(__kmem_cache_create_args); > > + > > +static struct kmem_cache * > > +do_kmem_cache_create_usercopy(const char *name, > > + unsigned int size, unsigned int freeptr_offset, > > + unsigned int align, slab_flags_t flags, > > + unsigned int useroffset, unsigned int usersize, > > + void (*ctor)(void *)) > > +{ > > + struct kmem_cache_args kmem_args = { > > + .align = align, > > + .use_freeptr_offset = freeptr_offset != UINT_MAX, > > + .freeptr_offset = freeptr_offset, > > + .useroffset = useroffset, > > + .usersize = usersize, > > + .ctor = ctor, > > + }; > > + > > + return __kmem_cache_create_args(name, size, &kmem_args, flags); > > +} > > + > > > > /** > > * kmem_cache_create_usercopy - Create a cache with a region suitable > > >
On Tue, Sep 03, 2024 at 04:20:43PM +0200, Christian Brauner wrote: > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > include/linux/slab.h | 21 ++++++++++++++++ > mm/slab_common.c | 67 +++++++++++++++++++++++++++++++++++++++------------- > 2 files changed, 72 insertions(+), 16 deletions(-) > > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 5b2da2cf31a8..79d8c8bca4a4 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -240,6 +240,27 @@ struct mem_cgroup; > */ > bool slab_is_available(void); > > +/** > + * @align: The required alignment for the objects. > + * @useroffset: Usercopy region offset > + * @usersize: Usercopy region size > + * @freeptr_offset: Custom offset for the free pointer in RCU caches > + * @use_freeptr_offset: Whether a @freeptr_offset is used > + * @ctor: A constructor for the objects. > + */ > +struct kmem_cache_args { > + unsigned int align; > + unsigned int useroffset; > + unsigned int usersize; > + unsigned int freeptr_offset; > + bool use_freeptr_offset; > + void (*ctor)(void *); > +}; > + > +struct kmem_cache *__kmem_cache_create_args(const char *name, > + unsigned int object_size, > + struct kmem_cache_args *args, > + slab_flags_t flags); > struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, > unsigned int align, slab_flags_t flags, > void (*ctor)(void *)); > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 91e0e36e4379..0f13c045b8d1 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -248,14 +248,24 @@ static struct kmem_cache *create_cache(const char *name, > return ERR_PTR(err); > } > > -static struct kmem_cache * > -do_kmem_cache_create_usercopy(const char *name, > - unsigned int size, unsigned int freeptr_offset, > - unsigned int align, slab_flags_t flags, > - unsigned int useroffset, unsigned int usersize, > - void (*ctor)(void *)) > +/** > + * __kmem_cache_create_args - Create a kmem cache > + * @name: A string which is used in /proc/slabinfo to identify this cache. > + * @object_size: The size of objects to be created in this cache. > + * @args: Arguments for the cache creation (see struct kmem_cache_args). > + * @flags: See %SLAB_* flags for an explanation of individual @flags. > + * > + * Cannot be called within a interrupt, but can be interrupted. > + * > + * Return: a pointer to the cache on success, NULL on failure. > + */ > +struct kmem_cache *__kmem_cache_create_args(const char *name, > + unsigned int object_size, > + struct kmem_cache_args *args, > + slab_flags_t flags) > { > struct kmem_cache *s = NULL; > + unsigned int freeptr_offset = UINT_MAX; > const char *cache_name; > int err; > > @@ -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. > if (s) > goto out_unlock; > > @@ -311,9 +323,11 @@ do_kmem_cache_create_usercopy(const char *name, > goto out_unlock; > } > > - s = create_cache(cache_name, size, freeptr_offset, > - calculate_alignment(flags, align, size), > - flags, useroffset, usersize, ctor); > + if (args->use_freeptr_offset) > + freeptr_offset = args->freeptr_offset; > + s = create_cache(cache_name, object_size, freeptr_offset, > + calculate_alignment(flags, args->align, object_size), > + flags, args->useroffset, args->usersize, args->ctor); > if (IS_ERR(s)) { > err = PTR_ERR(s); > kfree_const(cache_name); > @@ -335,6 +349,27 @@ do_kmem_cache_create_usercopy(const char *name, > } > return s; > } > +EXPORT_SYMBOL(__kmem_cache_create_args); > + > +static struct kmem_cache * > +do_kmem_cache_create_usercopy(const char *name, > + unsigned int size, unsigned int freeptr_offset, > + unsigned int align, slab_flags_t flags, > + unsigned int useroffset, unsigned int usersize, > + void (*ctor)(void *)) > +{ > + struct kmem_cache_args kmem_args = { > + .align = align, > + .use_freeptr_offset = freeptr_offset != UINT_MAX, > + .freeptr_offset = freeptr_offset, > + .useroffset = useroffset, > + .usersize = usersize, > + .ctor = ctor, > + }; > + > + return __kmem_cache_create_args(name, size, &kmem_args, flags); > +} > + > > /** > * kmem_cache_create_usercopy - Create a cache with a region suitable > > -- > 2.45.2 >
On Wed, Sep 04, 2024 at 06:16:16PM GMT, Mike Rapoport wrote: > On Tue, Sep 03, 2024 at 04:20:43PM +0200, Christian Brauner wrote: > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > --- > > include/linux/slab.h | 21 ++++++++++++++++ > > mm/slab_common.c | 67 +++++++++++++++++++++++++++++++++++++++------------- > > 2 files changed, 72 insertions(+), 16 deletions(-) > > > > diff --git a/include/linux/slab.h b/include/linux/slab.h > > index 5b2da2cf31a8..79d8c8bca4a4 100644 > > --- a/include/linux/slab.h > > +++ b/include/linux/slab.h > > @@ -240,6 +240,27 @@ struct mem_cgroup; > > */ > > bool slab_is_available(void); > > > > +/** > > + * @align: The required alignment for the objects. > > + * @useroffset: Usercopy region offset > > + * @usersize: Usercopy region size > > + * @freeptr_offset: Custom offset for the free pointer in RCU caches > > + * @use_freeptr_offset: Whether a @freeptr_offset is used > > + * @ctor: A constructor for the objects. > > + */ > > +struct kmem_cache_args { > > + unsigned int align; > > + unsigned int useroffset; > > + unsigned int usersize; > > + unsigned int freeptr_offset; > > + bool use_freeptr_offset; > > + void (*ctor)(void *); > > +}; > > + > > +struct kmem_cache *__kmem_cache_create_args(const char *name, > > + unsigned int object_size, > > + struct kmem_cache_args *args, > > + slab_flags_t flags); > > struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, > > unsigned int align, slab_flags_t flags, > > void (*ctor)(void *)); > > diff --git a/mm/slab_common.c b/mm/slab_common.c > > index 91e0e36e4379..0f13c045b8d1 100644 > > --- a/mm/slab_common.c > > +++ b/mm/slab_common.c > > @@ -248,14 +248,24 @@ static struct kmem_cache *create_cache(const char *name, > > return ERR_PTR(err); > > } > > > > -static struct kmem_cache * > > -do_kmem_cache_create_usercopy(const char *name, > > - unsigned int size, unsigned int freeptr_offset, > > - unsigned int align, slab_flags_t flags, > > - unsigned int useroffset, unsigned int usersize, > > - void (*ctor)(void *)) > > +/** > > + * __kmem_cache_create_args - Create a kmem cache > > + * @name: A string which is used in /proc/slabinfo to identify this cache. > > + * @object_size: The size of objects to be created in this cache. > > + * @args: Arguments for the cache creation (see struct kmem_cache_args). > > + * @flags: See %SLAB_* flags for an explanation of individual @flags. > > + * > > + * Cannot be called within a interrupt, but can be interrupted. > > + * > > + * Return: a pointer to the cache on success, NULL on failure. > > + */ > > +struct kmem_cache *__kmem_cache_create_args(const char *name, > > + unsigned int object_size, > > + struct kmem_cache_args *args, > > + slab_flags_t flags) > > { > > struct kmem_cache *s = NULL; > > + unsigned int freeptr_offset = UINT_MAX; > > const char *cache_name; > > int err; > > > > @@ -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. I think that callers that pass NULL should all be switched to KMEM_CACHE() and passing NULL should simply not be supported. And the few callers that need some very special alignment need to pass struct kmem_cache_args anyway. So there should never be a need to pass NULL.
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.
On Wed, Sep 04, 2024 at 05:48:31PM +0200, Christian Brauner wrote: > On Wed, Sep 04, 2024 at 06:16:16PM GMT, Mike Rapoport wrote: > > On Tue, Sep 03, 2024 at 04:20:43PM +0200, Christian Brauner wrote: > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > > --- > > > include/linux/slab.h | 21 ++++++++++++++++ > > > mm/slab_common.c | 67 +++++++++++++++++++++++++++++++++++++++------------- > > > 2 files changed, 72 insertions(+), 16 deletions(-) > > > > > > diff --git a/include/linux/slab.h b/include/linux/slab.h > > > index 5b2da2cf31a8..79d8c8bca4a4 100644 > > > --- a/include/linux/slab.h > > > +++ b/include/linux/slab.h > > > @@ -240,6 +240,27 @@ struct mem_cgroup; > > > */ > > > bool slab_is_available(void); > > > > > > +/** > > > + * @align: The required alignment for the objects. > > > + * @useroffset: Usercopy region offset > > > + * @usersize: Usercopy region size > > > + * @freeptr_offset: Custom offset for the free pointer in RCU caches > > > + * @use_freeptr_offset: Whether a @freeptr_offset is used > > > + * @ctor: A constructor for the objects. > > > + */ > > > +struct kmem_cache_args { > > > + unsigned int align; > > > + unsigned int useroffset; > > > + unsigned int usersize; > > > + unsigned int freeptr_offset; > > > + bool use_freeptr_offset; > > > + void (*ctor)(void *); > > > +}; > > > + > > > +struct kmem_cache *__kmem_cache_create_args(const char *name, > > > + unsigned int object_size, > > > + struct kmem_cache_args *args, > > > + slab_flags_t flags); > > > struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, > > > unsigned int align, slab_flags_t flags, > > > void (*ctor)(void *)); > > > diff --git a/mm/slab_common.c b/mm/slab_common.c > > > index 91e0e36e4379..0f13c045b8d1 100644 > > > --- a/mm/slab_common.c > > > +++ b/mm/slab_common.c > > > @@ -248,14 +248,24 @@ static struct kmem_cache *create_cache(const char *name, > > > return ERR_PTR(err); > > > } > > > > > > -static struct kmem_cache * > > > -do_kmem_cache_create_usercopy(const char *name, > > > - unsigned int size, unsigned int freeptr_offset, > > > - unsigned int align, slab_flags_t flags, > > > - unsigned int useroffset, unsigned int usersize, > > > - void (*ctor)(void *)) > > > +/** > > > + * __kmem_cache_create_args - Create a kmem cache > > > + * @name: A string which is used in /proc/slabinfo to identify this cache. > > > + * @object_size: The size of objects to be created in this cache. > > > + * @args: Arguments for the cache creation (see struct kmem_cache_args). > > > + * @flags: See %SLAB_* flags for an explanation of individual @flags. > > > + * > > > + * Cannot be called within a interrupt, but can be interrupted. > > > + * > > > + * Return: a pointer to the cache on success, NULL on failure. > > > + */ > > > +struct kmem_cache *__kmem_cache_create_args(const char *name, > > > + unsigned int object_size, > > > + struct kmem_cache_args *args, > > > + slab_flags_t flags) > > > { > > > struct kmem_cache *s = NULL; > > > + unsigned int freeptr_offset = UINT_MAX; > > > const char *cache_name; > > > int err; > > > > > > @@ -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. > > I think that callers that pass NULL should all be switched to > KMEM_CACHE() and passing NULL should simply not be supported. And the > few callers that need some very special alignment need to pass struct > kmem_cache_args anyway. So there should never be a need to pass NULL. But you can't guarantee that some random driver won't call __kmem_cache_create_args("name", size, NULL, flags); At least we'd need if (!args) return -EINVAL;
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__)
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.
On Wed, Sep 04, 2024 at 07:16:07PM GMT, Mike Rapoport wrote: > On Wed, Sep 04, 2024 at 05:48:31PM +0200, Christian Brauner wrote: > > On Wed, Sep 04, 2024 at 06:16:16PM GMT, Mike Rapoport wrote: > > > On Tue, Sep 03, 2024 at 04:20:43PM +0200, Christian Brauner wrote: > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > > > --- > > > > include/linux/slab.h | 21 ++++++++++++++++ > > > > mm/slab_common.c | 67 +++++++++++++++++++++++++++++++++++++++------------- > > > > 2 files changed, 72 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/include/linux/slab.h b/include/linux/slab.h > > > > index 5b2da2cf31a8..79d8c8bca4a4 100644 > > > > --- a/include/linux/slab.h > > > > +++ b/include/linux/slab.h > > > > @@ -240,6 +240,27 @@ struct mem_cgroup; > > > > */ > > > > bool slab_is_available(void); > > > > > > > > +/** > > > > + * @align: The required alignment for the objects. > > > > + * @useroffset: Usercopy region offset > > > > + * @usersize: Usercopy region size > > > > + * @freeptr_offset: Custom offset for the free pointer in RCU caches > > > > + * @use_freeptr_offset: Whether a @freeptr_offset is used > > > > + * @ctor: A constructor for the objects. > > > > + */ > > > > +struct kmem_cache_args { > > > > + unsigned int align; > > > > + unsigned int useroffset; > > > > + unsigned int usersize; > > > > + unsigned int freeptr_offset; > > > > + bool use_freeptr_offset; > > > > + void (*ctor)(void *); > > > > +}; > > > > + > > > > +struct kmem_cache *__kmem_cache_create_args(const char *name, > > > > + unsigned int object_size, > > > > + struct kmem_cache_args *args, > > > > + slab_flags_t flags); > > > > struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, > > > > unsigned int align, slab_flags_t flags, > > > > void (*ctor)(void *)); > > > > diff --git a/mm/slab_common.c b/mm/slab_common.c > > > > index 91e0e36e4379..0f13c045b8d1 100644 > > > > --- a/mm/slab_common.c > > > > +++ b/mm/slab_common.c > > > > @@ -248,14 +248,24 @@ static struct kmem_cache *create_cache(const char *name, > > > > return ERR_PTR(err); > > > > } > > > > > > > > -static struct kmem_cache * > > > > -do_kmem_cache_create_usercopy(const char *name, > > > > - unsigned int size, unsigned int freeptr_offset, > > > > - unsigned int align, slab_flags_t flags, > > > > - unsigned int useroffset, unsigned int usersize, > > > > - void (*ctor)(void *)) > > > > +/** > > > > + * __kmem_cache_create_args - Create a kmem cache > > > > + * @name: A string which is used in /proc/slabinfo to identify this cache. > > > > + * @object_size: The size of objects to be created in this cache. > > > > + * @args: Arguments for the cache creation (see struct kmem_cache_args). > > > > + * @flags: See %SLAB_* flags for an explanation of individual @flags. > > > > + * > > > > + * Cannot be called within a interrupt, but can be interrupted. > > > > + * > > > > + * Return: a pointer to the cache on success, NULL on failure. > > > > + */ > > > > +struct kmem_cache *__kmem_cache_create_args(const char *name, > > > > + unsigned int object_size, > > > > + struct kmem_cache_args *args, > > > > + slab_flags_t flags) > > > > { > > > > struct kmem_cache *s = NULL; > > > > + unsigned int freeptr_offset = UINT_MAX; > > > > const char *cache_name; > > > > int err; > > > > > > > > @@ -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. > > > > I think that callers that pass NULL should all be switched to > > KMEM_CACHE() and passing NULL should simply not be supported. And the > > few callers that need some very special alignment need to pass struct > > kmem_cache_args anyway. So there should never be a need to pass NULL. > > But you can't guarantee that some random driver won't call > > __kmem_cache_create_args("name", size, NULL, flags); > > At least we'd need > > if (!args) > return -EINVAL; Calling __kmem_cache_create_args() directly is a bug. That's why it's __*(). And we don't check for non-NULL @name either. In fact we almost never do such checks. Plus, if someone did: kmem_cache_create("foo", sizeof(foo), NULL, flags); they'd get a compile time error due to _Generic().
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
On Wed, 4 Sept 2024 at 11:21, Christian Brauner <brauner@kernel.org> wrote: > > Sure. So can you fold your suggestion above and the small diff below > into the translation layer patch? Please don't. This seems horrible. First you have a _Generic() macro that turns NULL into the same function that a proper __kmem_cache_create_args() with a real argument uses, and then you make that function check for NULL and turn it into something else. That seems *entirely* pointless. I think the right model is to either (a) not allow a NULL pointer at all (ie not have a _Generic() case for 'void *') and just error for that behavior OR (b) make a NULL pointer explicitly go to some other function than the one that gets a proper pointer but not this "do extra work in the function to make it accept the NULL we shunted to it". IOW, something like this: #define kmem_cache_create(__name, __object_size, __args, ...) \ _Generic((__args), \ struct kmem_cache_args *: __kmem_cache_create_args, \ void *: __kmem_cache_default_args, \ default: __kmem_cache_create)(__name, __object_size, __args, __VA_ARGS__) and then we have static inline struct kmem_cache *__kmem_cache_default_args(const char *name, unsigned int object_size, struct kmem_cache_args *args, slab_flags_t flags) { WARN_ON_ONCE(args); // It had *better* be NULL, not some random 'void *' return __kmem_cache_create_args(name, size, &kmem_args, flags); } which basically just does a "turn NULL into &kmem_args" thing. Notice how that does *not* add some odd NULL pointer check to the main path (and the WARN_ON_ONCE() check should be compiled away for any actual constant NULL argument, which is the only valid reason to have that 'void *' anyway). Linus
On Wed, Sep 04, 2024 at 11:53:05AM GMT, Linus Torvalds wrote: > On Wed, 4 Sept 2024 at 11:21, Christian Brauner <brauner@kernel.org> wrote: > > > > Sure. So can you fold your suggestion above and the small diff below > > into the translation layer patch? > > Please don't. > > This seems horrible. First you have a _Generic() macro that turns NULL > into the same function that a proper __kmem_cache_create_args() with a > real argument uses, and then you make that function check for NULL and > turn it into something else. > > That seems *entirely* pointless. > > I think the right model is to either > > (a) not allow a NULL pointer at all (ie not have a _Generic() case > for 'void *') and just error for that behavior Fine by me and what this did originally by erroring out with a compile time error. > > OR > > (b) make a NULL pointer explicitly go to some other function than the > one that gets a proper pointer > > but not this "do extra work in the function to make it accept the NULL > we shunted to it". > > IOW, something like this: > > #define kmem_cache_create(__name, __object_size, __args, ...) \ > _Generic((__args), \ > struct kmem_cache_args *: __kmem_cache_create_args, \ > void *: __kmem_cache_default_args, \ > default: __kmem_cache_create)(__name, __object_size, > __args, __VA_ARGS__) > > and then we have > > static inline struct kmem_cache *__kmem_cache_default_args(const char *name, > unsigned int object_size, > struct kmem_cache_args *args, > slab_flags_t flags) > { WARN_ON_ONCE(args); // It had *better* be NULL, not some random 'void *' > return __kmem_cache_create_args(name, size, &kmem_args, flags); } > > which basically just does a "turn NULL into &kmem_args" thing. > > Notice how that does *not* add some odd NULL pointer check to the main > path (and the WARN_ON_ONCE() check should be compiled away for any > actual constant NULL argument, which is the only valid reason to have > that 'void *' anyway). Also fine by me. See appended updated patch.
diff --git a/include/linux/slab.h b/include/linux/slab.h index 5b2da2cf31a8..79d8c8bca4a4 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -240,6 +240,27 @@ struct mem_cgroup; */ bool slab_is_available(void); +/** + * @align: The required alignment for the objects. + * @useroffset: Usercopy region offset + * @usersize: Usercopy region size + * @freeptr_offset: Custom offset for the free pointer in RCU caches + * @use_freeptr_offset: Whether a @freeptr_offset is used + * @ctor: A constructor for the objects. + */ +struct kmem_cache_args { + unsigned int align; + unsigned int useroffset; + unsigned int usersize; + unsigned int freeptr_offset; + bool use_freeptr_offset; + void (*ctor)(void *); +}; + +struct kmem_cache *__kmem_cache_create_args(const char *name, + unsigned int object_size, + struct kmem_cache_args *args, + slab_flags_t flags); struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, unsigned int align, slab_flags_t flags, void (*ctor)(void *)); diff --git a/mm/slab_common.c b/mm/slab_common.c index 91e0e36e4379..0f13c045b8d1 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -248,14 +248,24 @@ static struct kmem_cache *create_cache(const char *name, return ERR_PTR(err); } -static struct kmem_cache * -do_kmem_cache_create_usercopy(const char *name, - unsigned int size, unsigned int freeptr_offset, - unsigned int align, slab_flags_t flags, - unsigned int useroffset, unsigned int usersize, - void (*ctor)(void *)) +/** + * __kmem_cache_create_args - Create a kmem cache + * @name: A string which is used in /proc/slabinfo to identify this cache. + * @object_size: The size of objects to be created in this cache. + * @args: Arguments for the cache creation (see struct kmem_cache_args). + * @flags: See %SLAB_* flags for an explanation of individual @flags. + * + * Cannot be called within a interrupt, but can be interrupted. + * + * Return: a pointer to the cache on success, NULL on failure. + */ +struct kmem_cache *__kmem_cache_create_args(const char *name, + unsigned int object_size, + struct kmem_cache_args *args, + slab_flags_t flags) { struct kmem_cache *s = NULL; + unsigned int freeptr_offset = UINT_MAX; const char *cache_name; int err; @@ -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); if (s) goto out_unlock; @@ -311,9 +323,11 @@ do_kmem_cache_create_usercopy(const char *name, goto out_unlock; } - s = create_cache(cache_name, size, freeptr_offset, - calculate_alignment(flags, align, size), - flags, useroffset, usersize, ctor); + if (args->use_freeptr_offset) + freeptr_offset = args->freeptr_offset; + s = create_cache(cache_name, object_size, freeptr_offset, + calculate_alignment(flags, args->align, object_size), + flags, args->useroffset, args->usersize, args->ctor); if (IS_ERR(s)) { err = PTR_ERR(s); kfree_const(cache_name); @@ -335,6 +349,27 @@ do_kmem_cache_create_usercopy(const char *name, } return s; } +EXPORT_SYMBOL(__kmem_cache_create_args); + +static struct kmem_cache * +do_kmem_cache_create_usercopy(const char *name, + unsigned int size, unsigned int freeptr_offset, + unsigned int align, slab_flags_t flags, + unsigned int useroffset, unsigned int usersize, + void (*ctor)(void *)) +{ + struct kmem_cache_args kmem_args = { + .align = align, + .use_freeptr_offset = freeptr_offset != UINT_MAX, + .freeptr_offset = freeptr_offset, + .useroffset = useroffset, + .usersize = usersize, + .ctor = ctor, + }; + + return __kmem_cache_create_args(name, size, &kmem_args, flags); +} + /** * kmem_cache_create_usercopy - Create a cache with a region suitable
Signed-off-by: Christian Brauner <brauner@kernel.org> --- include/linux/slab.h | 21 ++++++++++++++++ mm/slab_common.c | 67 +++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 72 insertions(+), 16 deletions(-)