linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] percpu: add basic double free check
@ 2025-12-20  0:27 Dennis Zhou
  2026-01-12 16:03 ` Sebastian Andrzej Siewior
  2026-01-14 12:22 ` Chris Mason
  0 siblings, 2 replies; 6+ messages in thread
From: Dennis Zhou @ 2025-12-20  0:27 UTC (permalink / raw)
  To: Tejun Heo, Christoph Lameter, Andrew Morton
  Cc: linux-mm, linux-kernel, Dennis Zhou, Sebastian Andrzej Siewior

This adds a basic double free check by validating the first bit of the
allocation in alloc_map and bound_map are set. If the alloc_map bit is
not set, then this means the area is currently unallocated. If the
bound_map bit is not set, then we are not freeing from the beginning of
the allocation.

This is a respin of [1] adding the requested changes from me and
Christoph.

[1] https://lore.kernel.org/linux-mm/20250904143514.Yk6Ap-jy@linutronix.de/

Signed-off-by: Dennis Zhou <dennis@kernel.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 mm/percpu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/mm/percpu.c b/mm/percpu.c
index 81462ce5866e..c9f8df6c34c3 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -79,6 +79,7 @@
 #include <linux/mutex.h>
 #include <linux/percpu.h>
 #include <linux/pfn.h>
+#include <linux/ratelimit.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/vmalloc.h>
@@ -1285,6 +1286,11 @@ static int pcpu_free_area(struct pcpu_chunk *chunk, int off)
 
 	bit_off = off / PCPU_MIN_ALLOC_SIZE;
 
+	/* check double free */
+	if (!test_bit(bit_off, chunk->alloc_map) ||
+	    !test_bit(bit_off, chunk->bound_map))
+		return 0;
+
 	/* find end index */
 	end = find_next_bit(chunk->bound_map, pcpu_chunk_map_bits(chunk),
 			    bit_off + 1);
@@ -2225,6 +2231,7 @@ static void pcpu_balance_workfn(struct work_struct *work)
  */
 void free_percpu(void __percpu *ptr)
 {
+	static DEFINE_RATELIMIT_STATE(_rs, 60 * HZ, DEFAULT_RATELIMIT_BURST);
 	void *addr;
 	struct pcpu_chunk *chunk;
 	unsigned long flags;
@@ -2242,6 +2249,13 @@ void free_percpu(void __percpu *ptr)
 
 	spin_lock_irqsave(&pcpu_lock, flags);
 	size = pcpu_free_area(chunk, off);
+	if (size == 0) {
+		spin_unlock_irqrestore(&pcpu_lock, flags);
+
+		if (__ratelimit(&_rs))
+			WARN(1, "percpu double free\n");
+		return;
+	}
 
 	pcpu_alloc_tag_free_hook(chunk, off, size);
 
-- 
2.43.0



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] percpu: add basic double free check
  2025-12-20  0:27 [PATCH] percpu: add basic double free check Dennis Zhou
@ 2026-01-12 16:03 ` Sebastian Andrzej Siewior
  2026-01-14 12:22 ` Chris Mason
  1 sibling, 0 replies; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2026-01-12 16:03 UTC (permalink / raw)
  To: Dennis Zhou
  Cc: Tejun Heo, Christoph Lameter, Andrew Morton, linux-mm, linux-kernel

On 2025-12-19 16:27:37 [-0800], Dennis Zhou wrote:
> This adds a basic double free check by validating the first bit of the
> allocation in alloc_map and bound_map are set. If the alloc_map bit is
> not set, then this means the area is currently unallocated. If the
> bound_map bit is not set, then we are not freeing from the beginning of
> the allocation.
> 
> This is a respin of [1] adding the requested changes from me and
> Christoph.

Thank you.

> [1] https://lore.kernel.org/linux-mm/20250904143514.Yk6Ap-jy@linutronix.de/
> 

Sebastian


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] percpu: add basic double free check
  2025-12-20  0:27 [PATCH] percpu: add basic double free check Dennis Zhou
  2026-01-12 16:03 ` Sebastian Andrzej Siewior
@ 2026-01-14 12:22 ` Chris Mason
  2026-01-15 23:13   ` Andrew Morton
  2026-01-16  2:38   ` Dennis Zhou
  1 sibling, 2 replies; 6+ messages in thread
From: Chris Mason @ 2026-01-14 12:22 UTC (permalink / raw)
  To: Dennis Zhou
  Cc: Chris Mason, Tejun Heo, Christoph Lameter, Andrew Morton,
	linux-mm, linux-kernel, Sebastian Andrzej Siewior

On Fri, 19 Dec 2025 16:27:37 -0800 Dennis Zhou <dennis@kernel.org> wrote:

> This adds a basic double free check by validating the first bit of the
> allocation in alloc_map and bound_map are set. If the alloc_map bit is
> not set, then this means the area is currently unallocated. If the
> bound_map bit is not set, then we are not freeing from the beginning of
> the allocation.

