1. Patchew
  2. linux
  3. View series

[PATCH] debug: Fix a NULL vs IS_ERR() bug in __debug_object_init()

Haotian Zhang posted 1 patch 1 month, 1 week ago 
lib/debugobjects.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
[PATCH] debug: Fix a NULL vs IS_ERR() bug in __debug_object_init()
Posted by Haotian Zhang 1 month, 1 week ago 
The lookup_object_or_alloc() returns error pointers on failure, but the
code only checks for NULL. This leads to dereferencing an invalid error
pointer and causes a kernel crash.

Use IS_ERR_OR_NULL() instead of a NULL check to properly handle both
error pointers and NULL returns.

Fixes: 63a759694eed ("debugobject: Prevent init race with static objects")
Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
---
 lib/debugobjects.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 7f50c4480a4e..9587ef619054 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -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_OR_NULL(obj)) {
 		raw_spin_unlock_irqrestore(&db->lock, flags);
-		debug_objects_oom();
+		if (!obj)
+			debug_objects_oom();
 		return;
 	}
 
-- 
2.50.1.windows.1
Re: [PATCH] debug: Fix a NULL vs IS_ERR() bug in __debug_object_init()
Posted by Kuan-Wei Chiu 1 month, 1 week ago 
Hi Haotian,

On Mon, Nov 10, 2025 at 03:57:46PM +0800, Haotian Zhang wrote:
> The lookup_object_or_alloc() returns error pointers on failure, but the
> code only checks for NULL. This leads to dereferencing an invalid error
> pointer and causes a kernel crash.
> 
> Use IS_ERR_OR_NULL() instead of a NULL check to properly handle both
> error pointers and NULL returns.
> 
> Fixes: 63a759694eed ("debugobject: Prevent init race with static objects")
> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
> ---
>  lib/debugobjects.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index 7f50c4480a4e..9587ef619054 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -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_OR_NULL(obj)) {

Ideally, an API should either return error pointers to indicate errors
or solely return a NULL pointer to represent a failed operation. Mixing
error pointers and NULL pointers can easily lead to confusion about
what each represents.

In this case, it seems that lookup_object_or_alloc() only returns NULL
when alloc_object() fails due to being out of memory. Perhaps a better
approach would be to return -ENOMEM in this situation, then change the
check in __debug_object_init() to use IS_ERR(), and call
debug_objects_oom() when obj == ERR_PTR(-ENOMEM). I think this might
make the code clearer.

Regards,
Kuan-Wei

>  		raw_spin_unlock_irqrestore(&db->lock, flags);
> -		debug_objects_oom();
> +		if (!obj)
> +			debug_objects_oom();
>  		return;
>  	}
>  
> -- 
> 2.50.1.windows.1
> 
>
[PATCH v3] debugobjects: Fix inconsistent return handling and potential ERR_PTR dereference
Posted by Haotian Zhang 1 month ago 
The lookup_object_or_alloc() function can return NULL on memory
allocation failure, while returning an error pointer for other errors.
Call sites such as __debug_object_init() and debug_object_activate()
only check for errors using IS_ERR(), which does not evaluate to true
for a NULL pointer. This can lead to a NULL pointer dereference if
memory allocation fails.

To fix this, modify lookup_object_or_alloc() to consistently return
ERR_PTR(-ENOMEM) on allocation failure. This ensures that all error
paths return a valid error pointer that can be caught by IS_ERR().
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(), thus preventing the
potential bug and making the error handling robust and consistent.

Fixes: 63a759694eed ("debugobject: Prevent init race with static objects")
Suggested-by: Kuan-Wei Chiu <visitorckw@gmail.com>
Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
---
Changes in v3:
  -Reword commit message to clearly describe the NULL pointer
   dereference bug, as suggested by Kuan-Wei Chiu.
  -Change subject prefix to "debugobjects:".
