Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20231122231202.121277-1-andrey.konovalov@linux.dev> <ZV7whSufeIqslzzN@feng-clx>
In-Reply-To: <ZV7whSufeIqslzzN@feng-clx>
From:   Andrey Konovalov <andreyknvl@gmail.com>
Date:   Thu, 23 Nov 2023 17:12:08 +0100
Message-ID: <CA+fCnZcAnZh7H901SZFsaU=-XrpUeeJwUeThMpduDd1-Wt0gsA@mail.gmail.com>
Subject: Re: [PATCH mm] slub, kasan: improve interaction of KASAN and
 slub_debug poisoning
To:     Feng Tang <feng.tang@intel.com>
Cc:     "andrey.konovalov@linux.dev" <andrey.konovalov@linux.dev>,
        Andrew Morton <akpm@linux-foundation.org>,
        Marco Elver <elver@google.com>,
        Alexander Potapenko <glider@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "kasan-dev@googlegroups.com" <kasan-dev@googlegroups.com>,
        Evgenii Stepanov <eugenis@google.com>,
        Oscar Salvador <osalvador@suse.de>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Andrey Konovalov <andreyknvl@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Nov 23, 2023 at 7:35=E2=80=AFAM Feng Tang <feng.tang@intel.com> wro=
te:
>

Hi Feng,

> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -870,20 +870,20 @@ static inline void set_orig_size(struct kmem_cach=
e *s,
> >                               void *object, unsigned int orig_size)
> >  {
> >       void *p =3D kasan_reset_tag(object);
> > +     unsigned int kasan_meta_size;
> >
> >       if (!slub_debug_orig_size(s))
> >               return;
> >
> > -#ifdef CONFIG_KASAN_GENERIC
> >       /*
> > -      * KASAN could save its free meta data in object's data area at
> > -      * offset 0, if the size is larger than 'orig_size', it will
> > -      * overlap the data redzone in [orig_size+1, object_size], and
> > -      * the check should be skipped.
> > +      * KASAN can save its free meta data inside of the object at offs=
et 0.
> > +      * If this meta data size is larger than 'orig_size', it will ove=
rlap
> > +      * the data redzone in [orig_size+1, object_size]. Thus, we adjus=
t
> > +      * 'orig_size' to be as at least as big as KASAN's meta data.
> >        */
> > -     if (kasan_metadata_size(s, true) > orig_size)
> > -             orig_size =3D s->object_size;
> > -#endif
> > +     kasan_meta_size =3D kasan_metadata_size(s, true);
> > +     if (kasan_meta_size > orig_size)
> > +             orig_size =3D kasan_meta_size;
>
> 'orig_size' is to save the orignal request size for kmalloc object,
> and its main purpose is to detect the memory wastage of kmalloc
> objects, see commit 6edf2576a6cc "mm/slub: enable debugging memory
> wasting of kmalloc"
>
> Setting "orig_size =3D s->object_size" was to skip the wastage check
> and the redzone sanity check for this 'wasted space'.

Yes, I get that.

The point of my change was to allow slub_debug detecting overwrites in
the [kasan_meta_size, object_size) range when KASAN stores its free
meta in the [0, kasan_meta_size) range. If orig_size is set to
object_size, writes to that area will not be detected. I also thought
that using kasan_meta_size instead of object_size for orig_size might
give the reader better understanding of the memory layout.

> So it's better not to set 'kasan_meta_size' to orig_size.

I don't have a strong preference here: slub_debug and KASAN are not
really meant to be used together anyway. So if you prefer, I can
revert this change and keep using object_size as before.

> And from the below code, IIUC, the orig_size is not used in fixing
> the boot problem found by Hyeonggon?

No, this is a just a partially-related clean up. It just seemed
natural to include it into the fix, as it also touches the code around
a kasan_metadata_size call.

Thanks!