Re: [PATCH -next 3/5] mm/kmemleak: Add support for percpu memory leak detect

[Catalin Marinas <catalin.marinas@arm.com>]

On Mon, Sep 28, 2020 at 02:08:29PM +0000, chenjun (AM) wrote:
> On Mon, Sep 21, 2020 at 02:00:05AM +0000, Chen Jun wrote:
> > From: Wei Yongjun <weiyongjun1@huawei.com>
> >> @@ -651,6 +672,19 @@ static void create_object(unsigned long ptr, size_t size, int min_count,
> >>   	raw_spin_unlock_irqrestore(&kmemleak_lock, flags);
> >>   }
> >>   
> >> +static void create_object(unsigned long ptr, size_t size, int min_count,
> >> +			  gfp_t gfp)
> >> +{
> >> +	__create_object(ptr, size, min_count, 0, gfp);
> >> +}
> >> +
> >> +static void create_object_percpu(unsigned long ptr, size_t size, int min_count,
> >> +				 gfp_t gfp)
> >> +{
> >> +	__create_object(ptr, size, min_count, OBJECT_PERCPU | OBJECT_NO_SCAN,
> >> +			gfp);
> >> +}
> >> +
> >>   /*
> >>    * Mark the object as not allocated and schedule RCU freeing via put_object().
> >>    */
> >> @@ -912,10 +946,12 @@ void __ref kmemleak_alloc_percpu(const void __percpu *ptr, size_t size,
> >>   	 * Percpu allocations are only scanned and not reported as leaks
> >>   	 * (min_count is set to 0).
> >>   	 */
> >> -	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
> >> +	if (kmemleak_enabled && ptr && !IS_ERR(ptr)) {
> >>   		for_each_possible_cpu(cpu)
> >>   			create_object((unsigned long)per_cpu_ptr(ptr, cpu),
> >>   				      size, 0, gfp);
> >> +		create_object_percpu((unsigned long)ptr, size, 1, gfp);
> >> +	}
> >>   }
> > 
> > A concern I have here is that ptr may overlap with an existing object
> > and the insertion in the rb tree will fail. For example, with !SMP,
> > ptr == per_cpu_ptr(ptr, 0), so create_object() will fail and kmemleak
> > gets disabled.
> > 
> > An option would to figure out how to allow overlapping ranges with rb
> > tree (or find a replacement for it if not possible).
> > 
> > Another option would be to have an additional structure to track the
> > __percpu pointers since they have their own range. If size is not
> > relevant, maybe go for an xarray, otherwise another rb tree (do we have
> > any instance of pointers referring some inner member of a __percpu
> > object?). The scan_object() function will have to search two trees.
> 
> I would like to use CONFIG_SMP to seprate code:
> if SMP, we will create some objects for per_cpu_ptr(ptr, cpu) and an 
> object with OBJECT_NO_ACCESS for ptr.
> if !SMP, we will not create object for per_cpu_ptr(ptr,cpu), but an 
> object without OBJECT_NO_ACCESS for ptr will be created.
> What do you think about this opinion.

The !SMP case was just an example. Do you have a guarantee that the
value of the __percpu ptr doesn't clash with a linear map address?

-- 
Catalin