Re: [PATCH slab/for-next v4 6/8] mm/slab: introduce struct kvfree_rcu_head for kvfree_rcu batching

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KASAN tag loss in kvmalloc_obj_start_addr() causes debugobjects memory leak, corrupted tracepoint offsets, and KASAN double-free bypass.
--

--- Patch [6]: [PATCH slab/for-next v4 6/8] mm/slab: introduce struct kvfree_rcu_head for kvfree_rcu batching ---
commit b85878f9ac539d3e206f29f7df390cb30b50c271
Author: Harry Yoo (Oracle) <[email protected]>
Subject: mm/slab: introduce struct kvfree_rcu_head for kvfree_rcu batching

This patch introduces `kvfree_rcu_head` to replace `rcu_head` for
kvfree_rcu batching. It stops storing the object pointer directly and
instead calculates the object's start address dynamically via
`kvmalloc_obj_start_addr()` to reduce structure size overhead.

> diff --git a/mm/slab.h b/mm/slab.h
> index fdd293b3efa56..1efeaceb7db32 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -351,6 +351,33 @@ static inline int objs_per_slab(const struct kmem_cache *cache,

[ ... ]

> +/* kvfree_rcu_head offset can be only less than page size */
> +static inline void *kvmalloc_obj_start_addr(void *head)
> +{
> +	void *obj = head;
> +
> +	if (unlikely(is_vmalloc_addr(obj))) {
> +		obj = (void *) PAGE_ALIGN_DOWN((unsigned long)obj);
> +	} else {
> +		struct page *page = virt_to_page(obj);
> +		struct slab *slab = page_slab(page);
> +
> +		if (!slab) {
> +			obj = (void *) PAGE_ALIGN_DOWN((unsigned long)obj);
> +		} else if (is_kfence_address(obj)) {
> +			obj = kfence_object_start(obj);
> +		} else {
> +			struct kmem_cache *s = slab->slab_cache;
> +			unsigned int idx = __obj_to_index(s, slab_address(slab), obj);
> +
> +			obj = slab_address(slab) + s->size * idx;

[Severity: High]
Does reconstructing the base address using slab_address(slab) drop the KASAN
tag from the original allocation?

slab_address() returns a tagless linear map address, so the resulting obj
pointer will be untagged.

> +			obj = fixup_red_left(s, obj);
> +		}
> +	}
> +
> +	return obj;
> +}
> +
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 8c631bf97cd5b..c1c32909fa528 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c

[ ... ]

>  static void
> -kvfree_rcu_list(struct rcu_head *head)
> +kvfree_rcu_list(struct kvfree_rcu_head *head)
>  {
> -	struct rcu_head *next;
> +	struct kvfree_rcu_head *next;
>  
>  	for (; head; head = next) {
> -		void *ptr = (void *) head->func;
> +		void *ptr = kvmalloc_obj_start_addr(head);
>  		unsigned long offset = (void *) head - ptr;

[Severity: High]
If ptr is untagged but head still has its KASAN tag, could this cause the
offset calculation to yield a massive value due to the tag difference in the
upper bits?

Additionally, does passing the untagged ptr to debug_rcu_head_unqueue()
(called slightly later in this function) cause an exact-match hash failure
and permanently leak the tracking object?

>  
>  		next = head->next;
> diff --git a/mm/slub.c b/mm/slub.c
> index deac315d0f23e..d0976396d703d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -6699,43 +6699,21 @@ static void free_large_kmalloc(struct page *page, void *object)
>   */
>  void kvfree_rcu_cb(struct rcu_head *head)
>  {
> -	void *obj = head;

[ ... ]

> +	obj = kvmalloc_obj_start_addr(head);
>  
>  	if (is_vmalloc_addr(obj)) {
>  		vfree(obj);
>  	} else {
> -		unsigned int idx = __obj_to_index(s, slab_addr, obj);
> +		struct page *page = virt_to_page(obj);
> +		struct slab *slab = page_slab(page);
>  
> -		obj = slab_addr + s->size * idx;
> -		obj = fixup_red_left(s, obj);
> +		if (slab)
> +			slab_free(slab->slab_cache, slab, obj, _RET_IP_);

[Severity: High]
Does passing the untagged obj pointer into slab_free() silently bypass
KASAN quarantine and double-free/use-after-free validation checks that
rely on the tag?

> +		else
> +			free_large_kmalloc(page, obj);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6