Re: [PATCH v2 1/3] slab: make check_object() more consistent

[Vlastimil Babka]

On 6/5/24 9:13 AM, Chengming Zhou wrote:
> Now check_object() calls check_bytes_and_report() multiple times to
> check every section of the object it cares about, like left and right
> redzones, object poison, paddings poison and freepointer. It will
> abort the checking process and return 0 once it finds an error.
> 
> There are two inconsistencies in check_object(), which are alignment
> padding checking and object padding checking. We only print the error
> messages but don't return 0 to tell callers that something is wrong
> and needs to be handled. Please see alloc_debug_processing() and
> free_debug_processing() for details.
> 
> If the above inconsistencies are not intentional, we should fix it.

It doesn't seem intentional, I don't see why padding specifically would be
different from the other tests here.

<snip>

> -	if (!freeptr_outside_object(s) && val == SLUB_RED_ACTIVE)
> -		/*
> -		 * Object and freepointer overlap. Cannot check
> -		 * freepointer while object is allocated.
> -		 */
> -		return 1;
> -
> -	/* Check free pointer validity */
> -	if (!check_valid_pointer(s, slab, get_freepointer(s, p))) {
> +	/*
> +	 * Cannot check freepointer while object is allocated if
> +	 * object and freepointer overlap.
> +	 */
> +	if (!freeptr_outside_object(s) && val == SLUB_RED_ACTIVE &&

Seems this condition should have been logically flipped?

> +	    !check_valid_pointer(s, slab, get_freepointer(s, p))) {
>  		object_err(s, slab, p, "Freepointer corrupt");
>  		/*
>  		 * No choice but to zap it and thus lose the remainder
> @@ -1370,9 +1368,14 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>  		 * another error because the object count is now wrong.
>  		 */
>  		set_freepointer(s, p, NULL);
> -		return 0;

Should set ret = 0 here?

>  	}
> -	return 1;
> +
> +	if (!ret && !slab_add_kunit_errors()) {

Also 5/6 of slub_kunit tests now fail as we increased the number of recorded
errors vs expected. Either the slab_add_kunit_errors() test above should
have a variant (parameter?) so it will only detect we are in slab-kunit test
(to suppress the printing and taint) but doesn't increase slab_errors (we
increased them for the individual issues already), or simply raise the
expectations of the tests so it matches the new implementation.

Thanks,
Vlastimil

> +		print_trailer(s, slab, object);
> +		add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> +	}
> +
> +	return ret;
>  }
>  
>  static int check_slab(struct kmem_cache *s, struct slab *slab)
>