From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2C8B6C19F2B for ; Thu, 4 Aug 2022 10:48:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237448AbiHDKsS (ORCPT ); Thu, 4 Aug 2022 06:48:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41362 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229575AbiHDKsP (ORCPT ); Thu, 4 Aug 2022 06:48:15 -0400 Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9211D3DF2D for ; Thu, 4 Aug 2022 03:48:12 -0700 (PDT) Received: by mail-lf1-x12b.google.com with SMTP id x39so20633710lfu.7 for ; Thu, 04 Aug 2022 03:48:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=eegZBp28eQmH3OhuZfCVY16Y84OTpGQZtSHItuJdUI0=; b=LBXwMho0naCRAn0uLfy0/shvSkniUOYXZOse7ecIpBosuFqQK30HwYCSpjmed5m1Pk Beav35IYNT0r0MaaEh2ir6p3mNW6An4tqXR2sHJRK3H08h8vmfVuWqUvxjLUX/9k86ot kGq34sHJN/VRKF/73MVN+s1qgZrppUjyfw1yURt2rMc4BiiWNLgF6Z7RkKK7gLXEprwm RuXbEOsS+Q/ZAtzLeNAs4BRCcynwrYWZl8c1lzRBg+SVa4aoV8mecs4nC9pALmcnWRnk poVgCteiyEPAnaAUPBjznLxxQF/eLppVvXHF5Vi+kEARnKrwsil3QpqXTrykjI9V6KZB usnQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=eegZBp28eQmH3OhuZfCVY16Y84OTpGQZtSHItuJdUI0=; b=C1iew6188m0lzN71XVqspbGElYGKsE/xXV9Ui9D0vmjHNrpfr9m7QMuQ8DqryA9GY1 IkD4Brz1s5OgYZLc5CCxvG+VTVCbza96+tWw3sOrRXfZiUCOxF+CiJ+WewaIL+Oo5Hb4 Lkd6vnnPS/RhihJTaTQLKFXdaOeR16XPi+Z3Uf2vmXkqMGu7ZaxWHTIV7zD+tqxTRAFT WJ4H+Bw53nOEqedpI2Xu2GzYUuu4pdtDyO2lYo6iKyVwJphkBFpAxzWXFo7v3s3Ijzth srk/qFtIBjPUDFUwTRSYW6H0nXoIXcki4BI2zVmWKASf1InF8PbxKciYowpmzr18iIjQ zgqg== X-Gm-Message-State: ACgBeo36A6s0gZBuhjwlbutgGdcmAXstlwQCPZR1MshYlhUfRPJAmsqY 7SLUCJ0bjwD0CKvqO6FLTszCmxrMwNsmyNgYaGnJYA== X-Google-Smtp-Source: AA6agR4sbvFnha5ksT3ZlL3sUCWj6dLhquYwtG8AA19v9az2AcMHq+TuC0J8RgPecOYrD2Xa9R5XGidkqyspyIZE0PM= X-Received: by 2002:a05:6512:1086:b0:48b:27a4:5059 with SMTP id j6-20020a056512108600b0048b27a45059mr492779lfg.540.1659610090741; Thu, 04 Aug 2022 03:48:10 -0700 (PDT) MIME-Version: 1.0 References: <0e545088-d140-4c84-bbb2-a3be669740b2@suse.cz> <85ec4ea8-ae4c-3592-5491-3db6d0ad8c59@suse.cz> In-Reply-To: From: Dmitry Vyukov Date: Thu, 4 Aug 2022 12:47:58 +0200 Message-ID: Subject: Re: [mm/slub] 3616799128: BUG_kmalloc-#(Not_tainted):kmalloc_Redzone_overwritten To: Feng Tang Cc: Vlastimil Babka , "Sang, Oliver" , lkp , LKML , "linux-mm@kvack.org" , "lkp@lists.01.org" , Andrew Morton , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Roman Gushchin , Hyeonggon Yoo <42.hyeyoo@gmail.com>, "Hansen, Dave" , Robin Murphy , John Garry , Kefeng Wang , Andrey Konovalov , Andrey Ryabinin , Alexander Potapenko , "kasan-dev@googlegroups.com" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 4 Aug 2022 at 08:29, Feng Tang wrote: > > > > .On Tue, 2 Aug 2022 at 11:43, Vlastimil Babka wrote: > > > > > > > > > > On 8/2/22 09:06, Dmitry Vyukov wrote: > > > > > > On Tue, 2 Aug 2022 at 08:55, Feng Tang 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. > > > > > > > > > > Is there maybe some KASAN macro we can use instead of hardcoding 32? > > > > > > > > kasan_free_meta is placed in the object data after freeing, so it can > > > > be sizeof(kasan_free_meta) > > > > > > 'kasan_free_meta' is defined in mm/kasan/kasan.h, to use it we need to > > > include "../kasan/kasan.h" in slub.c, or move its definition to > > > "include/linux/kasan.h" > > > > > > Another idea is to save the info in kasan_info, like: > > > > > > --- > > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > > > index b092277bf48d..97e899948d0b 100644 > > > --- a/include/linux/kasan.h > > > +++ b/include/linux/kasan.h > > > @@ -100,6 +100,7 @@ static inline bool kasan_has_integrated_init(void) > > > struct kasan_cache { > > > int alloc_meta_offset; > > > int free_meta_offset; > > > + int free_meta_size; > > > > Storing it here looks fine to me. > > But I would name it based on the meaning for external users (i.e. that > > many bytes are occupied by kasan in freed objects). For some caches > > KASAN does not store anything in freed objects at all. > > OK, please review the below patch, thanks! > > - Feng > > ---8<--- > From c4fc739ea4d5222f0aba4b42b59668d64a010082 Mon Sep 17 00:00:00 2001 > From: Feng Tang > Date: Thu, 4 Aug 2022 13:25:35 +0800 > Subject: [PATCH] mm: kasan: Add free_meta size info in struct kasan_cache > > When kasan is enabled for slab/slub, it may save kasan' free_meta > data in the former part of slab object data area in slab object > free path, which works fine. > > There is ongoing effort to extend slub's debug function which will > redzone the latter part of kmalloc object area, and when both of > the debug are enabled, there is possible conflict, especially when > the kmalloc object has small size, as caught by 0Day bot [1] > > For better information for slab/slub, add free_meta's data size > info 'kasan_cache', so that its users can take right action to > avoid data conflict. > > [1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/ > Reported-by: kernel test robot > Signed-off-by: Feng Tang Acked-by: Dmitry Vyukov I assume there will be a second patch that uses free_meta_size_in_object in slub debug code. > --- > include/linux/kasan.h | 2 ++ > mm/kasan/common.c | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index b092277bf48d..293bdaa0ba09 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -100,6 +100,8 @@ static inline bool kasan_has_integrated_init(void) > struct kasan_cache { > int alloc_meta_offset; > int free_meta_offset; > + /* size of free_meta data saved in object's data area */ > + int free_meta_size_in_object; > bool is_kmalloc; > }; > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 78be2beb7453..a627efa267d1 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -201,6 +201,8 @@ void __kasan_cache_create(struct kmem_cache *cache, unsigned int *size, > cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META; > *size = ok_size; > } > + } else { > + cache->kasan_info.free_meta_size_in_object = sizeof(struct kasan_free_meta); > } > > /* Calculate size with optimal redzone. */ > -- > 2.27.0 From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============0731291296623682300==" MIME-Version: 1.0 From: Dmitry Vyukov To: lkp@lists.01.org Subject: Re: [mm/slub] 3616799128: BUG_kmalloc-#(Not_tainted):kmalloc_Redzone_overwritten Date: Thu, 04 Aug 2022 12:47:58 +0200 Message-ID: In-Reply-To: List-Id: --===============0731291296623682300== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On Thu, 4 Aug 2022 at 08:29, Feng Tang wrote: > > > > .On Tue, 2 Aug 2022 at 11:43, Vlastimil Babka wr= ote: > > > > > > > > > > On 8/2/22 09:06, Dmitry Vyukov wrote: > > > > > > On Tue, 2 Aug 2022 at 08:55, Feng Tang = wrote: > > > > > >> > > > > > >> On Mon, Aug 01, 2022 at 10:23:23PM +0800, Vlastimil Babka wrot= e: > > > > > >> > 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 writt= en 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 platfo= rm, 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 'ove= rwritten' > > > > > >> > > error is triggered. > > > > > >> > > > > > > > >> > > But it won't hurt other kmalloc slabs, as kasan's free met= a won't > > > > > >> > > conflict with kmalloc-redzone which stay in the latter par= t 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 ob= ject > > > > > >> > > just like for alloc meta > > > > > >> > > > > > > >> > Maybe we could add some hack that if both kasan and SLAB_STO= RE_USER is > > > > > >> > enabled, we bump the stored orig_size from <16 to 16? Simila= r 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 k= mem_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 redz= one check > > > > > >> + * for small kmalloc slabs to avoid the data conflict. > > > > > >> + */ > > > > > >> + if (s->object_size <=3D 32) > > > > > >> + orig_size =3D s->object_size; > > > > > >> +#endif > > > > > >> + > > > > > >> p +=3D get_info_end(s); > > > > > >> p +=3D sizeof(struct track) * 2; > > > > > >> > > > > > >> I extend the size to 32 for potential's kasan meta data size i= ncrease. > > > > > >> This is tested locally, if people are OK with it, I can ask fo= r 0Day's > > > > > >> help to verify this. > > > > > > > > > > Is there maybe some KASAN macro we can use instead of hardcoding = 32? > > > > > > > > kasan_free_meta is placed in the object data after freeing, so it c= an > > > > be sizeof(kasan_free_meta) > > > > > > 'kasan_free_meta' is defined in mm/kasan/kasan.h, to use it we need to > > > include "../kasan/kasan.h" in slub.c, or move its definition to > > > "include/linux/kasan.h" > > > > > > Another idea is to save the info in kasan_info, like: > > > > > > --- > > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > > > index b092277bf48d..97e899948d0b 100644 > > > --- a/include/linux/kasan.h > > > +++ b/include/linux/kasan.h > > > @@ -100,6 +100,7 @@ static inline bool kasan_has_integrated_init(void) > > > struct kasan_cache { > > > int alloc_meta_offset; > > > int free_meta_offset; > > > + int free_meta_size; > > > > Storing it here looks fine to me. > > But I would name it based on the meaning for external users (i.e. that > > many bytes are occupied by kasan in freed objects). For some caches > > KASAN does not store anything in freed objects at all. > > OK, please review the below patch, thanks! > > - Feng > > ---8<--- > From c4fc739ea4d5222f0aba4b42b59668d64a010082 Mon Sep 17 00:00:00 2001 > From: Feng Tang > Date: Thu, 4 Aug 2022 13:25:35 +0800 > Subject: [PATCH] mm: kasan: Add free_meta size info in struct kasan_cache > > When kasan is enabled for slab/slub, it may save kasan' free_meta > data in the former part of slab object data area in slab object > free path, which works fine. > > There is ongoing effort to extend slub's debug function which will > redzone the latter part of kmalloc object area, and when both of > the debug are enabled, there is possible conflict, especially when > the kmalloc object has small size, as caught by 0Day bot [1] > > For better information for slab/slub, add free_meta's data size > info 'kasan_cache', so that its users can take right action to > avoid data conflict. > > [1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu(a)xsang-OptiPlex-9020/ > Reported-by: kernel test robot > Signed-off-by: Feng Tang Acked-by: Dmitry Vyukov I assume there will be a second patch that uses free_meta_size_in_object in slub debug code. > --- > include/linux/kasan.h | 2 ++ > mm/kasan/common.c | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index b092277bf48d..293bdaa0ba09 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -100,6 +100,8 @@ static inline bool kasan_has_integrated_init(void) > struct kasan_cache { > int alloc_meta_offset; > int free_meta_offset; > + /* size of free_meta data saved in object's data area */ > + int free_meta_size_in_object; > bool is_kmalloc; > }; > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 78be2beb7453..a627efa267d1 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -201,6 +201,8 @@ void __kasan_cache_create(struct kmem_cache *cache, u= nsigned int *size, > cache->kasan_info.free_meta_offset =3D KASAN_NO_F= REE_META; > *size =3D ok_size; > } > + } else { > + cache->kasan_info.free_meta_size_in_object =3D sizeof(str= uct kasan_free_meta); > } > > /* Calculate size with optimal redzone. */ > -- > 2.27.0 --===============0731291296623682300==--