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: <ZS_81Oy5S-VeLrT3@arm.com>
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