linux-kernel - Re: [PATCH v2 1/3] slab: make check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <96f13b3c-ec74-4f87-be37-fbf796fe51eb@linux.dev>
Date: Fri, 7 Jun 2024 15:26:46 +0800
From: Chengming Zhou <chengming.zhou@...ux.dev>
To: Vlastimil Babka <vbabka@...e.cz>, Christoph Lameter <cl@...ux.com>,
 Pekka Enberg <penberg@...nel.org>, David Rientjes <rientjes@...gle.com>,
 Joonsoo Kim <iamjoonsoo.kim@....com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Roman Gushchin <roman.gushchin@...ux.dev>,
 Hyeonggon Yoo <42.hyeyoo@...il.com>, Feng Tang <feng.tang@...el.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 zhouchengming@...edance.com
Subject: Re: [PATCH v2 1/3] slab: make check_object() more consistent

On 2024/6/6 16:28, Vlastimil Babka wrote:
> 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.
>>
[...]
>> -	/* 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?

Ah, right, will fix.

> 
>> +	    !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?

Yes.

> 
>>  	}
>> -	return 1;
>> +
>> +	if (!ret && !slab_add_kunit_errors()) {
> 
> Also 5/6 of slub_kunit tests now fail as we increased the number of recorded

My bad, I didn't test with slub_kunit, will test later.

> 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

I think this way is simpler for me, only suppress the printing but doesn't
increase slab_errors, will take this way and test again.

Thanks!

> increased them for the individual issues already), or simply raise the
> expectations of the tests so it matches the new implementation.
>