Re: [PATCH] mm/slab: remove HAVE_HARDENED_USERCOPY_ALLOCATOR

[David Hildenbrand <david@redhat.com>]

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



Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb