linux-kernel - Re: [PATCH v2] debug: Fix a mixed use of NULL and error 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: <aRY2Qyl8PJ5Gaga4@google.com>
Date: Fri, 14 Nov 2025 03:49:23 +0800
From: Kuan-Wei Chiu <visitorckw@...il.com>
To: Haotian Zhang <vulab@...as.ac.cn>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] debug: Fix a mixed use of NULL and error pointers

Hi Haotian,

On Tue, Nov 11, 2025 at 10:15:21AM +0800, Haotian Zhang wrote:
> The lookup_object_or_alloc() function currently returns either error
> pointers (ERR_PTR(-ENOENT)) or NULL on allocation failure. Mixing error
> pointers and NULL is confusing and makes the code harder to maintain.
> Change lookup_object_or_alloc() to consistently return error pointers
> for all error cases by returning ERR_PTR(-ENOMEM) instead of NULL when
> allocation fails.
> 
> Update all three call sites (__debug_object_init, debug_object_activate,
> and debug_object_assert_init) to use IS_ERR() for error checking and
> handle -ENOMEM by calling debug_objects_oom().

The code change itself LGTM.

However, the v2 commit message removed the previous description about
the potential for dereferencing an error pointer. This removal shifts
the narrative, making it appear more like a cleanup patch than a
necessary bug fix.

I'll recommend you reinstate a clear explanation of why the previous
behavior was buggy.

Regards,
Kuan-Wei

> 
> Fixes: 63a759694eed ("debugobject: Prevent init race with static objects")
> Suggested-by: Kuan-Wei Chiu <visitorckw@...il.com>
> Signed-off-by: Haotian Zhang <vulab@...as.ac.cn>
> ---
> Changes in v2:
>   -Make error handling consistent across all call sites as suggested
>    by Kuan-Wei Chiu
> ---
>  lib/debugobjects.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index 7f50c4480a4e..d2d1979e2d12 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -691,7 +691,7 @@ static struct debug_obj *lookup_object_or_alloc(void *addr, struct debug_bucket
>  
>  	/* Out of memory. Do the cleanup outside of the locked region */
>  	debug_objects_enabled = false;
> -	return NULL;
> +	return ERR_PTR(-ENOMEM);
>  }
>  
>  static void debug_objects_fill_pool(void)
> @@ -741,9 +741,10 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
>  	raw_spin_lock_irqsave(&db->lock, flags);
>  
>  	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
> -	if (unlikely(!obj)) {
> +	if (IS_ERR(obj)) {
>  		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		debug_objects_oom();
> +		if (PTR_ERR(obj) == -ENOMEM)
> +			debug_objects_oom();
>  		return;
>  	}
>  
> @@ -818,11 +819,13 @@ int debug_object_activate(void *addr, const struct debug_obj_descr *descr)
>  	raw_spin_lock_irqsave(&db->lock, flags);
>  
>  	obj = lookup_object_or_alloc(addr, db, descr, false, true);
> -	if (unlikely(!obj)) {
> -		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		debug_objects_oom();
> -		return 0;
> -	} else if (likely(!IS_ERR(obj))) {
> +	if (IS_ERR(obj)) {
> +		if (PTR_ERR(obj) == -ENOMEM) {
> +			raw_spin_unlock_irqrestore(&db->lock, flags);
> +			debug_objects_oom();
> +			return 0;
> +		}
> +	} else {
>  		switch (obj->state) {
>  		case ODEBUG_STATE_ACTIVE:
>  		case ODEBUG_STATE_DESTROYED:
> @@ -1007,11 +1010,11 @@ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr)
>  	raw_spin_lock_irqsave(&db->lock, flags);
>  	obj = lookup_object_or_alloc(addr, db, descr, false, true);
>  	raw_spin_unlock_irqrestore(&db->lock, flags);
> -	if (likely(!IS_ERR_OR_NULL(obj)))
> +	if (!IS_ERR(obj))
>  		return;
>  
>  	/* If NULL the allocation has hit OOM */
> -	if (!obj) {
> +	if (PTR_ERR(obj) == -ENOMEM) {
>  		debug_objects_oom();
>  		return;
>  	}
> -- 
> 2.50.1.windows.1
>