Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 19 Jul 2022 23:03:15 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        Robin Murphy <robin.murphy@arm.com>,
        John Garry <john.garry@huawei.com>
Subject: Re: [PATCH v1] mm/slub: enable debugging memory wasting of kmalloc
Message-ID: <20220719150315.GB56558@shbuild999.sh.intel.com>
References: <20220701135954.45045-1-feng.tang@intel.com>
 <41763154-f923-ae99-55c0-0f3717636779@suse.cz>
 <20220713073642.GA69088@shbuild999.sh.intel.com>
 <45906408-34ce-4b79-fbe4-768335ffbf96@suse.cz>
 <20220715082922.GA88035@shbuild999.sh.intel.com>
 <20220719134503.GA56558@shbuild999.sh.intel.com>
 <5ad51c9f-ce84-5d1b-309c-6e475cebca97@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5ad51c9f-ce84-5d1b-309c-6e475cebca97@suse.cz>
Precedence: bulk

On Tue, Jul 19, 2022 at 04:39:58PM +0200, Vlastimil Babka wrote:
> On 7/19/22 15:45, Feng Tang wrote:
> > Hi Vlastimil,
> > 
> > On Fri, Jul 15, 2022 at 04:29:22PM +0800, Tang, Feng wrote:
> > [...]
> >> > >> - the knowledge of actual size could be used to improve poisoning checks as
> >> > >> well, detect cases when there's buffer overrun over the orig_size but not
> >> > >> cache's size. e.g. if you kmalloc(48) and overrun up to 64 we won't detect
> >> > >> it now, but with orig_size stored we could?
> >> > > 
> >> > > The above patch doesn't touch this. As I have a question, for the
> >> > > [orib_size, object_size) area, shall we fill it with POISON_XXX no matter
> >> > > REDZONE flag is set or not?
> >> > 
> >> > Ah, looks like we use redzoning, not poisoning, for padding from
> >> > s->object_size to word boundary. So it would be more consistent to use the
> >> > redzone pattern (RED_ACTIVE) and check with the dynamic orig_size. Probably
> >> > no change for RED_INACTIVE handling is needed though.
> >> 
> >> Thanks for clarifying, will go this way and do more test. Also I'd 
> >> make it a separate patch, as it is logically different from the space
> >> wastage.
> > 
> > I made a draft to redzone the wasted space, which basically works (patch
> > pasted at the end of the mail) as detecting corruption of below test code:
> > 	
> > 	size = 256;
> > 	buf = kmalloc(size + 8, GFP_KERNEL);
> > 	memset(buf + size + size/2, 0xff, size/4);
> > 	print_section(KERN_ERR, "Corruptted-kmalloc-space", buf, size * 2);
> > 	kfree(buf);
> > 
> > However when it is enabled globally, there are many places reporting
> > corruption. I debugged one case, and found that the network(skb_buff)
> > code already knows this "wasted" kmalloc space and utilize it which is
> > detected by my patch.
> > 
> > The allocation stack is:
> > 
> > [    0.933675] BUG kmalloc-2k (Not tainted): kmalloc unused part overwritten
> > [    0.933675] -----------------------------------------------------------------------------
> > [    0.933675]
> > [    0.933675] 0xffff888237d026c0-0xffff888237d026e3 @offset=9920. First byte 0x0 instead of 0xcc
> > [    0.933675] Allocated in __alloc_skb+0x8e/0x1d0 age=5 cpu=0 pid=1
> > [    0.933675]  __slab_alloc.constprop.0+0x52/0x90
> > [    0.933675]  __kmalloc_node_track_caller+0x129/0x380
> > [    0.933675]  kmalloc_reserve+0x2a/0x70
> > [    0.933675]  __alloc_skb+0x8e/0x1d0
> > [    0.933675]  audit_buffer_alloc+0x3a/0xc0
> > [    0.933675]  audit_log_start.part.0+0xa3/0x300
> > [    0.933675]  audit_log+0x62/0xc0
> > [    0.933675]  audit_init+0x15c/0x16f
> > 
> > And the networking code which touches the [orig_size, object_size) area
> > is in __build_skb_around(), which put a 'struct skb_shared_info' at the
> > end of this area:
> > 
> > 	static void __build_skb_around(struct sk_buff *skb, void *data,
> > 				       unsigned int frag_size)
> > 	{
> > 		struct skb_shared_info *shinfo;
> > 		unsigned int size = frag_size ? : ksize(data);
> 
> Hmm so it's a ksize() user, which should be legitimate way to use the
> "waste" data. Hopefully it should be then enough to patch __ksize() to set
> the object's tracked waste to 0 (orig_size to size) - assume that if
> somebody called ksize() they intend to use the space. That would also make
> the debugfs report more truthful.

Yep, it sounds good to me. Will chase other corrupted places, hope
they are legitimate users too :)

Thanks,
Feng