From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751712AbcFNHSG (ORCPT ); Tue, 14 Jun 2016 03:18:06 -0400 Received: from mail-bn1on0140.outbound.protection.outlook.com ([157.56.110.140]:22804 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751409AbcFNHSE convert rfc822-to-8bit (ORCPT ); Tue, 14 Jun 2016 03:18:04 -0400 X-Greylist: delayed 1025 seconds by postgrey-1.27 at vger.kernel.org; Tue, 14 Jun 2016 03:18:04 EDT From: "Luruo, Kuthonuzo" To: Andrey Ryabinin CC: "glider@google.com" , "dvyukov@google.com" , "cl@linux.com" , "penberg@kernel.org" , "rientjes@google.com" , "iamjoonsoo.kim@lge.com" , "akpm@linux-foundation.org" , "kasan-dev@googlegroups.com" , "linux-kernel@vger.kernel.org" , "ynorov@caviumnetworks.com" Subject: RE: [PATCH v5 1/2] mm, kasan: improve double-free detection Thread-Topic: [PATCH v5 1/2] mm, kasan: improve double-free detection Thread-Index: AQHRwOb4yyHEEEIC00C2P9n5DEQOrZ/hXweAgAGTJoCABZhZEA== Date: Tue, 14 Jun 2016 06:46:04 +0000 Message-ID: References: <20160607180322.GA1782@cherokee.in.rdlabs.hpecorp.net> <5759A0A9.3080301@virtuozzo.com> <575AF2D9.7030701@virtuozzo.com> In-Reply-To: <575AF2D9.7030701@virtuozzo.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=kuthonuzo.luruo@hpe.com; x-originating-ip: [15.219.195.8] x-ms-office365-filtering-correlation-id: 10f3a011-c126-40ee-b605-08d3941f8ed5 x-microsoft-exchange-diagnostics: 1;DF4PR84MB0089;5:6slCy5bobUo9ZSov2OHbE6kRjIe0ZH8xU1HS0/QTA4L7Wd8Hqt0jpCz/DFKq5zO08sek2xdrma/hHj2UCENHJLpaaJiuYQ1M5Wg3zKyg+d0QER5JmZdFpLWvlGh0rEkN7QbHcbGxcZSPpnBL1r4+YQ==;24:JkMV8GW5Sse5iAaNs9Aa/Fa7ZOzC9hEPf+SOVWtGq4o367AUkVBnXr++xaEJZQzfsW6QRkd9vG5/0WDi3SzTX7zCQkXCWlhpYvuVKWmBOBo=;7:wOe3y1rfcfDkTsdxdBXttmGxTWtFDrb3j+cNq1MKyC5w3qO3QDglhtXo0aSJSP4wDQYz76HEwkM/Qe/Yu8qWJlLgkSutG62DVofNPzeOdhaNc2ceoDEYagJ+J0jm1U0lxCXXiBWQu9qje4DE3kj/haw2AKa2PP/DA0AjOZNTignUmMlsP26HVV9ks3mxkEHS1equinRbb1rquLDNHLW28Gz9LJKo3UlMS9rhEyQCW3Y= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0089; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(20558992708506); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001);SRVR:DF4PR84MB0089;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0089; x-forefront-prvs: 09730BD177 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(7916002)(51914003)(199003)(189002)(3280700002)(3660700001)(68736007)(8936002)(77096005)(6116002)(10400500002)(102836003)(3846002)(586003)(19580395003)(9686002)(5008740100001)(92566002)(189998001)(110136002)(66066001)(106116001)(97736004)(106356001)(105586002)(2950100001)(15975445007)(5004730100002)(11100500001)(99286002)(2900100001)(87936001)(33656002)(76176999)(50986999)(54356999)(8676002)(81156014)(81166006)(4326007)(5003600100002)(122556002)(2906002)(575784001)(101416001)(86362001)(5002640100001);DIR:OUT;SFP:1102;SCL:1;SRVR:DF4PR84MB0089;H:DF4PR84MB0089.NAMPRD84.PROD.OUTLOOK.COM;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Jun 2016 06:46:04.9858 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc X-MS-Exchange-Transport-CrossTenantHeadersStamped: DF4PR84MB0089 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > > Next time, when/if you send patch series, send patches in one thread, i.e. > > patches should be replies to the cover letter. > > Your patches are not linked together, which makes them harder to track. Thanks for the tip; but doesn't this conflict with the advice in https://www.kernel.org/doc/Documentation/SubmittingPatches, specifically the use of the "summary phrase"... > > > > > >> Currently, KASAN may fail to detect concurrent deallocations of the same > >> object due to a race in kasan_slab_free(). This patch makes double-free > >> detection more reliable by serializing access to KASAN object metadata. > >> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to > >> lock/unlock per-object metadata. Double-free errors are now reported via > >> kasan_report(). > >> > >> Per-object lock concept from suggestion/observations by Dmitry Vyukov. > >> > > > > > > So, I still don't like this, this too way hacky and complex. I don't think patch is particularly complex; but respect your judgment. > > I have some thoughts about how to make this lockless and robust enough. > > I'll try to sort this out tomorrow. > > > > > So, I something like this should work. > Tested very briefly. > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index ac4b3c4..8691142 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -75,6 +75,8 @@ struct kasan_cache { > int kasan_module_alloc(void *addr, size_t size); > void kasan_free_shadow(const struct vm_struct *vm); > > +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object); > + > size_t ksize(const void *); > static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); } > > @@ -102,6 +104,9 @@ static inline void kasan_unpoison_object_data(struct > kmem_cache *cache, > static inline void kasan_poison_object_data(struct kmem_cache *cache, > void *object) {} > > +static inline void kasan_init_slab_obj(struct kmem_cache *cache, > + const void *object) { } > + > static inline void kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags) {} > static inline void kasan_kfree_large(const void *ptr) {} > static inline void kasan_poison_kfree(void *ptr) {} > diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c > index 6845f92..ab0fded 100644 > --- a/mm/kasan/kasan.c > +++ b/mm/kasan/kasan.c > @@ -388,11 +388,9 @@ void kasan_cache_create(struct kmem_cache *cache, > size_t *size, > *size += sizeof(struct kasan_alloc_meta); > > /* Add free meta. */ > - if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor || > - cache->object_size < sizeof(struct kasan_free_meta)) { > - cache->kasan_info.free_meta_offset = *size; > - *size += sizeof(struct kasan_free_meta); > - } > + cache->kasan_info.free_meta_offset = *size; > + *size += sizeof(struct kasan_free_meta); > + > redzone_adjust = optimal_redzone(cache->object_size) - > (*size - cache->object_size); > if (redzone_adjust > 0) > @@ -431,13 +429,6 @@ void kasan_poison_object_data(struct kmem_cache > *cache, void *object) > kasan_poison_shadow(object, > round_up(cache->object_size, > KASAN_SHADOW_SCALE_SIZE), > KASAN_KMALLOC_REDZONE); > -#ifdef CONFIG_SLAB > - if (cache->flags & SLAB_KASAN) { > - struct kasan_alloc_meta *alloc_info = > - get_alloc_info(cache, object); > - alloc_info->state = KASAN_STATE_INIT; > - } > -#endif > } > > #ifdef CONFIG_SLAB > @@ -501,6 +492,20 @@ struct kasan_free_meta *get_free_info(struct > kmem_cache *cache, > BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32); > return (void *)object + cache->kasan_info.free_meta_offset; > } > + > +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object) > +{ > + struct kasan_alloc_meta *alloc_info; > + struct kasan_free_meta *free_info; > + > + if (!(cache->flags & SLAB_KASAN)) > + return; > + > + alloc_info = get_alloc_info(cache, object); > + free_info = get_free_info(cache, object); > + __memset(alloc_info, 0, sizeof(*alloc_info)); > + __memset(free_info, 0, sizeof(*free_info)); > +} > #endif > > void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags) > @@ -523,37 +528,47 @@ static void kasan_poison_slab_free(struct > kmem_cache *cache, void *object) > bool kasan_slab_free(struct kmem_cache *cache, void *object) > { > #ifdef CONFIG_SLAB > + struct kasan_free_meta *free_info = get_free_info(cache, object); > + struct kasan_track new_free_stack, old_free_stack; > + s8 old_shadow; > + > /* RCU slabs could be legally used after free within the RCU period */ > if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU)) > return false; > > - if (likely(cache->flags & SLAB_KASAN)) { > - struct kasan_alloc_meta *alloc_info = > - get_alloc_info(cache, object); > - struct kasan_free_meta *free_info = > - get_free_info(cache, object); > - > - switch (alloc_info->state) { > - case KASAN_STATE_ALLOC: > - alloc_info->state = KASAN_STATE_QUARANTINE; > - quarantine_put(free_info, cache); > - set_track(&free_info->track, GFP_NOWAIT); > - kasan_poison_slab_free(cache, object); > - return true; > - case KASAN_STATE_QUARANTINE: > - case KASAN_STATE_FREE: > - pr_err("Double free"); > - dump_stack(); > - break; > - default: > - break; > - } > + if (unlikely(!(cache->flags & SLAB_KASAN))) > + return false; > + > + set_track(&new_free_stack, GFP_NOWAIT); > + old_free_stack = xchg(&free_info->track, new_free_stack); > + old_shadow = xchg((s8 *)kasan_mem_to_shadow(object), > + KASAN_KMALLOC_FREE); > + > + if (old_shadow < 0 || old_shadow >= KASAN_SHADOW_SCALE_SIZE) { > + struct kasan_track free_stack; > + > + /* Paired with xchg() above */ > + free_stack = smp_load_acquire(&free_info->track); > + > + /* > + * We didn't raced with another instance of kasan_slab_free() > + * so the previous free stack supposed to be in old_free_stack. > + * Otherwise, free_stack will contain stack trace of another > + * kfree() call. > + */ > + if (free_stack.id == new_free_stack.id) > + free_stack = old_free_stack; > + > + kasan_report_double_free(cache, object, > + free_stack, old_shadow); > + return false; > } > - return false; > -#else > kasan_poison_slab_free(cache, object); > - return false; > + return true; > + > #endif > + kasan_poison_slab_free(cache, object); > + return false; > } > > void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size, > @@ -581,7 +596,6 @@ void kasan_kmalloc(struct kmem_cache *cache, const > void *object, size_t size, > struct kasan_alloc_meta *alloc_info = > get_alloc_info(cache, object); > > - alloc_info->state = KASAN_STATE_ALLOC; > alloc_info->alloc_size = size; > set_track(&alloc_info->track, flags); > } > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h > index fb87923..9b46d2e 100644 > --- a/mm/kasan/kasan.h > +++ b/mm/kasan/kasan.h > @@ -59,24 +59,21 @@ struct kasan_global { > * Structures to keep alloc and free tracks * > */ > > -enum kasan_state { > - KASAN_STATE_INIT, > - KASAN_STATE_ALLOC, > - KASAN_STATE_QUARANTINE, > - KASAN_STATE_FREE > -}; > - > #define KASAN_STACK_DEPTH 64 > > struct kasan_track { > - u32 pid; > - depot_stack_handle_t stack; > +union { > + struct { > + u32 pid; > + depot_stack_handle_t stack; > + }; > + u64 id; > +}; > }; > > struct kasan_alloc_meta { > struct kasan_track track; > - u32 state : 2; /* enum kasan_state */ > - u32 alloc_size : 30; > + u32 alloc_size; > }; > > struct qlist_node { > @@ -109,6 +106,9 @@ static inline bool kasan_report_enabled(void) > > void kasan_report(unsigned long addr, size_t size, > bool is_write, unsigned long ip); > +void kasan_report_double_free(struct kmem_cache *cache, void *object, > + struct kasan_track free_stack, s8 shadow); > + > > #ifdef CONFIG_SLAB > void quarantine_put(struct kasan_free_meta *info, struct kmem_cache > *cache); > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c > index 4973505..3ec039c 100644 > --- a/mm/kasan/quarantine.c > +++ b/mm/kasan/quarantine.c > @@ -144,11 +144,9 @@ static void *qlink_to_object(struct qlist_node *qlink, > struct kmem_cache *cache) > static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache) > { > void *object = qlink_to_object(qlink, cache); > - struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object); > unsigned long flags; > > local_irq_save(flags); > - alloc_info->state = KASAN_STATE_FREE; > ___cache_free(cache, object, _THIS_IP_); > local_irq_restore(flags); > } > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index b3c122d..a0f4519 100644 > --- a/mm/kasan/report.c > +++ b/mm/kasan/report.c > @@ -140,28 +140,13 @@ static void object_err(struct kmem_cache *cache, > struct page *page, > pr_err("Object at %p, in cache %s\n", object, cache->name); > if (!(cache->flags & SLAB_KASAN)) > return; > - switch (alloc_info->state) { > - case KASAN_STATE_INIT: > - pr_err("Object not allocated yet\n"); > - break; > - case KASAN_STATE_ALLOC: > - pr_err("Object allocated with size %u bytes.\n", > - alloc_info->alloc_size); > - pr_err("Allocation:\n"); > - print_track(&alloc_info->track); > - break; > - case KASAN_STATE_FREE: > - case KASAN_STATE_QUARANTINE: > - pr_err("Object freed, allocated with size %u bytes\n", > - alloc_info->alloc_size); > - free_info = get_free_info(cache, object); > - pr_err("Allocation:\n"); > - print_track(&alloc_info->track); > - pr_err("Deallocation:\n"); > - print_track(&free_info->track); > - break; > - } > + free_info = get_free_info(cache, object); > + pr_err("Allocation:\n"); > + print_track(&alloc_info->track); > + pr_err("Deallocation:\n"); > + print_track(&free_info->track); > } > + > #endif > > static void print_address_description(struct kasan_access_info *info) > @@ -245,17 +230,31 @@ static void print_shadow_for_address(const void > *addr) > > static DEFINE_SPINLOCK(report_lock); > > -static void kasan_report_error(struct kasan_access_info *info) > +static void kasan_start_report(unsigned long *flags) > { > - unsigned long flags; > - const char *bug_type; > - > /* > * Make sure we don't end up in loop. > */ > kasan_disable_current(); > - spin_lock_irqsave(&report_lock, flags); > + spin_lock_irqsave(&report_lock, *flags); > > pr_err("==================================================== > ==============\n"); > +} > + > +static void kasan_end_report(unsigned long *flags) > +{ > + > pr_err("==================================================== > ==============\n"); > + add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE); > + spin_unlock_irqrestore(&report_lock, *flags); > + kasan_enable_current(); > +} > + > +static void kasan_report_error(struct kasan_access_info *info) > +{ > + unsigned long flags; > + const char *bug_type; > + > + kasan_start_report(&flags); > + > if (info->access_addr < > kasan_shadow_to_mem((void > *)KASAN_SHADOW_START)) { > if ((unsigned long)info->access_addr < PAGE_SIZE) > @@ -276,10 +275,29 @@ static void kasan_report_error(struct > kasan_access_info *info) > print_address_description(info); > print_shadow_for_address(info->first_bad_addr); > } > - > pr_err("==================================================== > ==============\n"); > - add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE); > - spin_unlock_irqrestore(&report_lock, flags); > - kasan_enable_current(); > + > + kasan_end_report(&flags); > +} > + > +void kasan_report_double_free(struct kmem_cache *cache, void *object, > + struct kasan_track free_stack, s8 shadow) > +{ > + unsigned long flags; > + > + kasan_start_report(&flags); > + > + pr_err("BUG: Double free or corrupt pointer\n"); > + pr_err("Unexpected shadow byte: 0x%hhX\n", shadow); > + > + dump_stack(); > + pr_err("Object at %p, in cache %s\n", object, cache->name); > + get_alloc_info(cache, object); > + pr_err("Allocation:\n"); > + print_track(&get_alloc_info(cache, object)->track); > + pr_err("Deallocation:\n"); > + print_track(&free_stack); > + > + kasan_end_report(&flags); > } > > void kasan_report(unsigned long addr, size_t size, > diff --git a/mm/slab.c b/mm/slab.c > index 763096a..65c942b 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -2604,9 +2604,11 @@ static void cache_init_objs(struct kmem_cache > *cachep, > } > > for (i = 0; i < cachep->num; i++) { > + objp = index_to_obj(cachep, page, i); > + kasan_init_slab_obj(cachep, objp); > + > /* constructor could break poison info */ > if (DEBUG == 0 && cachep->ctor) { > - objp = index_to_obj(cachep, page, i); > kasan_unpoison_object_data(cachep, objp); > cachep->ctor(objp); > kasan_poison_object_data(cachep, objp); Nice hack & novel approach. It does have the flaw that subsequent error reports for object will have a bogus deallocation track. Kuthonuzo