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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Zqj1eByCCStxAKNJ@arm.com>
Date: Tue, 30 Jul 2024 15:15:20 +0100
From: Catalin Marinas <catalin.marinas@....com>
To: Pavel Tikhomirov <ptikhomirov@...tuozzo.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

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.

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

Thanks.

-- 
Catalin