Hi Dennis,

I ran mm-new through the AI review prompts, and this one was flagged:

> diff --git a/mm/percpu.c b/mm/percpu.c
> --- a/mm/percpu.c
> +++ b/mm/percpu.c

[ ... ]

> @@ -1285,6 +1286,11 @@ static int pcpu_free_area(struct pcpu_chunk *chunk, int off)
>
> 	bit_off = off / PCPU_MIN_ALLOC_SIZE;
>
> +	/* check double free */
> +	if (!test_bit(bit_off, chunk->alloc_map) ||
> +	    !test_bit(bit_off, chunk->bound_map))
> +		return 0;

Does this early return corrupt pcpu_stats.nr_cur_alloc and
chunk->nr_alloc?

Looking at pcpu_free_area(), earlier in the function (before this new
check) there is a call to pcpu_stats_area_dealloc(chunk) which decrements
these counters:

    static int pcpu_free_area(struct pcpu_chunk *chunk, int off)
    {
        ...
        lockdep_assert_held(&pcpu_lock);
        pcpu_stats_area_dealloc(chunk);   <-- decrements counters here
        ...
        bit_off = off / PCPU_MIN_ALLOC_SIZE;

        /* check double free */
        if (!test_bit(bit_off, chunk->alloc_map) ||
            !test_bit(bit_off, chunk->bound_map))
            return 0;                     <-- returns without actual free
        ...
    }

When a double-free is detected:

  1. pcpu_stats_area_dealloc() runs, decrementing pcpu_stats.nr_cur_alloc
     and chunk->nr_alloc
  2. The check detects the double-free and returns 0
  3. No actual memory is freed, but the counters are now wrong

Should pcpu_stats_area_dealloc() be moved after the double-free check,
or should there be a compensating increment on the early return path?

This could also affect percpu_stats_show() in mm/percpu-stats.c, which
uses chunk->nr_alloc via find_max_nr_alloc() to size a buffer:

    buffer = vmalloc_array(2 * max_nr_alloc + 1, sizeof(int));

If nr_alloc is underreported due to this bug, the buffer may be undersized
for the actual number of allocations tracked in alloc_map, which
chunk_map_stats() iterates based on the actual bitmap contents.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] percpu: add basic double free check
  2026-01-14 12:22 ` Chris Mason
@ 2026-01-15 23:13   ` Andrew Morton
  2026-01-16  2:56     ` Dennis Zhou
  2026-01-16  2:38   ` Dennis Zhou
  1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2026-01-15 23:13 UTC (permalink / raw)
  To: Chris Mason
  Cc: Dennis Zhou, Tejun Heo, Christoph Lameter, linux-mm,
	linux-kernel, Sebastian Andrzej Siewior

On Wed, 14 Jan 2026 04:22:00 -0800 Chris Mason <clm@meta.com> wrote:

> On Fri, 19 Dec 2025 16:27:37 -0800 Dennis Zhou <dennis@kernel.org> wrote:
> 
> > This adds a basic double free check by validating the first bit of the
> > allocation in alloc_map and bound_map are set. If the alloc_map bit is
> > not set, then this means the area is currently unallocated. If the
> > bound_map bit is not set, then we are not freeing from the beginning of
> > the allocation.
> 
> Hi Dennis,
> 
> I ran mm-new through the AI review prompts, and this one was flagged:

So good.

> If nr_alloc is underreported due to this bug, the buffer may be undersized
> for the actual number of allocations tracked in alloc_map, which
> chunk_map_stats() iterates based on the actual bitmap contents.

That's pretty serious, although unlikely to occur.  I'll drop this
version of the patch, thanks.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] percpu: add basic double free check
  2026-01-14 12:22 ` Chris Mason
  2026-01-15 23:13   ` Andrew Morton
