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;
MIME-Version: 1.0
References: <20220727071042.8796-4-feng.tang@intel.com> <YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020>
 <YuY6Wc39DbL3YmGi@feng-skl> <Yudw5ge/lJ26Hksk@feng-skl> <0e545088-d140-4c84-bbb2-a3be669740b2@suse.cz>
 <YujKCxu2lJJFm73P@feng-skl>
In-Reply-To: <YujKCxu2lJJFm73P@feng-skl>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Tue, 2 Aug 2022 09:06:41 +0200
Message-ID: <CACT4Y+Zwg8BP=6WJpQ5cCbJxLu4HcnCjx8e53aDEbTZ5uzpUyg@mail.gmail.com>
Subject: Re: [mm/slub] 3616799128: BUG_kmalloc-#(Not_tainted):kmalloc_Redzone_overwritten
To:     Feng Tang <feng.tang@intel.com>
Cc:     Vlastimil Babka <vbabka@suse.cz>,
        "Sang, Oliver" <oliver.sang@intel.com>, lkp <lkp@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "lkp@lists.01.org" <lkp@lists.01.org>,
        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>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        Robin Murphy <robin.murphy@arm.com>,
        John Garry <john.garry@huawei.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        Alexander Potapenko <glider@google.com>,
        "kasan-dev@googlegroups.com" <kasan-dev@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 2 Aug 2022 at 08:55, Feng Tang <feng.tang@intel.com> wrote:
>
> On Mon, Aug 01, 2022 at 10:23:23PM +0800, Vlastimil Babka wrote:
> > On 8/1/22 08:21, Feng Tang wrote:
> [snip]
> > > Cc kansan  mail list.
> > >
> > > This is really related with KASAN debug, that in free path, some
> > > kmalloc redzone ([orig_size+1, object_size]) area is written by
> > > kasan to save free meta info.
> > >
> > > The callstack is:
> > >
> > >   kfree
> > >     slab_free
> > >       slab_free_freelist_hook
> > >           slab_free_hook
> > >             __kasan_slab_free
> > >               ____kasan_slab_free
> > >                 kasan_set_free_info
> > >                   kasan_set_track
> > >
> > > And this issue only happens with "kmalloc-16" slab. Kasan has 2
> > > tracks: alloc_track and free_track, for x86_64 test platform, most
> > > of the slabs will reserve space for alloc_track, and reuse the
> > > 'object' area for free_track.  The kasan free_track is 16 bytes
> > > large, that it will occupy the whole 'kmalloc-16's object area,
> > > so when kmalloc-redzone is enabled by this patch, the 'overwritten'
> > > error is triggered.
> > >
> > > But it won't hurt other kmalloc slabs, as kasan's free meta won't
> > > conflict with kmalloc-redzone which stay in the latter part of
> > > kmalloc area.
> > >
> > > So the solution I can think of is:
> > > * skip the kmalloc-redzone for kmalloc-16 only, or
> > > * skip kmalloc-redzone if kasan is enabled, or
> > > * let kasan reserve the free meta (16 bytes) outside of object
> > >   just like for alloc meta
> >
> > Maybe we could add some hack that if both kasan and SLAB_STORE_USER is
> > enabled, we bump the stored orig_size from <16 to 16? Similar to what
> > __ksize() does.
>
> How about the following patch:
>
> ---
> diff --git a/mm/slub.c b/mm/slub.c
> index added2653bb0..33bbac2afaef 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -830,6 +830,16 @@ static inline void set_orig_size(struct kmem_cache *s,
>         if (!slub_debug_orig_size(s))
>                 return;
>
> +#ifdef CONFIG_KASAN
> +       /*
> +        * When kasan is enabled, it could save its free meta data in the
> +        * start part of object area, so skip the kmalloc redzone check
> +        * for small kmalloc slabs to avoid the data conflict.
> +        */
> +       if (s->object_size <= 32)
> +               orig_size = s->object_size;
> +#endif
> +
>         p += get_info_end(s);
>         p += sizeof(struct track) * 2;
>
> I extend the size to 32 for potential's kasan meta data size increase.
> This is tested locally, if people are OK with it, I can ask for 0Day's
> help to verify this.

Where is set_orig_size() function defined? Don't see it upstream nor
in linux-next.
This looks fine but my only concern is that this should not increase
memory consumption when slub debug tracking is not enabled, which
should be the main operation mode when KASAN is enabled. But I can't
figure this out w/o context.


> Thanks,
> Feng
>
> >
> > > I don't have way to test kasan's SW/HW tag configuration, which
> > > is only enabled on arm64 now. And I don't know if there will
> > > also be some conflict.
> > >
> > > Thanks,
> > > Feng
> > >
> >
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/YujKCxu2lJJFm73P%40feng-skl.