linux-kernel - Re: [PATCH 1/2] kmemleak: enable tracking for percpu pointers

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 for Android: free password hash cracker in your pocket

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

Message-ID: <baad00ff-6450-4a1b-b62a-3fabdb35beb3@virtuozzo.com>
Date: Wed, 31 Jul 2024 10:30:35 +0800
From: Pavel Tikhomirov <ptikhomirov@...tuozzo.com>
To: Catalin Marinas <catalin.marinas@....com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, Wei Yongjun <weiyongjun1@...wei.com>,
 Chen Jun <chenjun102@...wei.com>,
 Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@...onical.com>,
 kernel@...nvz.org
Subject: Re: [PATCH 1/2] kmemleak: enable tracking for percpu pointers



On 7/30/24 22:15, Catalin Marinas wrote:
> Hi Pavel,
> 
> On Thu, Jul 25, 2024 at 12:12:15PM +0800, Pavel Tikhomirov wrote:
>> @@ -1308,12 +1319,23 @@ static bool update_checksum(struct kmemleak_object *object)
>>   {
>>   	u32 old_csum = object->checksum;
>>   
>> -	if (WARN_ON_ONCE(object->flags & (OBJECT_PHYS | OBJECT_PERCPU)))
>> +	if (WARN_ON_ONCE(object->flags & OBJECT_PHYS))
>>   		return false;
>>   
>>   	kasan_disable_current();
>>   	kcsan_disable_current();
>> -	object->checksum = crc32(0, kasan_reset_tag((void *)object->pointer), object->size);
>> +	if (object->flags & OBJECT_PERCPU) {
>> +		unsigned int cpu;
>> +
>> +		object->checksum = 0;
>> +		for_each_possible_cpu(cpu) {
>> +			void *ptr = per_cpu_ptr((void __percpu *)object->pointer, cpu);
>> +
>> +			object->checksum ^= crc32(0, kasan_reset_tag((void *)ptr), object->size);
>> +		}
> 
> Slight worry this may take too long for large-ish objects with a large
> number of CPUs. But we can revisit if anyone complains.
> 
>> +	} else {
>> +		object->checksum = crc32(0, kasan_reset_tag((void *)object->pointer), object->size);
>> +	}
>>   	kasan_enable_current();
>>   	kcsan_enable_current();
>>   
>> @@ -1365,6 +1387,64 @@ static int scan_should_stop(void)
>>   	return 0;
>>   }
>>   
>> +static void scan_pointer(struct kmemleak_object *scanned,
>> +			 unsigned long pointer, unsigned int objflags)
> 
> Nitpick: I'd have called this lookup_pointer or something like that.
> When I first saw it, I tried to figure out why it doesn't have a size
> argument but it became clear that it's not actually scanning/reading the
> location at 'pointer' but simply looking the value up in various trees.
> Up to you if you want to change the name to something else. The patch
> looks good.

Yes, I agree "scan_" is a bit too confusing. Let's try 
"pointer_update_refs", as it is more self-explanatory, and represents 
what the function does. The "lookup_pointer" seems like a function which 
should return a pointer, e.g. like "lookup_object" returns an object, 
but we can always switch to "lookup_pointer" if you like.

I will send v2 with this change shortly.

> 
> Reviewed-by: Catalin Marinas <catalin.marinas@....com>
> 
> Thanks.
> 

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.