On Thu, Apr 22, 2021 at 11:58 AM Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>
> On Thu, Apr 22, 2021 at 11:17 AM Maninder Singh <maninder1.s@xxxxxxxxxxx> wrote:
> >
> > when KASAN multishot is ON and some buggy code hits same code path
> > of KASAN issue repetetively, it can flood logs on console.
> >
> > Check for allocaton, free and backtrace path at time of KASAN error,
> > if these are same then it is duplicate error and avoid these prints
> > from KASAN.
Can this be tested with the kunit kasan tests? If yes, please add a
test for this new code.
> > Co-developed-by: Vaneet Narang <v.narang@xxxxxxxxxxx>
> > Signed-off-by: Vaneet Narang <v.narang@xxxxxxxxxxx>
> > Signed-off-by: Maninder Singh <maninder1.s@xxxxxxxxxxx>
> > ---
> > mm/kasan/kasan.h | 6 +++++
> > mm/kasan/report.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 73 insertions(+)
> >
> > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > index 78cf99247139..d14ccce246ba 100644
> > --- a/mm/kasan/kasan.h
> > +++ b/mm/kasan/kasan.h
> > @@ -102,6 +102,12 @@ struct kasan_access_info {
> > unsigned long ip;
> > };
> >
> > +struct kasan_record {
> > + depot_stack_handle_t bt_handle;
> > + depot_stack_handle_t alloc_handle;
> > + depot_stack_handle_t free_handle;
> > +};
>
> Hi Maninder,
>
> There is no need to declare this in the header, it can be declared
> more locally in report.h.
>
> > +
> > /* The layout of struct dictated by compiler */
> > struct kasan_source_location {
> > const char *filename;
> > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > index 87b271206163..4576de76991b 100644
> > --- a/mm/kasan/report.c
> > +++ b/mm/kasan/report.c
> > @@ -39,6 +39,10 @@ static unsigned long kasan_flags;
> > #define KASAN_BIT_REPORTED 0
> > #define KASAN_BIT_MULTI_SHOT 1
> >
> > +#define MAX_RECORDS (200)
>
> s/MAX_RECORDS/KASAN_MAX_RECORDS/
>
> > +static struct kasan_record kasan_records[MAX_RECORDS];
>
> Since all fields in kasan_record are stack handles, the code will be
> simpler and more uniform, if we store just an array of handles w/o
> distinguishing between alloc/free/access.
>
> > +static int stored_kasan_records;
> > +
> > bool kasan_save_enable_multi_shot(void)
> > {
> > return test_and_set_bit(KASAN_BIT_MULTI_SHOT, &kasan_flags);
> > @@ -360,6 +364,65 @@ void kasan_report_invalid_free(void *object, unsigned long ip)
> > end_report(&flags, (unsigned long)object);
> > }
> >
> > +/*
> > + * @save_report()
> > + *
> > + * returns false if same record is already saved.
>
> s/same/the same/
>
> > + * returns true if its new record and saved in database of KASAN.
>
> s/its/it's/
> s/database/the database/
>
> > + */
> > +static bool save_report(void *addr, struct kasan_access_info *info, u8 tag, unsigned long *flags)
> > +{
> > + struct kasan_record record = {0};
> > + depot_stack_handle_t bt_handle;
> > + int i = 0;
> > + const char *bug_type;
> > + struct kasan_alloc_meta *alloc_meta;
> > + struct kasan_track *free_track;
> > + struct page *page;
> > + bool ret = true;
> > +
> > + kasan_disable_current();
> > + spin_lock_irqsave(&report_lock, *flags);
>
> Reusing the caller flags looks strange, do we need it?
> But also the very next function start_report() also does the same
> dance: kasan_disable_current/spin_lock_irqsave. It feels reasonable to
> lock once, check for dups and return early if it's a dup.
>
> > + bug_type = kasan_get_bug_type(info);
> > + page = kasan_addr_to_page(addr);
> > + bt_handle = kasan_save_stack(GFP_KERNEL);
>
> ASsign directly to record.bt_handle.
>
> > + if (page && PageSlab(page)) {
> > + struct kmem_cache *cache = page->slab_cache;
> > + void *object = nearest_obj(cache, page, addr);
>
> Since you already declare new var in this block, move
> alloc_meta/free_track here as well.
>
> > +
> > + alloc_meta = kasan_get_alloc_meta(cache, object);
> > + free_track = kasan_get_free_track(cache, object, tag);
> > + record.alloc_handle = alloc_meta->alloc_track.stack;
> > + if (free_track)
> > + record.free_handle = free_track->stack;
> > + }
> > +
> > + record.bt_handle = bt_handle;
> > +
> > + for (i = 0; i < stored_kasan_records; i++) {
> > + if (record.bt_handle != kasan_records[i].bt_handle)
> > + continue;
> > + if (record.alloc_handle != kasan_records[i].alloc_handle)
> > + continue;
> > + if (!strncmp("use-after-free", bug_type, 15) &&
>
> Comparing strings is unreliable and will break in future. Compare
> handle with 0 instead, you already assume that 0 handle is "no
> handle".
>
> > + (record.free_handle != kasan_records[i].free_handle))
> > + continue;
> > +
> > + ret = false;
> > + goto done;
> > + }
> > +
> > + memcpy(&kasan_records[stored_kasan_records], &record, sizeof(struct kasan_record));
> > + stored_kasan_records++;
>
> I think you just introduced an out-of-bounds write into KASAN, check
> for MAX_RECORDS ;)
>
>
> > +
> > +done:
> > + spin_unlock_irqrestore(&report_lock, *flags);
> > + kasan_enable_current();
> > + return ret;
> > +}
> > +
> > static void __kasan_report(unsigned long addr, size_t size, bool is_write,
> > unsigned long ip)
> > {
> > @@ -388,6 +451,10 @@ static void __kasan_report(unsigned long addr, size_t size, bool is_write,
> > info.is_write = is_write;
> > info.ip = ip;
> >
> > + if (addr_has_metadata(untagged_addr) &&
>
> Why addr_has_metadata check?
> The kernel will probably crash later anyway, but from point of view of
> this code, I don't see reasons to not dedup wild accesses.
>
> > + !save_report(untagged_addr, &info, get_tag(tagged_addr), &flags))
> > + return;
> > +
> > start_report(&flags);
> >
> > print_error_description(&info);
> > --
> > 2.17.1
> >
