Re: [PATCH RFC 3/4] rcu, slab: use a regular callback function for kvfree_rcu

[Uladzislau Rezki <urezki@gmail.com>]

On Fri, Jan 24, 2025 at 03:16:05PM +0100, Vlastimil Babka wrote:
> On 1/24/25 13:47, Uladzislau Rezki wrote:
> > On Thu, Jan 23, 2025 at 11:37:20AM +0100, Vlastimil Babka wrote:
> >> RCU has been special-casing callback function pointers that are integers
> >> lower than 4096 as offsets of rcu_head for kvfree() instead. The tree
> >> RCU implementation no longer does that as the batched kvfree_rcu() is
> >> not a simple call_rcu(). The tiny RCU still does, and the plan is also
> >> to make tree RCU use call_rcu() for SLUB_TINY configurations.
> >> 
> >> Instead of teaching tree RCU again to special case the offsets, let's
> >> remove the special casing completely. Since there's no SLOB anymore, it
> >> is possible to create a callback function that can take a pointer to a
> >> middle of slab object with unknown offset and determine the object's
> >> pointer before freeing it, so implement that as kvfree_rcu_cb().
> >> 
> >> Large kmalloc and vmalloc allocations are handled simply by aligning
> >> down to page size. For that we retain the requirement that the offset is
> >> smaller than 4096. But we can remove __is_kvfree_rcu_offset() completely
> >> and instead just opencode the condition in the BUILD_BUG_ON() check.
> >> 
> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> >> ---
> >>  include/linux/rcupdate.h | 24 +++++++++---------------
> >>  kernel/rcu/tiny.c        | 13 -------------
> >>  mm/slab.h                |  2 ++
> >>  mm/slab_common.c         |  5 +----
> >>  mm/slub.c                | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>  5 files changed, 54 insertions(+), 32 deletions(-)
> >> 
> >> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> >> index 3f70d1c8144426f40553c8c589f07097ece8a706..7ff16a70ca1c0fb1012c4118388f60687c5e5b3f 100644
> >> --- a/include/linux/rcupdate.h
> >> +++ b/include/linux/rcupdate.h
> >> @@ -1025,12 +1025,6 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
> >>  #define RCU_POINTER_INITIALIZER(p, v) \
> >>  		.p = RCU_INITIALIZER(v)
> >>  
> >> -/*
> >> - * Does the specified offset indicate that the corresponding rcu_head
> >> - * structure can be handled by kvfree_rcu()?
> >> - */
> >> -#define __is_kvfree_rcu_offset(offset) ((offset) < 4096)
> >> -
> >>  /**
> >>   * kfree_rcu() - kfree an object after a grace period.
> >>   * @ptr: pointer to kfree for double-argument invocations.
> >> @@ -1041,11 +1035,11 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
> >>   * when they are used in a kernel module, that module must invoke the
> >>   * high-latency rcu_barrier() function at module-unload time.
> >>   *
> >> - * The kfree_rcu() function handles this issue.  Rather than encoding a
> >> - * function address in the embedded rcu_head structure, kfree_rcu() instead
> >> - * encodes the offset of the rcu_head structure within the base structure.
> >> - * Because the functions are not allowed in the low-order 4096 bytes of
> >> - * kernel virtual memory, offsets up to 4095 bytes can be accommodated.
> >> + * The kfree_rcu() function handles this issue. In order to have a universal
> >> + * callback function handling different offsets of rcu_head, the callback needs
> >> + * to determine the starting address of the freed object, which can be a large
> >> + * kmalloc of vmalloc allocation. To allow simply aligning the pointer down to
> >> + * page boundary for those, only offsets up to 4095 bytes can be accommodated.
> >>   * If the offset is larger than 4095 bytes, a compile-time error will
> >>   * be generated in kvfree_rcu_arg_2(). If this error is triggered, you can
> >>   * either fall back to use of call_rcu() or rearrange the structure to
> >> @@ -1091,10 +1085,10 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr);
> >>  do {									\
> >>  	typeof (ptr) ___p = (ptr);					\
> >>  									\
> >> -	if (___p) {									\
> >> -		BUILD_BUG_ON(!__is_kvfree_rcu_offset(offsetof(typeof(*(ptr)), rhf)));	\
> >> -		kvfree_call_rcu(&((___p)->rhf), (void *) (___p));			\
> >> -	}										\
> >> +	if (___p) {							\
> >> +		BUILD_BUG_ON(offsetof(typeof(*(ptr)), rhf) >= 4096);	\
> >> +		kvfree_call_rcu(&((___p)->rhf), (void *) (___p));	\
> >> +	}								\
> >>
> > Why removing the macro? At least __is_kvfree_rcu_offset() makes it
> > clear what and why + it has a nice comment what it is used for. 4096
> > looks like hard-coded value.
> 
> Hmm but it's ultimately shorter. We could add a short comment to
> kvfree_rcu_arg_2() referring to the longer explanation in the kfree_rcu()
> comment?
> 
Sounds good to place or keep the comment.

> > Or you do not want that someone else started to use that macro in
> > another places?
> 
> And also that, since this has to be exposed in a "public" header.
> 
> >> diff --git a/mm/slab.h b/mm/slab.h
> >> index e9fd9bf0bfa65b343a4ae0ecd5b4c2a325b04883..2f01c7317988ce036f0b22807403226a59f0f708 100644
> >> --- a/mm/slab.h
> >> +++ b/mm/slab.h
> >> @@ -604,6 +604,8 @@ void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
> >>  			    void **p, int objects, struct slabobj_ext *obj_exts);
> >>  #endif
> >>  
> >> +void kvfree_rcu_cb(struct rcu_head *head);
> >> +
> >>  size_t __ksize(const void *objp);
> >>  
> >>  static inline size_t slab_ksize(const struct kmem_cache *s)
> >> diff --git a/mm/slab_common.c b/mm/slab_common.c
> >> index 330cdd8ebc5380090ee784c58e8ca1d1a52b3758..f13d2c901daf1419993620459fbd5845eecb85f1 100644
> >> --- a/mm/slab_common.c
> >> +++ b/mm/slab_common.c
> >> @@ -1532,9 +1532,6 @@ kvfree_rcu_list(struct rcu_head *head)
> >>  		rcu_lock_acquire(&rcu_callback_map);
> >>  		trace_rcu_invoke_kvfree_callback("slab", head, offset);
> >>  
> >> -		if (!WARN_ON_ONCE(!__is_kvfree_rcu_offset(offset)))
> >> -			kvfree(ptr);
> >> -
> > This is not correct unless i miss something. Why do you remove this?
> 
> Oops, meant to remove just the warn check, and unconditionally call
> kvfree(), thanks for noticing :)
> 
> The warning could really only trigger due to a use-after-free corruption of
> the ptr pointer, because the BUILD_BUG_ON() would catch misuse with a too
> large offset, so we don't need to keep it.
> 
> >> +void kvfree_rcu_cb(struct rcu_head *head)
> >> +{
> >> +	void *obj = head;
> >> +	struct folio *folio;
> >> +	struct slab *slab;
> >> +	struct kmem_cache *s;
> >> +	void *slab_addr;
> >> +
> >> +	if (unlikely(is_vmalloc_addr(obj))) {
> >> +		obj = (void *) PAGE_ALIGN_DOWN((unsigned long)obj);
> >> +		vfree(obj);
> >> +		return;
> >> +	}
> >> +
> >> +	folio = virt_to_folio(obj);
> >> +	if (unlikely(!folio_test_slab(folio))) {
> >> +		/*
> >> +		 * rcu_head offset can be only less than page size so no need to
> >> +		 * consider folio order
> >> +		 */
> >> +		obj = (void *) PAGE_ALIGN_DOWN((unsigned long)obj);
> >> +		free_large_kmalloc(folio, obj);
> >> +		return;
> >> +	}
> >> +
> >> +	slab = folio_slab(folio);
> >> +	s = slab->slab_cache;
> >> +	slab_addr = folio_address(folio);
> >> +
> >> +	if (is_kfence_address(obj)) {
> >> +		obj = kfence_object_start(obj);
> >> +	} else {
> >> +		unsigned int idx = __obj_to_index(s, slab_addr, obj);
> >> +
> >> +		obj = slab_addr + s->size * idx;
> >> +		obj = fixup_red_left(s, obj);
> >> +	}
> >> +
> >> +	slab_free(s, slab, obj, _RET_IP_);
> >> +}
> >> +
> > Tiny computer case. Just small nit, maybe remove unlikely() but you decide :)
> 
> Force of habbit :)
> 
:)

--
Uladzislau Rezki