From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754185AbcEQIzT (ORCPT ); Tue, 17 May 2016 04:55:19 -0400 Received: from mail-lb0-f180.google.com ([209.85.217.180]:34069 "EHLO mail-lb0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751153AbcEQIzQ convert rfc822-to-8bit (ORCPT ); Tue, 17 May 2016 04:55:16 -0400 MIME-Version: 1.0 In-Reply-To: References: <1462987130-144092-1-git-send-email-glider@google.com> Date: Tue, 17 May 2016 10:55:13 +0200 Message-ID: Subject: Re: [PATCH v9] mm: kasan: Initial memory quarantine implementation From: Alexander Potapenko To: Alexey Klimov Cc: Andrey Konovalov , Christoph Lameter , Dmitry Vyukov , Andrew Morton , Steven Rostedt , Joonsoo Kim , Joonsoo Kim , Kostya Serebryany , Andrey Ryabinin , kasan-dev , Linux Memory Management List , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Alexey, On Tue, May 17, 2016 at 12:03 AM, Alexey Klimov wrote: > Hi Alexander, > > On Wed, May 11, 2016 at 6:18 PM, Alexander Potapenko wrote: >> Quarantine isolates freed objects in a separate queue. The objects are >> returned to the allocator later, which helps to detect use-after-free >> errors. >> >> Freed objects are first added to per-cpu quarantine queues. >> When a cache is destroyed or memory shrinking is requested, the objects >> are moved into the global quarantine queue. Whenever a kmalloc call >> allows memory reclaiming, the oldest objects are popped out of the >> global queue until the total size of objects in quarantine is less than >> 3/4 of the maximum quarantine size (which is a fraction of installed >> physical memory). >> >> As long as an object remains in the quarantine, KASAN is able to report >> accesses to it, so the chance of reporting a use-after-free is increased. >> Once the object leaves quarantine, the allocator may reuse it, in which >> case the object is unpoisoned and KASAN can't detect incorrect accesses >> to it. >> >> Right now quarantine support is only enabled in SLAB allocator. >> Unification of KASAN features in SLAB and SLUB will be done later. >> >> This patch is based on the "mm: kasan: quarantine" patch originally >> prepared by Dmitry Chernenkov. A number of improvements have been >> suggested by Andrey Ryabinin. >> >> Signed-off-by: Alexander Potapenko >> --- >> v2: - added copyright comments >> - per request from Joonsoo Kim made __cache_free() more straightforward >> - added comments for smp_load_acquire()/smp_store_release() >> >> v3: - incorporate changes introduced by the "mm, kasan: SLAB support" patch >> >> v4: - fix kbuild compile-time error (missing ___cache_free() declaration) >> and a warning (wrong format specifier) >> >> v6: - extended the patch description >> - dropped the unused qlist_remove() function >> >> v9: - incorporate the fixes by Andrey Ryabinin: >> * Fix comment styles, >> * Get rid of some ifdefs >> * Revert needless functions renames in quarantine patch >> * Remove needless local_irq_save()/restore() in >> per_cpu_remove_cache() >> * Add new 'struct qlist_node' instead of 'void **' types. This makes >> code a bit more redable. >> - remove the non-deterministic quarantine test >> - dropped smp_load_acquire()/smp_store_release() >> --- >> include/linux/kasan.h | 13 ++- >> mm/kasan/Makefile | 1 + >> mm/kasan/kasan.c | 57 ++++++++-- >> mm/kasan/kasan.h | 21 +++- >> mm/kasan/quarantine.c | 291 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> mm/kasan/report.c | 1 + >> mm/mempool.c | 2 +- >> mm/slab.c | 12 ++- >> mm/slab.h | 2 + >> mm/slab_common.c | 2 + >> 10 files changed, 387 insertions(+), 15 deletions(-) >> create mode 100644 mm/kasan/quarantine.c >> >> diff --git a/include/linux/kasan.h b/include/linux/kasan.h >> index 737371b..611927f 100644 >> --- a/include/linux/kasan.h >> +++ b/include/linux/kasan.h >> @@ -50,6 +50,8 @@ void kasan_free_pages(struct page *page, unsigned int order); >> > > [...] > >> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c >> new file mode 100644 >> index 0000000..4973505 >> --- /dev/null >> +++ b/mm/kasan/quarantine.c >> @@ -0,0 +1,291 @@ >> +/* >> + * KASAN quarantine. >> + * >> + * Author: Alexander Potapenko >> + * Copyright (C) 2016 Google, Inc. >> + * >> + * Based on code by Dmitry Chernenkov. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, but >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + * >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "../slab.h" >> +#include "kasan.h" >> + >> +/* Data structure and operations for quarantine queues. */ >> + >> +/* >> + * Each queue is a signle-linked list, which also stores the total size of >> + * objects inside of it. >> + */ >> +struct qlist_head { >> + struct qlist_node *head; >> + struct qlist_node *tail; >> + size_t bytes; >> +}; >> + >> +#define QLIST_INIT { NULL, NULL, 0 } >> + >> +static bool qlist_empty(struct qlist_head *q) >> +{ >> + return !q->head; >> +} >> + >> +static void qlist_init(struct qlist_head *q) >> +{ >> + q->head = q->tail = NULL; >> + q->bytes = 0; >> +} >> + >> +static void qlist_put(struct qlist_head *q, struct qlist_node *qlink, >> + size_t size) >> +{ >> + if (unlikely(qlist_empty(q))) >> + q->head = qlink; >> + else >> + q->tail->next = qlink; >> + q->tail = qlink; >> + qlink->next = NULL; >> + q->bytes += size; >> +} >> + >> +static void qlist_move_all(struct qlist_head *from, struct qlist_head *to) >> +{ >> + if (unlikely(qlist_empty(from))) >> + return; >> + >> + if (qlist_empty(to)) { >> + *to = *from; >> + qlist_init(from); >> + return; >> + } >> + >> + to->tail->next = from->head; >> + to->tail = from->tail; >> + to->bytes += from->bytes; >> + >> + qlist_init(from); >> +} >> + >> +static void qlist_move(struct qlist_head *from, struct qlist_node *last, >> + struct qlist_head *to, size_t size) >> +{ >> + if (unlikely(last == from->tail)) { >> + qlist_move_all(from, to); >> + return; >> + } >> + if (qlist_empty(to)) >> + to->head = from->head; >> + else >> + to->tail->next = from->head; >> + to->tail = last; >> + from->head = last->next; >> + last->next = NULL; >> + from->bytes -= size; >> + to->bytes += size; >> +} > > I see conversation with Andrew in previous emails about moving this > code above into generic library facility but I don't anything is going > on here. I also feel like this belongs to lib/*. > Do I miss something or did you decide to do it later? > > [...] I remember Andrew said that, but I think it will be cleaner to make that change in a separate patch. > -- > Best regards, Klimov Alexey -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg