Re: [merged] kasan-fix-object-remain-in-offline-per-cpu-quarantine.patch removed from -mm tree

[Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>]

On Tue, 2020-12-15 at 17:06 +0800, Kuan-Ying Lee wrote:
> On Mon, 2020-12-14 at 10:19 -0800, Andrew Morton wrote:
> > On Mon, 14 Dec 2020 13:46:50 +0800 Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> wrote:
> > 
> > > On Sat, 2020-12-12 at 22:05 -0800, akpm@linux-foundation.org wrote:
> > > > The patch titled
> > > >      Subject: kasan: fix object remaining in offline per-cpu quarantine
> > > > has been removed from the -mm tree.  Its filename was
> > > >      kasan-fix-object-remain-in-offline-per-cpu-quarantine.patch
> > > > 
> > > > This patch was dropped because it was merged into mainline or a subsystem tree
> > > > 
> > > 
> > > Hi Andrew,
> > > 
> > > Sorry to bother.
> > > This patch has dependency with two patches of Andrey's patch series as
> > > below.
> > > "kasan: rename get_alloc/free_info"
> > > "kasan: sanitize objects when metadata doesnt fit"
> > 
> > Are you sure?  Please check 5.10 and if there are problems there,
> > please propose a standalone fix.
> > 
> 
> Yes.
> 
> Andrey's patch has the return value and return false.
> Return false will make slab allocator free the object and qlink_free()
> also free the object, so Qiang remove the qlink_free() to resolve the
> double free as below.
> https://lore.kernel.org/linux-mm/20201204102206.20237-1-qiang.zhang@windriver.com/
> 
>  	q = this_cpu_ptr(&cpu_quarantine);
>  	if (q->offline) {
> -		qlink_free(&meta->quarantine_link, cache); // free once
>  		local_irq_restore(flags);
>  		return false;  // free twice
>  	}
> 
> 
> But if removing qlink_free() without Andrey's patch, this 
> object will not be freed. It will cause memory leak as below.
> 
>  	q = this_cpu_ptr(&cpu_quarantine);
>  	if (q->offline) {
>  		local_irq_restore(flags);
>  		return;
>  	}
> 
> Thus, before applying Andrey's patch, we still need qlink_free().
> I will prepare a standalone fix to add qlink_free() back.
> 
> Thanks.
> 

Hi Andrew,

I upload the v2 standalone fixup patch to fix the memory leak issue as
below.
https://marc.info/?l=linux-mm&m=160820751825252&w=2
I think this slab memory leak issue is important. It's because when we
do kmem_cache_destroy, it will report object remaining error.

Add this v2 patch to mm-tree, it will have conflicts with
Andrey's patches as below.
"kasan: rename get_alloc/free_info"
"kasan: sanitize objects when metadata doesnt fit"

I think this standalone fixup patch should be added before Andrey's
patch in mm-tree. Because only merging this standalone fix patch to 5.10
stable, we can resolve this leak issue instead of merging the whole 
patchset of Andrey's patch to 5.10 stable.
However, merging the fixup patch into mm-tree will cause some conflicts
in mm-tree.

Please help to fix the conflicts.
And I think the conflict between standalone fixup patch and
Andrey's patches will be fixed as below.

I think this patch "kasan: rename get_alloc/free_info" need to rename
the "info" to "meta" as below.

-       qlink_free(&info->quarantine_link, cache);
+       qlink_free(&meta->quarantine_link, cache);


This patch "kasan: sanitize objects when metadata doesnt fit" need to
remove the qlink_free() and add return false as below.

	q = this_cpu_ptr(&cpu_quarantine);
	if (q->offline) {
-		qlink_free(&meta->quarantine_link, cache);
 		local_irq_restore(flags);
- 		return;
+ 		return false;
	}

Thanks a lot.