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)
>
