From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932622AbcFIQof (ORCPT ); Thu, 9 Jun 2016 12:44:35 -0400 Received: from mail-am1on0103.outbound.protection.outlook.com ([157.56.112.103]:1920 "EHLO emea01-am1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932089AbcFIQod (ORCPT ); Thu, 9 Jun 2016 12:44:33 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=aryabinin@virtuozzo.com; Subject: Re: [PATCH] mm, kasan: switch SLUB to stackdepot, enable memory quarantine for SLUB To: Alexander Potapenko , , , , , , , , , References: <1465411243-102618-1-git-send-email-glider@google.com> CC: , , From: Andrey Ryabinin Message-ID: <57599D1C.2080701@virtuozzo.com> Date: Thu, 9 Jun 2016 19:45:16 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <1465411243-102618-1-git-send-email-glider@google.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [195.214.232.10] X-ClientProxiedBy: HE1PR03CA0036.eurprd03.prod.outlook.com (10.163.170.174) To DB6PR0801MB1303.eurprd08.prod.outlook.com (10.168.11.21) X-MS-Office365-Filtering-Correlation-Id: c62459d1-1513-4dad-802a-08d390855335 X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1303;2:ZFD7Nu/OeiDQQLpjFO7+OxOTaD9MTSzucn8rTy1rTjCg+kp6kF5faUZ+UVd9DEM2tYx2b83SYJWMDmqzzotsdC8SsIlvigs1ZdkiopO8m8vcvPKR1o2MvUs3QHToB5K1I+9tjDyinx+QrCvYZS/fV2hjUkRBUrxhIwxUUEQMmfl7M7C8rEJcSdbuKu5k9zbW;3:u8N+1dbMW5UEFyNNEV7g7PVj6aDLkkBm3jdfsFPwWy5rpJpN9VYJVMFI1/RaKNfdpll4mloKQH3VxMoXaA/KTsTnHRye5rUsKEJ2DqqpkjFf+Ob9++DQJE1Yn5PUMncH;25:tmLLxcuVXKYZPuDTHqOHhbEdQgF/4VtCVITEEvmxjSg/x4nNujYOA5Jk5tHwTe6zrc3joH1HGTphulcHmLs9Wdk8G9ZJhg8hb7LWdt49Dsv8nnMRwPd0ofQ0FVMQHO93BwCGw02cw7QSwFMVvMOBmi46W/gRzV3tpqsb9Z27SY5PbS0f4uKMIDX68z3atBHRuslwtOI6UnYSiH3XmAdMwmKrl51wWkc4P+sD6Psy3vBmXqiDUsQYei8M/xUq+6fX31T5cgWbq6gvbJG2bOQwQ5imAuViKh2ltmqZrd/h6G50/6wMnJHw6wG9tKJp69kjm/bq/Fp0iVh9QWgW+eWC9YCW7mkP946xICC6AjWdeUZsb1qQP6tBzKraGDuSvRUJC2jS4Ffbjz548XpS0Llu3jAINj9jNto7QQbUPHPRZcY= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DB6PR0801MB1303; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040130)(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046)(6041072)(6043046);SRVR:DB6PR0801MB1303;BCL:0;PCL:0;RULEID:;SRVR:DB6PR0801MB1303; X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1303;4:tgUjnTUoRjEGEuUHkYgHtKgf2wLOCPnTMBkaU2d8/02+g587qz+TewyfJnFQOpLtXYFMEe8qwsWJyRAobB8U8bsulrEdT+pxjoiRT4CG8j9vVXTw0c3Lzcys+Gq9+5zGPMcPxFlA8AMN4ieJ7NQemNI98QCA3U9iwh2tYlqi5zTSn239sbKBdMClnB4SgdhzQV1b2QNa5+3mQw6PRYR/mZzDbH8exRFiHpMfn2IQ8JKPJ42qD3QTwRmEi/vhhjEOrlElLAglAYEtlbQceijX/bCi40cNGtj+8K4RgPrre2ABNEfaiOnnUxo8HIAuCkQ39vcaD98ZpJs5cGr6BhABczHEtf2+TyqNwdvmHc9Z3SjM8FKg2Dra5/goqlRDlQNq0lITXBKpVop/hwFVYKW+mB4cH0IVTKpAKeICx7doSSY= X-Forefront-PRVS: 0968D37274 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6049001)(6009001)(24454002)(377454003)(189002)(199003)(106356001)(65816999)(50986999)(65806001)(47776003)(77096005)(50466002)(66066001)(101416001)(65956001)(189998001)(4326007)(76176999)(99136001)(87266999)(54356999)(92566002)(68736007)(3846002)(105586002)(86362001)(36756003)(5004730100002)(64126003)(6116002)(2201001)(2950100001)(4001350100001)(42186005)(83506001)(230700001)(23746002)(586003)(59896002)(5001770100001)(33656002)(2906002)(80316001)(8676002)(81156014)(97736004)(81166006)(5008740100001)(921003)(1121003)(83996005)(2101003)(473944003);DIR:OUT;SFP:1102;SCL:1;SRVR:DB6PR0801MB1303;H:[10.30.19.223];FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?Windows-1252?Q?1;DB6PR0801MB1303;23:C68l54Xq4nGVLJYRaiuX6/PUGANWaK3xiBC?= =?Windows-1252?Q?ojqkImRHf5SzINqu0kJYrHQBmZWw+yEmD0dfnv5E5tu5w54PDDCGNIfj?= =?Windows-1252?Q?n7E8+YQlh8X7PEUR3mXR2v4EHvIt7deURZBIwv4U18ZH/ulyBGquq5Se?= =?Windows-1252?Q?v6i0P9hl4e4sqZEHocDp590YFI2aCWy6qBHA7YTMucfvLydEcaof3S5e?= =?Windows-1252?Q?8XtKf8cwF8yBMa0z9mkxE3zf5aac3PYT1tQkjbmF2nstbyUW/M3DcANf?= =?Windows-1252?Q?mtvzJMqjPRShZvOcMU+k3Fq7kPZORN6u/Umk59tMQY/TxarEKJOB9iX0?= =?Windows-1252?Q?UDXKWP0JmWc8OVp8HR+Kxz2pt6B5goge5bzrs97HbT0+wn8RTASxHOuX?= =?Windows-1252?Q?TPd6TUR1FYXvaMxhZ5l+bzuoVwcKvnyBk3IgwWF8EhYP/RtVIlzXhHja?= =?Windows-1252?Q?W58BFRf+mpXR5WZVEkc8HK/XoyFm/lT2FdhmLCwx9dSeofECSrKAy12k?= =?Windows-1252?Q?Ez2YH83QSeFgrXb885LmWhhFlaGrdHWZzOw2BbnPF5pFa6afKe1QPAc/?= =?Windows-1252?Q?OUIc0VVWKKNdoTwXIjv5E4ml1/clTlAib7bwwxW46hNt+HcBiVhvqCAx?= =?Windows-1252?Q?HWpZNlFh/AoSSn4KEmipJQYGBfE6nP+E1BIFnGitvI1N6s7KCaQmXLqX?= =?Windows-1252?Q?Y3A6+3eQ9tS6gQvw4DgbXImYuJaagtAX5d6RgiMAGgRpHfGpfrRGFCMw?= =?Windows-1252?Q?nbZEidxqteUeIHJDiu4rZBkYTWqbuebflnWNJlf9sgy8rhAJoPnNAPX7?= =?Windows-1252?Q?GA9JPh46jhf/lBGdgFK8CUqK58FZC/hJY9lwZ5bs1HHMQnhWnd/sBSYX?= =?Windows-1252?Q?LtNLfF0jJzIbulKqh1OqJ1VcrtXTKIUGd7SfrBven/d/54yzVb3HQ9Jq?= =?Windows-1252?Q?zZZ+/ZJ4lN7StaA7AM5X6Vxg5hK7b0GnGzlomLG/4KfonVFZAKIw32h5?= =?Windows-1252?Q?NLZGG3j8M02L3eoHClGlcCgat7fkcv4d1RNB9eUqm9NJVLNV7CoISDDg?= =?Windows-1252?Q?qoDVoM/IPZcJo6unI7nGsbjdMqJQf6Yl6MTdGD8Ct/Puu5KZg9m6HDt3?= =?Windows-1252?Q?REpkwXTUYo4neyl7/qU8OMrAwWCDFI5sc7WQ3q0o828L5Nt+u40HMnkf?= =?Windows-1252?Q?aMBTPIeG6/yeRDA8Eu1Gdf/lkBX/mCcxYAK7j9IILhtEENa99OEvn46T?= =?Windows-1252?Q?5LZMk3CFVafg5Nf21c36fkOy3pdW98iAFpOERryqy6lnsES7eJZXVTRr?= =?Windows-1252?Q?Ut2xpEixc3E7Kf2r7odnygYQGX2BSKpHpIig0yxAVw0hRUNlBdmQ+m1l?= =?Windows-1252?Q?EaJkbuYyjyH4/xNWhrfscGa9o5DrBtgGB0acHMoK+VKRN67Gz0EZcJcZ?= =?Windows-1252?Q?KOuumCnS+pyID3Nhj0WRM/zKonb1G0lSdp29noonR8Q=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1303;5:Sg745gv7+IA2WCUly5J+XOGrfla27YBlL1FGqywXw5OSWkyEwauyas9z8aaZU77c8cCLTdStj97MO4KfeAc3sqqzuJVaO2to/NzfIZv84fuSvdti8lrLDzTEsseUnM96w4UwT0hYK0DRzGdm3Rhe1w==;24:D6XM239kv4Y9y8sLrrmA+daQGmIF90LjEegFr55fTtXs8TQob6mIBeRzpJ4aqqfeEzglzvL7VZlv0CaoCDKWICZqrs5yXE6+3cbIiLMMpu4=;7:6WgJmmq88dN621T+USmk0sDcLhPqG6XAPDLMJgZXZjA4WVlJaiGfhusHU2BkTClAVy4YLsYVH4yl/41s74NML9iZjfLveJaDZ5c/AhHjbCydjxhZDU9pQbxXRYHd9fwlQC/X1MK6GQREeg/pUqZuhq0qAFd/LYP32hEqjmUe5FMkZE3cO0WiWTseg1qXc1CkOj0s5Fm6MzZNycKHT2XVVLSe9hnE2OAWU+w2a8q8cX8=;20:3/At19ovO5+nS4oJNGarLfU2k7CPi1NDV3MKTELalc/bpYuhsVwv42f/Wvx0XwOFAlLz5eau3837dqIjqj+kbeK/orgodJUBbL96HZD1FZy7hcCTCTQuPXanVIAlEjCBNh6fQp9jbxTMSe2U4z2/J9jqY78y7Y/CYojhY/ZsDYw= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: virtuozzo.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jun 2016 16:44:27.0436 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0801MB1303 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/08/2016 09:40 PM, Alexander Potapenko wrote: > For KASAN builds: > - switch SLUB allocator to using stackdepot instead of storing the > allocation/deallocation stacks in the objects; > - define SLAB_RED_ZONE, SLAB_POISON, SLAB_STORE_USER to zero, > effectively disabling these debug features, as they're redundant in > the presence of KASAN; Instead of having duplicated functionality, I think it might be better to switch SLAB_STORE_USER to stackdepot instead. Because now, we have two piles of code which do basically the same thing, but do differently. > - refactor the slab freelist hook, put freed memory into the quarantine. > What you did with slab_freelist_hook() is not refactoring, it's an obfuscation. > } > > -#ifdef CONFIG_SLAB > /* > * Adaptive redzone policy taken from the userspace AddressSanitizer runtime. > * For larger allocations larger redzones are used. > @@ -372,17 +371,21 @@ static size_t optimal_redzone(size_t object_size) > void kasan_cache_create(struct kmem_cache *cache, size_t *size, > unsigned long *flags) > { > - int redzone_adjust; > - /* Make sure the adjusted size is still less than > - * KMALLOC_MAX_CACHE_SIZE. > - * TODO: this check is only useful for SLAB, but not SLUB. We'll need > - * to skip it for SLUB when it starts using kasan_cache_create(). > + int redzone_adjust, orig_size = *size; > + > +#ifdef CONFIG_SLAB > + /* > + * Make sure the adjusted size is still less than > + * KMALLOC_MAX_CACHE_SIZE, i.e. we don't use the page allocator. > */ > + > if (*size > KMALLOC_MAX_CACHE_SIZE - This is wrong. You probably wanted KMALLOC_MAX_SIZE here. However, we should get rid of SLAB_KASAN altogether. It's absolutely useless, and only complicates the code. And if we don't fit in KMALLOC_MAX_SIZE, just don't create cache. > sizeof(struct kasan_alloc_meta) - > sizeof(struct kasan_free_meta)) > return; > +#endif > *flags |= SLAB_KASAN; > + > /* Add alloc meta. */ > cache->kasan_info.alloc_meta_offset = *size; > *size += sizeof(struct kasan_alloc_meta); > @@ -392,17 +395,37 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size, > cache->object_size < sizeof(struct kasan_free_meta)) { > cache->kasan_info.free_meta_offset = *size; > *size += sizeof(struct kasan_free_meta); > + } else { > + cache->kasan_info.free_meta_offset = 0; > } > redzone_adjust = optimal_redzone(cache->object_size) - > (*size - cache->object_size); > + > if (redzone_adjust > 0) > *size += redzone_adjust; > + > +#ifdef CONFIG_SLAB > *size = min(KMALLOC_MAX_CACHE_SIZE, > max(*size, > cache->object_size + > optimal_redzone(cache->object_size))); > -} > + /* > + * If the metadata doesn't fit, disable KASAN at all. > + */ > + if (*size <= cache->kasan_info.alloc_meta_offset || > + *size <= cache->kasan_info.free_meta_offset) { > + *flags &= ~SLAB_KASAN; > + *size = orig_size; > + cache->kasan_info.alloc_meta_offset = -1; > + cache->kasan_info.free_meta_offset = -1; > + } > +#else > + *size = max(*size, > + cache->object_size + > + optimal_redzone(cache->object_size)); > + > #endif > +} > > void kasan_cache_shrink(struct kmem_cache *cache) > { > @@ -431,16 +454,14 @@ 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; > + if (alloc_info) If I read the code right alloc_info can be NULL only if SLAB_KASAN is set. > + alloc_info->state = KASAN_STATE_INIT; > } > -#endif > } > > -#ifdef CONFIG_SLAB > static inline int in_irqentry_text(unsigned long ptr) > { > return (ptr >= (unsigned long)&__irqentry_text_start && > @@ -492,6 +513,8 @@ struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache, > const void *object) > { > BUILD_BUG_ON(sizeof(struct kasan_alloc_meta) > 32); > + if (cache->kasan_info.alloc_meta_offset == -1) > + return NULL; What's the point of this ? This should be always false. > return (void *)object + cache->kasan_info.alloc_meta_offset; > } > > @@ -499,9 +522,10 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache, > const void *object) > { > BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32); > + if (cache->kasan_info.free_meta_offset == -1) > + return NULL; > return (void *)object + cache->kasan_info.free_meta_offset; > } > -#endif > > void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags) > { > @@ -522,7 +546,6 @@ void kasan_poison_slab_free(struct kmem_cache *cache, void *object) > > bool kasan_slab_free(struct kmem_cache *cache, void *object) > { > -#ifdef CONFIG_SLAB > /* RCU slabs could be legally used after free within the RCU period */ > if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU)) > return false; > @@ -532,7 +555,10 @@ bool kasan_slab_free(struct kmem_cache *cache, void *object) > get_alloc_info(cache, object); > struct kasan_free_meta *free_info = > get_free_info(cache, object); > - > + WARN_ON(!alloc_info); > + WARN_ON(!free_info); > + if (!alloc_info || !free_info) > + return; Again, never possible. > switch (alloc_info->state) { > case KASAN_STATE_ALLOC: > alloc_info->state = KASAN_STATE_QUARANTINE; > @@ -550,10 +576,6 @@ bool kasan_slab_free(struct kmem_cache *cache, void *object) > } > } > return false; > -#else > - kasan_poison_slab_free(cache, object); > - return false; > -#endif > } > > void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size, > @@ -568,24 +590,29 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size, > if (unlikely(object == NULL)) > return; > > + if (!(cache->flags & SLAB_KASAN)) > + return; > + > redzone_start = round_up((unsigned long)(object + size), > KASAN_SHADOW_SCALE_SIZE); > redzone_end = round_up((unsigned long)object + cache->object_size, > KASAN_SHADOW_SCALE_SIZE); > > kasan_unpoison_shadow(object, size); > + WARN_ON(redzone_start > redzone_end); > + if (redzone_start > redzone_end) How that's can happen? > + return; > kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start, > 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_ALLOC; > - alloc_info->alloc_size = size; > - set_track(&alloc_info->track, flags); > + if (alloc_info) { And again... > + alloc_info->state = KASAN_STATE_ALLOC; > + alloc_info->alloc_size = size; > + set_track(&alloc_info->track, flags); > + } > } > -#endif > } > EXPORT_SYMBOL(kasan_kmalloc); > [..] > diff --git a/mm/slab.h b/mm/slab.h > index dedb1a9..fde1fea 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -366,6 +366,10 @@ static inline size_t slab_ksize(const struct kmem_cache *s) > if (s->flags & (SLAB_RED_ZONE | SLAB_POISON)) > return s->object_size; > # endif > +# ifdef CONFIG_KASAN Gush, you love ifdefs, don't you? Hint: it's redundant here. > + if (s->flags & SLAB_KASAN) > + return s->object_size; > +# endif > /* > * If we have the need to store the freelist pointer ...