Messages in this thread | | | Date | Tue, 23 May 2023 10:28:32 +0200 | Subject | Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR | From | David Hildenbrand <> |
| |
On 23.05.23 10:19, Lorenzo Stoakes wrote: > On Tue, May 23, 2023 at 10:14:24AM +0200, David Hildenbrand wrote: >> On 23.05.23 09:56, Lorenzo Stoakes wrote: >>> On Tue, May 23, 2023 at 09:46:46AM +0200, Vlastimil Babka wrote: >>>> On 5/23/23 09:42, Lorenzo Stoakes wrote: >>>>> On Tue, May 23, 2023 at 09:31:36AM +0200, Vlastimil Babka wrote: >>>>>> With SLOB removed, both remaining allocators support hardened usercopy, >>>>>> so remove the config and associated #ifdef. >>>>>> >>>>>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >>>>>> --- >>>>>> mm/Kconfig | 2 -- >>>>>> mm/slab.h | 9 --------- >>>>>> security/Kconfig | 8 -------- >>>>>> 3 files changed, 19 deletions(-) >>>>>> >>>>>> diff --git a/mm/Kconfig b/mm/Kconfig >>>>>> index 7672a22647b4..041f0da42f2b 100644 >>>>>> --- a/mm/Kconfig >>>>>> +++ b/mm/Kconfig >>>>>> @@ -221,7 +221,6 @@ choice >>>>>> config SLAB >>>>>> bool "SLAB" >>>>>> depends on !PREEMPT_RT >>>>>> - select HAVE_HARDENED_USERCOPY_ALLOCATOR >>>>>> help >>>>>> The regular slab allocator that is established and known to work >>>>>> well in all environments. It organizes cache hot objects in >>>>>> @@ -229,7 +228,6 @@ config SLAB >>>>>> >>>>>> config SLUB >>>>>> bool "SLUB (Unqueued Allocator)" >>>>>> - select HAVE_HARDENED_USERCOPY_ALLOCATOR >>>>>> help >>>>>> SLUB is a slab allocator that minimizes cache line usage >>>>>> instead of managing queues of cached objects (SLAB approach). >>>>>> diff --git a/mm/slab.h b/mm/slab.h >>>>>> index f01ac256a8f5..695ef96b4b5b 100644 >>>>>> --- a/mm/slab.h >>>>>> +++ b/mm/slab.h >>>>>> @@ -832,17 +832,8 @@ struct kmem_obj_info { >>>>>> void __kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *slab); >>>>>> #endif >>>>>> >>>>>> -#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR >>>>>> void __check_heap_object(const void *ptr, unsigned long n, >>>>>> const struct slab *slab, bool to_user); >>>>>> -#else >>>>>> -static inline >>>>>> -void __check_heap_object(const void *ptr, unsigned long n, >>>>>> - const struct slab *slab, bool to_user) >>>>>> -{ >>>>>> -} >>>>>> -#endif >>>>> >>>>> Hm, this is still defined in slab.c/slub.c and invoked in usercopy.c, do we >>>>> not want the prototype? >>>> >>>> Well I didn't delete the prototype, just the ifdef/else around, so now it's >>>> there unconditionally. >>>> >>>>> Perhaps replacing with #ifdef >>>>> CONFIG_HARDENED_USERCOPY instead? I may be missing something here :) >>>> >>>> Putting it under that #ifdef would work and match that the implementations >>>> of that function are under that same ifdef, but maybe it's unnecessary noise >>>> in the header? >>>> >>> >>> Yeah my brain inserted extra '-'s there, sorry! >>> >>> Given we only define __check_heap_object() in sl[au]b.c if >>> CONFIG_HARDENED_USERCOPY wouldn't we need to keep the empty version around >>> if !CONFIG_HARDENED_USERCOPY since check_heap_object() appears to be called >>> unconditionally? >>> >> >> The file is only compiled with CONFIG_HARDENED_USERCOPY: >> >> mm/Makefile:obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o >> > > Yeah ugh at this sort of implicit thing. Anyway it'd be preferable to stick > #ifdef CONFIG_HARDENED_USERCOPY around the prototype just so it's > abundantly clear this function doesn't exist unless that is set.
I recall that it is very common to not use ifdefs unless really required. Because less ifefs are obviously preferable ;)
Compilation+linking will fail in any case.
-- Thanks,
David / dhildenb
| |