@ 2026-01-16  2:38   ` Dennis Zhou
  1 sibling, 0 replies; 6+ messages in thread
From: Dennis Zhou @ 2026-01-16  2:38 UTC (permalink / raw)
  To: Chris Mason
  Cc: Tejun Heo, Christoph Lameter, Andrew Morton, linux-mm,
	linux-kernel, Sebastian Andrzej Siewior

Hi Chris,

On Wed, Jan 14, 2026 at 04:22:00AM -0800, Chris Mason wrote:
> On Fri, 19 Dec 2025 16:27:37 -0800 Dennis Zhou <dennis@kernel.org> wrote:
> 
> > This adds a basic double free check by validating the first bit of the
> > allocation in alloc_map and bound_map are set. If the alloc_map bit is
> > not set, then this means the area is currently unallocated. If the
> > bound_map bit is not set, then we are not freeing from the beginning of
> > the allocation.
> 
> Hi Dennis,
> 
> I ran mm-new through the AI review prompts, and this one was flagged:
> 

Wow that's cool, thanks!

> > diff --git a/mm/percpu.c b/mm/percpu.c
> > --- a/mm/percpu.c
> > +++ b/mm/percpu.c
> 
> [ ... ]
> 
> > @@ -1285,6 +1286,11 @@ static int pcpu_free_area(struct pcpu_chunk *chunk, int off)
> >
> > 	bit_off = off / PCPU_MIN_ALLOC_SIZE;
> >
> > +	/* check double free */
> > +	if (!test_bit(bit_off, chunk->alloc_map) ||
> > +	    !test_bit(bit_off, chunk->bound_map))
> > +		return 0;
> 
> Does this early return corrupt pcpu_stats.nr_cur_alloc and
> chunk->nr_alloc?
> 
> Looking at pcpu_free_area(), earlier in the function (before this new
> check) there is a call to pcpu_stats_area_dealloc(chunk) which decrements
> these counters:
> 
>     static int pcpu_free_area(struct pcpu_chunk *chunk, int off)
>     {
>         ...
>         lockdep_assert_held(&pcpu_lock);
>         pcpu_stats_area_dealloc(chunk);   <-- decrements counters here
>         ...
>         bit_off = off / PCPU_MIN_ALLOC_SIZE;
> 
>         /* check double free */
>         if (!test_bit(bit_off, chunk->alloc_map) ||
>             !test_bit(bit_off, chunk->bound_map))
>             return 0;                     <-- returns without actual free
>         ...
>     }
> 
> When a double-free is detected:
> 
>   1. pcpu_stats_area_dealloc() runs, decrementing pcpu_stats.nr_cur_alloc
>      and chunk->nr_alloc
>   2. The check detects the double-free and returns 0
>   3. No actual memory is freed, but the counters are now wrong
> 
> Should pcpu_stats_area_dealloc() be moved after the double-free check,
> or should there be a compensating increment on the early return path?

I moved pcpu_stats_area_dealloc() to the end of the function in v2.

> 
> This could also affect percpu_stats_show() in mm/percpu-stats.c, which
> uses chunk->nr_alloc via find_max_nr_alloc() to size a buffer:
> 
>     buffer = vmalloc_array(2 * max_nr_alloc + 1, sizeof(int));
> 
> If nr_alloc is underreported due to this bug, the buffer may be undersized
> for the actual number of allocations tracked in alloc_map, which
> chunk_map_stats() iterates based on the actual bitmap contents.
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] percpu: add basic double free check
  2026-01-15 23:13   ` Andrew Morton
@ 2026-01-16  2:56     ` Dennis Zhou
  0 siblings, 0 replies; 6+ messages in thread
From: Dennis Zhou @ 2026-01-16  2:56 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Chris Mason, Tejun Heo, Christoph Lameter, linux-mm,
	linux-kernel, Sebastian Andrzej Siewior

Hi Andrew,

On Thu, Jan 15, 2026 at 03:13:28PM -0800, Andrew Morton wrote:
> On Wed, 14 Jan 2026 04:22:00 -0800 Chris Mason <clm@meta.com> wrote:
> 
> > On Fri, 19 Dec 2025 16:27:37 -0800 Dennis Zhou <dennis@kernel.org> wrote:
> > 
> > > This adds a basic double free check by validating the first bit of the
> > > allocation in alloc_map and bound_map are set. If the alloc_map bit is
> > > not set, then this means the area is currently unallocated. If the
> > > bound_map bit is not set, then we are not freeing from the beginning of
> > > the allocation.
> > 
> > Hi Dennis,
> > 
> > I ran mm-new through the AI review prompts, and this one was flagged:
> 
> So good.
> 
> > If nr_alloc is underreported due to this bug, the buffer may be undersized
> > for the actual number of allocations tracked in alloc_map, which
> > chunk_map_stats() iterates based on the actual bitmap contents.
> 
> That's pretty serious, although unlikely to occur.  I'll drop this
> version of the patch, thanks.
> 

It's a good catch by AI, I fixed it in v2 [1].

My thoughts are this is really to serve developers. It's not really a
recoverable error because if the pointer is re-allocated before the
double free, then you'll only find out in the future when the new owner
tries to free memory that it doesn't own. An even more contrived case is
the pages get freed out from under it and then the new owner seg faults.

Thanks,
Dennis


[1] https://lore.kernel.org/lkml/20260116023216.14515-1-dennis@kernel.org/


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-01-16  2:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-12-20  0:27 [PATCH] percpu: add basic double free check Dennis Zhou
2026-01-12 16:03 ` Sebastian Andrzej Siewior
2026-01-14 12:22 ` Chris Mason
2026-01-15 23:13   ` Andrew Morton
2026-01-16  2:56     ` Dennis Zhou
2026-01-16  2:38   ` Dennis Zhou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox