* [PATCH 2/3] kasan: record and print the free track
@ 2020-05-06 5:21 Walter Wu
2020-05-06 9:50 ` Dmitry Vyukov
0 siblings, 1 reply; 5+ messages in thread
From: Walter Wu @ 2020-05-06 5:21 UTC (permalink / raw)
To: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, Matthias Brugger
Cc: kasan-dev, linux-mm, linux-kernel, linux-arm-kernel,
wsd_upstream, linux-mediatek, Walter Wu
We add new KASAN_RCU_STACK_RECORD configuration option. It will move
free track from slub meta-data (struct kasan_alloc_meta) into freed object.
Because we hope this options doesn't enlarge slub meta-data size.
This option doesn't enlarge struct kasan_alloc_meta size.
- add two call_rcu() call stack into kasan_alloc_meta, size is 8 bytes.
- remove free track from kasan_alloc_meta, size is 8 bytes.
This option is only suitable for generic KASAN, because we move free track
into the freed object, so free track is valid information only when it
exists in quarantine. If the object is in-use state, then the KASAN report
doesn't print call_rcu() free track information.
[1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Alexander Potapenko <glider@google.com>
---
mm/kasan/common.c | 10 +++++++++-
mm/kasan/report.c | 24 +++++++++++++++++++++---
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 32d422bdf127..13ec03e225a7 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -321,8 +321,15 @@ void kasan_record_callrcu(void *addr)
/* record last call_rcu() call stack */
alloc_info->rcu_free_stack[1] = save_stack(GFP_NOWAIT);
}
-#endif
+static void kasan_set_free_info(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ /* store free track into freed object */
+ set_track((struct kasan_track *)(object + BYTES_PER_WORD), GFP_NOWAIT);
+}
+
+#else
static void kasan_set_free_info(struct kmem_cache *cache,
void *object, u8 tag)
{
@@ -339,6 +346,7 @@ static void kasan_set_free_info(struct kmem_cache *cache,
set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
}
+#endif
void kasan_poison_slab(struct page *page)
{
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 7aaccc70b65b..f2b0c6b9dffa 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -175,8 +175,23 @@ static void kasan_print_rcu_free_stack(struct kasan_alloc_meta *alloc_info)
print_track(&free_track, "Last call_rcu() call stack", true);
pr_err("\n");
}
-#endif
+static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag, const void *addr)
+{
+ u8 *shadow_addr = (u8 *)kasan_mem_to_shadow(addr);
+
+ /*
+ * Only the freed object can get free track,
+ * because free track information is stored to freed object.
+ */
+ if (*shadow_addr == KASAN_KMALLOC_FREE)
+ return (struct kasan_track *)(object + BYTES_PER_WORD);
+ else
+ return NULL;
+}
+
+#else
static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
void *object, u8 tag, const void *addr)
{
@@ -196,6 +211,7 @@ static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
return &alloc_meta->free_track[i];
}
+#endif
static void describe_object(struct kmem_cache *cache, void *object,
const void *addr, u8 tag)
@@ -208,8 +224,10 @@ static void describe_object(struct kmem_cache *cache, void *object,
print_track(&alloc_info->alloc_track, "Allocated", false);
pr_err("\n");
free_track = kasan_get_free_track(cache, object, tag, addr);
- print_track(free_track, "Freed", false);
- pr_err("\n");
+ if (free_track) {
+ print_track(free_track, "Freed", false);
+ pr_err("\n");
+ }
#ifdef CONFIG_KASAN_RCU_STACK_RECORD
kasan_print_rcu_free_stack(alloc_info);
#endif
--
2.18.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] kasan: record and print the free track
2020-05-06 5:21 [PATCH 2/3] kasan: record and print the free track Walter Wu
@ 2020-05-06 9:50 ` Dmitry Vyukov
2020-05-06 11:56 ` Walter Wu
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Vyukov @ 2020-05-06 9:50 UTC (permalink / raw)
To: Walter Wu
Cc: Andrey Ryabinin, Alexander Potapenko, Matthias Brugger,
kasan-dev, Linux-MM, LKML, Linux ARM, wsd_upstream,
linux-mediatek
On Wed, May 6, 2020 at 7:22 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
>
> We add new KASAN_RCU_STACK_RECORD configuration option. It will move
> free track from slub meta-data (struct kasan_alloc_meta) into freed object.
> Because we hope this options doesn't enlarge slub meta-data size.
>
> This option doesn't enlarge struct kasan_alloc_meta size.
> - add two call_rcu() call stack into kasan_alloc_meta, size is 8 bytes.
> - remove free track from kasan_alloc_meta, size is 8 bytes.
>
> This option is only suitable for generic KASAN, because we move free track
> into the freed object, so free track is valid information only when it
> exists in quarantine. If the object is in-use state, then the KASAN report
> doesn't print call_rcu() free track information.
>
> [1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
>
> Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Alexander Potapenko <glider@google.com>
> ---
> mm/kasan/common.c | 10 +++++++++-
> mm/kasan/report.c | 24 +++++++++++++++++++++---
> 2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 32d422bdf127..13ec03e225a7 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -321,8 +321,15 @@ void kasan_record_callrcu(void *addr)
> /* record last call_rcu() call stack */
> alloc_info->rcu_free_stack[1] = save_stack(GFP_NOWAIT);
> }
> -#endif
>
> +static void kasan_set_free_info(struct kmem_cache *cache,
> + void *object, u8 tag)
> +{
> + /* store free track into freed object */
> + set_track((struct kasan_track *)(object + BYTES_PER_WORD), GFP_NOWAIT);
> +}
> +
> +#else
> static void kasan_set_free_info(struct kmem_cache *cache,
> void *object, u8 tag)
> {
> @@ -339,6 +346,7 @@ static void kasan_set_free_info(struct kmem_cache *cache,
>
> set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
> }
> +#endif
>
> void kasan_poison_slab(struct page *page)
> {
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 7aaccc70b65b..f2b0c6b9dffa 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -175,8 +175,23 @@ static void kasan_print_rcu_free_stack(struct kasan_alloc_meta *alloc_info)
> print_track(&free_track, "Last call_rcu() call stack", true);
> pr_err("\n");
> }
> -#endif
>
> +static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> + void *object, u8 tag, const void *addr)
> +{
> + u8 *shadow_addr = (u8 *)kasan_mem_to_shadow(addr);
> +
> + /*
> + * Only the freed object can get free track,
> + * because free track information is stored to freed object.
> + */
> + if (*shadow_addr == KASAN_KMALLOC_FREE)
> + return (struct kasan_track *)(object + BYTES_PER_WORD);
Humm... the other patch defines BYTES_PER_WORD as 4... I would assume
seeing 8 (or sizeof(long)) here. Why 4?
Have you tested all 4 modes (RCU/no-RCU x SLAB/SLUB)? As far as I
remember one of the allocators stored something in the object.
Also, does this work with objects with ctors and slabs destroyed by
rcu? kasan_track may smash other things in these cases.
Have you looked at the KASAN implementation when free_track was
removed? That may have useful details :)
> + else
> + return NULL;
> +}
> +
> +#else
> static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> void *object, u8 tag, const void *addr)
> {
> @@ -196,6 +211,7 @@ static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
>
> return &alloc_meta->free_track[i];
> }
> +#endif
>
> static void describe_object(struct kmem_cache *cache, void *object,
> const void *addr, u8 tag)
> @@ -208,8 +224,10 @@ static void describe_object(struct kmem_cache *cache, void *object,
> print_track(&alloc_info->alloc_track, "Allocated", false);
> pr_err("\n");
> free_track = kasan_get_free_track(cache, object, tag, addr);
> - print_track(free_track, "Freed", false);
> - pr_err("\n");
> + if (free_track) {
> + print_track(free_track, "Freed", false);
> + pr_err("\n");
> + }
> #ifdef CONFIG_KASAN_RCU_STACK_RECORD
> kasan_print_rcu_free_stack(alloc_info);
> #endif
> --
> 2.18.0
>
> --
> 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/20200506052155.14515-1-walter-zh.wu%40mediatek.com.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] kasan: record and print the free track
2020-05-06 9:50 ` Dmitry Vyukov
@ 2020-05-06 11:56 ` Walter Wu
2020-05-06 11:59 ` Dmitry Vyukov
0 siblings, 1 reply; 5+ messages in thread
From: Walter Wu @ 2020-05-06 11:56 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Alexander Potapenko, Matthias Brugger,
kasan-dev, Linux-MM, LKML, Linux ARM, wsd_upstream,
linux-mediatek
On Wed, 2020-05-06 at 11:50 +0200, Dmitry Vyukov wrote:
> On Wed, May 6, 2020 at 7:22 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> >
> > We add new KASAN_RCU_STACK_RECORD configuration option. It will move
> > free track from slub meta-data (struct kasan_alloc_meta) into freed object.
> > Because we hope this options doesn't enlarge slub meta-data size.
> >
> > This option doesn't enlarge struct kasan_alloc_meta size.
> > - add two call_rcu() call stack into kasan_alloc_meta, size is 8 bytes.
> > - remove free track from kasan_alloc_meta, size is 8 bytes.
> >
> > This option is only suitable for generic KASAN, because we move free track
> > into the freed object, so free track is valid information only when it
> > exists in quarantine. If the object is in-use state, then the KASAN report
> > doesn't print call_rcu() free track information.
> >
> > [1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
> >
> > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Alexander Potapenko <glider@google.com>
> > ---
> > mm/kasan/common.c | 10 +++++++++-
> > mm/kasan/report.c | 24 +++++++++++++++++++++---
> > 2 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > index 32d422bdf127..13ec03e225a7 100644
> > --- a/mm/kasan/common.c
> > +++ b/mm/kasan/common.c
> > @@ -321,8 +321,15 @@ void kasan_record_callrcu(void *addr)
> > /* record last call_rcu() call stack */
> > alloc_info->rcu_free_stack[1] = save_stack(GFP_NOWAIT);
> > }
> > -#endif
> >
> > +static void kasan_set_free_info(struct kmem_cache *cache,
> > + void *object, u8 tag)
> > +{
> > + /* store free track into freed object */
> > + set_track((struct kasan_track *)(object + BYTES_PER_WORD), GFP_NOWAIT);
> > +}
> > +
> > +#else
> > static void kasan_set_free_info(struct kmem_cache *cache,
> > void *object, u8 tag)
> > {
> > @@ -339,6 +346,7 @@ static void kasan_set_free_info(struct kmem_cache *cache,
> >
> > set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
> > }
> > +#endif
> >
> > void kasan_poison_slab(struct page *page)
> > {
> > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > index 7aaccc70b65b..f2b0c6b9dffa 100644
> > --- a/mm/kasan/report.c
> > +++ b/mm/kasan/report.c
> > @@ -175,8 +175,23 @@ static void kasan_print_rcu_free_stack(struct kasan_alloc_meta *alloc_info)
> > print_track(&free_track, "Last call_rcu() call stack", true);
> > pr_err("\n");
> > }
> > -#endif
> >
> > +static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > + void *object, u8 tag, const void *addr)
> > +{
> > + u8 *shadow_addr = (u8 *)kasan_mem_to_shadow(addr);
> > +
> > + /*
> > + * Only the freed object can get free track,
> > + * because free track information is stored to freed object.
> > + */
> > + if (*shadow_addr == KASAN_KMALLOC_FREE)
> > + return (struct kasan_track *)(object + BYTES_PER_WORD);
>
> Humm... the other patch defines BYTES_PER_WORD as 4... I would assume
> seeing 8 (or sizeof(long)) here. Why 4?
It should be a pointer size, maybe sizeof(long) makes more sense.
> Have you tested all 4 modes (RCU/no-RCU x SLAB/SLUB)? As far as I
> remember one of the allocators stored something in the object.
Good question, I only tested in RCU x SLUB, would you tell mew how do
no-RCU? I will test them in v2 pathset.
>
> Also, does this work with objects with ctors and slabs destroyed by
> rcu? kasan_track may smash other things in these cases.
> Have you looked at the KASAN implementation when free_track was
> removed? That may have useful details :)
Set free_track before put into quarantine, free_track should not have to
be removed, it only have to overwirte itself.
>
>
> > + else
> > + return NULL;
> > +}
> > +
> > +#else
> > static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > void *object, u8 tag, const void *addr)
> > {
> > @@ -196,6 +211,7 @@ static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> >
> > return &alloc_meta->free_track[i];
> > }
> > +#endif
> >
> > static void describe_object(struct kmem_cache *cache, void *object,
> > const void *addr, u8 tag)
> > @@ -208,8 +224,10 @@ static void describe_object(struct kmem_cache *cache, void *object,
> > print_track(&alloc_info->alloc_track, "Allocated", false);
> > pr_err("\n");
> > free_track = kasan_get_free_track(cache, object, tag, addr);
> > - print_track(free_track, "Freed", false);
> > - pr_err("\n");
> > + if (free_track) {
> > + print_track(free_track, "Freed", false);
> > + pr_err("\n");
> > + }
> > #ifdef CONFIG_KASAN_RCU_STACK_RECORD
> > kasan_print_rcu_free_stack(alloc_info);
> > #endif
> > --
> > 2.18.0
> >
> > --
> > 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/20200506052155.14515-1-walter-zh.wu%40mediatek.com.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] kasan: record and print the free track
2020-05-06 11:56 ` Walter Wu
@ 2020-05-06 11:59 ` Dmitry Vyukov
2020-05-06 12:07 ` Walter Wu
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Vyukov @ 2020-05-06 11:59 UTC (permalink / raw)
To: Walter Wu
Cc: Andrey Ryabinin, Alexander Potapenko, Matthias Brugger,
kasan-dev, Linux-MM, LKML, Linux ARM, wsd_upstream,
linux-mediatek
On Wed, May 6, 2020 at 1:56 PM Walter Wu <walter-zh.wu@mediatek.com> wrote:
>
> On Wed, 2020-05-06 at 11:50 +0200, Dmitry Vyukov wrote:
> > On Wed, May 6, 2020 at 7:22 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> > >
> > > We add new KASAN_RCU_STACK_RECORD configuration option. It will move
> > > free track from slub meta-data (struct kasan_alloc_meta) into freed object.
> > > Because we hope this options doesn't enlarge slub meta-data size.
> > >
> > > This option doesn't enlarge struct kasan_alloc_meta size.
> > > - add two call_rcu() call stack into kasan_alloc_meta, size is 8 bytes.
> > > - remove free track from kasan_alloc_meta, size is 8 bytes.
> > >
> > > This option is only suitable for generic KASAN, because we move free track
> > > into the freed object, so free track is valid information only when it
> > > exists in quarantine. If the object is in-use state, then the KASAN report
> > > doesn't print call_rcu() free track information.
> > >
> > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
> > >
> > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > Cc: Alexander Potapenko <glider@google.com>
> > > ---
> > > mm/kasan/common.c | 10 +++++++++-
> > > mm/kasan/report.c | 24 +++++++++++++++++++++---
> > > 2 files changed, 30 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > index 32d422bdf127..13ec03e225a7 100644
> > > --- a/mm/kasan/common.c
> > > +++ b/mm/kasan/common.c
> > > @@ -321,8 +321,15 @@ void kasan_record_callrcu(void *addr)
> > > /* record last call_rcu() call stack */
> > > alloc_info->rcu_free_stack[1] = save_stack(GFP_NOWAIT);
> > > }
> > > -#endif
> > >
> > > +static void kasan_set_free_info(struct kmem_cache *cache,
> > > + void *object, u8 tag)
> > > +{
> > > + /* store free track into freed object */
> > > + set_track((struct kasan_track *)(object + BYTES_PER_WORD), GFP_NOWAIT);
> > > +}
> > > +
> > > +#else
> > > static void kasan_set_free_info(struct kmem_cache *cache,
> > > void *object, u8 tag)
> > > {
> > > @@ -339,6 +346,7 @@ static void kasan_set_free_info(struct kmem_cache *cache,
> > >
> > > set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
> > > }
> > > +#endif
> > >
> > > void kasan_poison_slab(struct page *page)
> > > {
> > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > > index 7aaccc70b65b..f2b0c6b9dffa 100644
> > > --- a/mm/kasan/report.c
> > > +++ b/mm/kasan/report.c
> > > @@ -175,8 +175,23 @@ static void kasan_print_rcu_free_stack(struct kasan_alloc_meta *alloc_info)
> > > print_track(&free_track, "Last call_rcu() call stack", true);
> > > pr_err("\n");
> > > }
> > > -#endif
> > >
> > > +static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > > + void *object, u8 tag, const void *addr)
> > > +{
> > > + u8 *shadow_addr = (u8 *)kasan_mem_to_shadow(addr);
> > > +
> > > + /*
> > > + * Only the freed object can get free track,
> > > + * because free track information is stored to freed object.
> > > + */
> > > + if (*shadow_addr == KASAN_KMALLOC_FREE)
> > > + return (struct kasan_track *)(object + BYTES_PER_WORD);
> >
> > Humm... the other patch defines BYTES_PER_WORD as 4... I would assume
> > seeing 8 (or sizeof(long)) here. Why 4?
> It should be a pointer size, maybe sizeof(long) makes more sense.
>
> > Have you tested all 4 modes (RCU/no-RCU x SLAB/SLUB)? As far as I
> > remember one of the allocators stored something in the object.
> Good question, I only tested in RCU x SLUB, would you tell mew how do
> no-RCU? I will test them in v2 pathset.
I meant with CONFIG_KASAN_RCU_STACK_RECORD=y and with
CONFIG_KASAN_RCU_STACK_RECORD not set.
But if we drop CONFIG_KASAN_RCU_STACK_RECORD config, then it halves
the number of configurations to test ;)
> >
> > Also, does this work with objects with ctors and slabs destroyed by
> > rcu? kasan_track may smash other things in these cases.
> > Have you looked at the KASAN implementation when free_track was
> > removed? That may have useful details :)
> Set free_track before put into quarantine, free_track should not have to
> be removed, it only have to overwirte itself.
>
> >
> >
> > > + else
> > > + return NULL;
> > > +}
> > > +
> > > +#else
> > > static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > > void *object, u8 tag, const void *addr)
> > > {
> > > @@ -196,6 +211,7 @@ static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > >
> > > return &alloc_meta->free_track[i];
> > > }
> > > +#endif
> > >
> > > static void describe_object(struct kmem_cache *cache, void *object,
> > > const void *addr, u8 tag)
> > > @@ -208,8 +224,10 @@ static void describe_object(struct kmem_cache *cache, void *object,
> > > print_track(&alloc_info->alloc_track, "Allocated", false);
> > > pr_err("\n");
> > > free_track = kasan_get_free_track(cache, object, tag, addr);
> > > - print_track(free_track, "Freed", false);
> > > - pr_err("\n");
> > > + if (free_track) {
> > > + print_track(free_track, "Freed", false);
> > > + pr_err("\n");
> > > + }
> > > #ifdef CONFIG_KASAN_RCU_STACK_RECORD
> > > kasan_print_rcu_free_stack(alloc_info);
> > > #endif
> > > --
> > > 2.18.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] kasan: record and print the free track
2020-05-06 11:59 ` Dmitry Vyukov
@ 2020-05-06 12:07 ` Walter Wu
0 siblings, 0 replies; 5+ messages in thread
From: Walter Wu @ 2020-05-06 12:07 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Alexander Potapenko, Matthias Brugger,
kasan-dev, Linux-MM, LKML, Linux ARM, wsd_upstream,
linux-mediatek
On Wed, 2020-05-06 at 13:59 +0200, Dmitry Vyukov wrote:
> On Wed, May 6, 2020 at 1:56 PM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> >
> > On Wed, 2020-05-06 at 11:50 +0200, Dmitry Vyukov wrote:
> > > On Wed, May 6, 2020 at 7:22 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
> > > >
> > > > We add new KASAN_RCU_STACK_RECORD configuration option. It will move
> > > > free track from slub meta-data (struct kasan_alloc_meta) into freed object.
> > > > Because we hope this options doesn't enlarge slub meta-data size.
> > > >
> > > > This option doesn't enlarge struct kasan_alloc_meta size.
> > > > - add two call_rcu() call stack into kasan_alloc_meta, size is 8 bytes.
> > > > - remove free track from kasan_alloc_meta, size is 8 bytes.
> > > >
> > > > This option is only suitable for generic KASAN, because we move free track
> > > > into the freed object, so free track is valid information only when it
> > > > exists in quarantine. If the object is in-use state, then the KASAN report
> > > > doesn't print call_rcu() free track information.
> > > >
> > > > [1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
> > > >
> > > > Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com>
> > > > Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > Cc: Alexander Potapenko <glider@google.com>
> > > > ---
> > > > mm/kasan/common.c | 10 +++++++++-
> > > > mm/kasan/report.c | 24 +++++++++++++++++++++---
> > > > 2 files changed, 30 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > > > index 32d422bdf127..13ec03e225a7 100644
> > > > --- a/mm/kasan/common.c
> > > > +++ b/mm/kasan/common.c
> > > > @@ -321,8 +321,15 @@ void kasan_record_callrcu(void *addr)
> > > > /* record last call_rcu() call stack */
> > > > alloc_info->rcu_free_stack[1] = save_stack(GFP_NOWAIT);
> > > > }
> > > > -#endif
> > > >
> > > > +static void kasan_set_free_info(struct kmem_cache *cache,
> > > > + void *object, u8 tag)
> > > > +{
> > > > + /* store free track into freed object */
> > > > + set_track((struct kasan_track *)(object + BYTES_PER_WORD), GFP_NOWAIT);
> > > > +}
> > > > +
> > > > +#else
> > > > static void kasan_set_free_info(struct kmem_cache *cache,
> > > > void *object, u8 tag)
> > > > {
> > > > @@ -339,6 +346,7 @@ static void kasan_set_free_info(struct kmem_cache *cache,
> > > >
> > > > set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
> > > > }
> > > > +#endif
> > > >
> > > > void kasan_poison_slab(struct page *page)
> > > > {
> > > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > > > index 7aaccc70b65b..f2b0c6b9dffa 100644
> > > > --- a/mm/kasan/report.c
> > > > +++ b/mm/kasan/report.c
> > > > @@ -175,8 +175,23 @@ static void kasan_print_rcu_free_stack(struct kasan_alloc_meta *alloc_info)
> > > > print_track(&free_track, "Last call_rcu() call stack", true);
> > > > pr_err("\n");
> > > > }
> > > > -#endif
> > > >
> > > > +static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > > > + void *object, u8 tag, const void *addr)
> > > > +{
> > > > + u8 *shadow_addr = (u8 *)kasan_mem_to_shadow(addr);
> > > > +
> > > > + /*
> > > > + * Only the freed object can get free track,
> > > > + * because free track information is stored to freed object.
> > > > + */
> > > > + if (*shadow_addr == KASAN_KMALLOC_FREE)
> > > > + return (struct kasan_track *)(object + BYTES_PER_WORD);
> > >
> > > Humm... the other patch defines BYTES_PER_WORD as 4... I would assume
> > > seeing 8 (or sizeof(long)) here. Why 4?
> > It should be a pointer size, maybe sizeof(long) makes more sense.
> >
> > > Have you tested all 4 modes (RCU/no-RCU x SLAB/SLUB)? As far as I
> > > remember one of the allocators stored something in the object.
> > Good question, I only tested in RCU x SLUB, would you tell mew how do
> > no-RCU? I will test them in v2 pathset.
>
> I meant with CONFIG_KASAN_RCU_STACK_RECORD=y and with
> CONFIG_KASAN_RCU_STACK_RECORD not set.
> But if we drop CONFIG_KASAN_RCU_STACK_RECORD config, then it halves
> the number of configurations to test ;)
>
Ok, I have be tested by RCU xSLUB and no_RCUxSLUB, It is workable. So I
only miss this SLAB combination.
>
> > >
> > > Also, does this work with objects with ctors and slabs destroyed by
> > > rcu? kasan_track may smash other things in these cases.
> > > Have you looked at the KASAN implementation when free_track was
> > > removed? That may have useful details :)
> > Set free_track before put into quarantine, free_track should not have to
> > be removed, it only have to overwirte itself.
> >
> > >
> > >
> > > > + else
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +#else
> > > > static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > > > void *object, u8 tag, const void *addr)
> > > > {
> > > > @@ -196,6 +211,7 @@ static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > > >
> > > > return &alloc_meta->free_track[i];
> > > > }
> > > > +#endif
> > > >
> > > > static void describe_object(struct kmem_cache *cache, void *object,
> > > > const void *addr, u8 tag)
> > > > @@ -208,8 +224,10 @@ static void describe_object(struct kmem_cache *cache, void *object,
> > > > print_track(&alloc_info->alloc_track, "Allocated", false);
> > > > pr_err("\n");
> > > > free_track = kasan_get_free_track(cache, object, tag, addr);
> > > > - print_track(free_track, "Freed", false);
> > > > - pr_err("\n");
> > > > + if (free_track) {
> > > > + print_track(free_track, "Freed", false);
> > > > + pr_err("\n");
> > > > + }
> > > > #ifdef CONFIG_KASAN_RCU_STACK_RECORD
> > > > kasan_print_rcu_free_stack(alloc_info);
> > > > #endif
> > > > --
> > > > 2.18.0
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-05-06 12:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-06 5:21 [PATCH 2/3] kasan: record and print the free track Walter Wu
2020-05-06 9:50 ` Dmitry Vyukov
2020-05-06 11:56 ` Walter Wu
2020-05-06 11:59 ` Dmitry Vyukov
2020-05-06 12:07 ` Walter Wu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).