Re: + radix-tree-fix-kmemleak-false-positives-on-tree-head-reassignment.patch added to mm-new branch

[Breno Leitao <leitao@debian.org>]

Hello Catalin,

First of all, thanks for th
On Tue, Jul 07, 2026 at 12:19:50AM +0100, Catalin Marinas wrote:
> On Mon, Jul 06, 2026 at 05:25:25PM +0100, Catalin Marinas wrote:
> > On Mon, Jul 06, 2026 at 07:53:30AM -0700, Breno Leitao wrote:
> > > On Mon, Jul 06, 2026 at 12:39:30PM +0100, Catalin Marinas wrote:
> > > > I wonder whether we should force the scanning to happen twice in a row
> > > > and drop the min_unref_count. Those transient leaks happen because of
> > > > some micro/milliseconds miss of a pointer. If we have new white objects
> > > > of the end of a scan, go one more round through the root and gray
> > > > objects (but do not reset them to white) and only then report the leaks.
> > > > If the white objects have been reported already or we don't have any
> > > > left, skip this additional scan or bail out early. We could have a
> > > > tunable for this one to go 2-3 times if needed, though I guess twice is
> > > > sufficient. The interface is also preserved as you do an echo scan only
> > > > once (or twice initially with the checksum calculation).
> > > 
> > > That is a good proposal, and I am happy to hack it up.
> > > 
> > > On the other side, I _think_ we want to have both approaches
> > > (your rescan-after-white) and min_unref_count. They serve different
> > > purposes. This is how I see them serving different purposes:
> > > 
> > > 1) This rescan-after-white proposal:
> > > 
> > >   Target: Developers that cat manually scanning for leaks when they
> > >   develop something.
> > > 
> > >     a) The goal is to produce a memory leaks after the scan is done.
> > >     b) Latency is more important than false positives
> > >     c) min_unref_count = 1
> > > 
> > > 2) min_unref_count 
> > > 
> > >   Target: Production servers running kmemleak on some cloud "probe
> > >   points", where the service will run for hours/days.
> > > 
> > >     a) Latency is not important (system is automatically deployed and
> > >     tested)
> > >     b) False positives is heavily undesirable. It causes an alarm to get
> > >     some engineer to investigate.
> > >     c) In this case min_unref_count will be super high (>10)
> > >       - I.e, just report when you are pretty sure this is a real issue.
> > >    
> > > Anyway, that's what I'm seeing from my angle. Let me know if I'm way
> > > off.
> > 
> > You are right. If you only ever use min_unref_count of 2, then the first
> > option might be alright but for larger numbers, you can't just keep
> > scanning 10 times in a row. If option 1 works, we might be able to get
> > rid of the transient leak annotations.
> > 
> > I got Claude to refactor for the first idea and it mostly works. For
> > some reason, after modprobe kmemleak-test, it always does the
> > confirmation scan. There's an object (vmalloc) left that's reported as
> > a potential leak candidate but not confirmed in the subsequent scan.
> > I'll check tomorrow, need to finish the day early.
> 
> I found the issue. It was the passing of the excess_ref on the
> subsequent scan. I thought I could avoid marking gray objects as white
> again in the second scan but it messes up the excess_ref since they are
> counted only after the object became gray. I had to add a flag,
> OBJECT_SUSPECT, since we mark all objects white again for the second
> pass. In principle, it's not different from your two scans approach,
> only that they are done back to back.
> 
> Anyway, a better diff for the first idea below. I need to do more
> testing and can turn it into a proper commit (if we don't deem it
> redundant because of the other min_unref_count).

Thanks for it. I've reviewed it and it looks sane.

I am also testing it on my side.


> +static bool flag_suspects(void)
> +{
> +	struct kmemleak_object *object;
> +	int suspects = 0;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(object, &object_list, object_list) {

__kmemleak_scan() above alread iterated over these objects. Is it
possible to piggy back on __kmemleak_scan() list_for_each_entry_rcu
forthe objects to flag SUSPECTS?

> +		raw_spin_lock_irq(&object->lock);
> +		if (unreferenced_object(object) &&
> +		    !(object->flags & OBJECT_REPORTED)) {
> +			object->flags |= OBJECT_SUSPECT;
> +			suspects++;
> +		} else {
> +			object->flags &= ~OBJECT_SUSPECT;
> +		}
> +		raw_spin_unlock_irq(&object->lock);
> +		if (need_resched())
> +			kmemleak_cond_resched(object);
> +	}
> +	rcu_read_unlock();
> +
> +	return suspects != 0;
> +}
> +
> +/*
> + * Scan the memory and report the unreferenced objects as leaks. Must be
> + * called with the scan_mutex held.
> + */
> +static void kmemleak_scan(void)
> +{
> +	struct kmemleak_object *object;
> +	struct xarray dedup;
> +	int new_leaks = 0;

It seems new_leaks and dedup are not used at all.


reviewing this code, I found that unreferenced_object(object)) is always
called/checked with (object->flags & OBJECT_REPORTED). Would it be ok to
move (object->flags & OBJECT_REPORTED) to inside unreferenced_object()?

Thanks for this work,
--breno