---
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
Re: [PATCH v3] debugobjects: Fix inconsistent return handling and potential ERR_PTR dereference
Posted by Thomas Gleixner 1 month ago 
On Fri, Nov 14 2025 at 09:56, Haotian Zhang wrote:
> The lookup_object_or_alloc() function can return NULL on memory
> allocation failure, while returning an error pointer for other errors.
> Call sites such as __debug_object_init() and debug_object_activate()
> only check for errors using IS_ERR(), which does not evaluate to true
> for a NULL pointer. This can lead to a NULL pointer dereference if
> memory allocation fails.

Nice fairy tale. Let's look at the facts.

__debug_object_init():
	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
	if (unlikely(!obj)) {
           ....

Does not use IS_ERR() at all and _is_ completely correct because
lookup_object_or_alloc() can only return NULL or a valid object but
never an error pointer because the 'alloc_ifstatic' argument is NULL.

debug_object_activate():
	obj = lookup_object_or_alloc(addr, db, descr, false, true);
	if (unlikely(!obj)) {
           ....
	} else if (likely(!IS_ERR(obj))) {
           ....

handles both the NULL pointer and the error pointer case correctly.

I have no idea which code you were analyzing or which tool halluzinated
about it.

> Fixes: 63a759694eed ("debugobject: Prevent init race with static objects")

There is nothing broken, so this fixes nothing.

Thanks,

        tglx
Re: [PATCH v3] debugobjects: Fix inconsistent return handling and potential ERR_PTR dereference
Posted by Thomas Gleixner 1 month ago 
On Sun, Nov 16 2025 at 00:18, Thomas Gleixner wrote:
> On Fri, Nov 14 2025 at 09:56, Haotian Zhang wrote:
>> The lookup_object_or_alloc() function can return NULL on memory
>> allocation failure, while returning an error pointer for other errors.
>> Call sites such as __debug_object_init() and debug_object_activate()
>> only check for errors using IS_ERR(), which does not evaluate to true
>> for a NULL pointer. This can lead to a NULL pointer dereference if
>> memory allocation fails.
>
> Nice fairy tale. Let's look at the facts.
>
> __debug_object_init():
> 	obj = lookup_object_or_alloc(addr, db, descr, onstack, false);
> 	if (unlikely(!obj)) {
>            ....
>
> Does not use IS_ERR() at all and _is_ completely correct because
> lookup_object_or_alloc() can only return NULL or a valid object but
> never an error pointer because the 'alloc_ifstatic' argument is NULL.
>
> debug_object_activate():
> 	obj = lookup_object_or_alloc(addr, db, descr, false, true);
> 	if (unlikely(!obj)) {
>            ....
> 	} else if (likely(!IS_ERR(obj))) {
>            ....
>
> handles both the NULL pointer and the error pointer case correctly.
>
> I have no idea which code you were analyzing or which tool halluzinated
> about it.

That said. You clearly failed to explain how you found that. I'm well
aware that you are deeply involved in LLM based code analysis, so don't
tell me that reviewing random code is your new hobby.

Thanks,

        tglx
[PATCH v2] debug: Fix a mixed use of NULL and error pointers
Posted by Haotian Zhang 1 month, 1 week ago 
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().

Fixes: 63a759694eed ("debugobject: Prevent init race with static objects")
Suggested-by: Kuan-Wei Chiu <visitorckw@gmail.com>
Signed-off-by: Haotian Zhang <vulab@iscas.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
Re: [PATCH v2] debug: Fix a mixed use of NULL and error pointers
Posted by Kuan-Wei Chiu 1 month ago 
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().
> 
> Fixes: 63a759694eed ("debugobject: Prevent init race with static objects")
> Suggested-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>

Nit: The majority of the past git commit history uses debugobjects: as
the subject line prefix rather than debug:

Regards,
Kuan-Wei
Re: [PATCH v2] debug: Fix a mixed use of NULL and error pointers
Posted by Kuan-Wei Chiu 1 month ago 
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@gmail.com>
> Signed-off-by: Haotian Zhang <vulab@iscas.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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.