linux-kernel - Re: [PATCH v3 4/7] mm: kmemleak: split __create

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]

Date:   Wed, 18 Oct 2023 16:42:12 +0100
From:   Catalin Marinas <catalin.marinas@....com>
To:     Liu Shixin <liushixin2@...wei.com>
Cc:     Patrick Wang <patrick.wang.shcn@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kefeng Wang <wangkefeng.wang@...wei.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/7] mm: kmemleak: split __create_object into two
 functions

Thanks for this, it looks better.

On Wed, Oct 18, 2023 at 06:29:49PM +0800, Liu Shixin wrote:
> -/*
> - * Create the metadata (struct kmemleak_object) corresponding to an allocated
> - * memory block and add it to the object_list and object_tree_root (or
> - * object_phys_tree_root).
> - */
> -static void __create_object(unsigned long ptr, size_t size,
> -			    int min_count, gfp_t gfp, bool is_phys)
> +static struct kmemleak_object * __alloc_object(gfp_t gfp)
>  {
> -	unsigned long flags;
> -	struct kmemleak_object *object, *parent;
> -	struct rb_node **link, *rb_parent;
> -	unsigned long untagged_ptr;
> -	unsigned long untagged_objp;
> +	struct kmemleak_object *object;
>  
>  	object = mem_pool_alloc(gfp);
>  	if (!object) {
>  		pr_warn("Cannot allocate a kmemleak_object structure\n");
>  		kmemleak_disable();
> -		return;
> +		return NULL;
>  	}
>  
>  	INIT_LIST_HEAD(&object->object_list);
> @@ -649,13 +639,8 @@ static void __create_object(unsigned long ptr, size_t size,
>  	INIT_HLIST_HEAD(&object->area_list);
>  	raw_spin_lock_init(&object->lock);
>  	atomic_set(&object->use_count, 1);
> -	object->flags = OBJECT_ALLOCATED | (is_phys ? OBJECT_PHYS : 0);
> -	object->pointer = ptr;
> -	object->size = kfence_ksize((void *)ptr) ?: size;
>  	object->excess_ref = 0;
> -	object->min_count = min_count;
>  	object->count = 0;			/* white color initially */
> -	object->jiffies = jiffies;
>  	object->checksum = 0;
>  	object->del_state = 0;

I'd keep all the initialisation in one place even if it means passing
more arguments to __alloc_object(). It feels a bit weird and error prone
to split the initialisation in two places. Otherwise I'm fine with the
split.

-- 
Catalin