From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932681AbcFIRJt (ORCPT ); Thu, 9 Jun 2016 13:09:49 -0400 Received: from mail-by2on0109.outbound.protection.outlook.com ([207.46.100.109]:45472 "EHLO na01-by2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932136AbcFIRJr (ORCPT ); Thu, 9 Jun 2016 13:09:47 -0400 From: "Luruo, Kuthonuzo" To: Alexander Potapenko CC: Andrey Ryabinin , Dmitriy Vyukov , Christoph Lameter , "penberg@kernel.org" , David Rientjes , Joonsoo Kim , Andrew Morton , kasan-dev , LKML , "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/hJQAAgAAuqHA= Date: Thu, 9 Jun 2016 16:54:57 +0000 Message-ID: References: <20160607180322.GA1782@cherokee.in.rdlabs.hpecorp.net> In-Reply-To: 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: [106.51.242.76] x-ms-office365-filtering-correlation-id: 4c99bc6d-6648-493a-4372-08d39086ca26 x-microsoft-exchange-diagnostics: 1;DF4PR84MB0090;5:A5iaWOyUpl8YII/mrdqCw4hqGyLdmH0xGPNbeXT6uYM3OKxuz14KDhSMp+MSURTZK4XEkQZ9LcSHMls4FQE6+YriCCqIAAppt44d8UqPPY0jeajQbPrbX5KLHLz028yfnC9RIiDauJlL5VpmYDgd5A==;24:GWy+PSeQi95rqxtHe1MN6d3zydj5j+Zhe2JzShNr5H+SQldF74XA5WMHctVOz/b2VRBumCL2zgONZUeyVgrK2GF+ybjEBrjdKlCnuy7GSP4=;7:/tksTHlAmrh3aiKnzyW3PBn43yZtmC0xBG9rMxi2O25CP+agYogCXg+gn+3N5N6xDXmyBEUs5OrEK2F7mfFTUKX/cmVcVhQWQZRnz5VffZAvdn0fsyhEZjpHk9SA9zpmOSvFbRiqT2V8cvrOzylW7ZYWgXTqoTM+8DMHXDn8pPqOeyyCng+qfgSTImGI0HHHtcnjZAKLkDL2uujZK6q3iVnFKOx3k0QQkgjPkwYAgQs= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0090; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001);SRVR:DF4PR84MB0090;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0090; x-forefront-prvs: 0968D37274 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(199003)(45984002)(189002)(81166006)(106356001)(8676002)(81156014)(3280700002)(9686002)(105586002)(9456002)(2950100001)(3660700001)(106116001)(86362001)(11100500001)(8936002)(99286002)(122556002)(5008740100001)(33656002)(3846002)(586003)(110136002)(102836003)(68736007)(2906002)(77096005)(189998001)(5002640100001)(66066001)(5004730100002)(4326007)(97736004)(6116002)(101416001)(87936001)(5003600100002)(2900100001)(76176999)(92566002)(54356999)(50986999)(10400500002);DIR:OUT;SFP:1102;SCL:1;SRVR:DF4PR84MB0090;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="utf-8" MIME-Version: 1.0 X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Jun 2016 16:54:57.7925 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc X-MS-Exchange-Transport-CrossTenantHeadersStamped: DF4PR84MB0090 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u59H9sfT019018 > > 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. > Note I've sent out a patch that enables stackdepot support in SLUB. > I'll probably need to wait till you patch lands and add locking to SLUB as well. My patch can wait; It can be rebased and resent for consideration by maintainers/reviewers after your patch has been reviewed. > > +void kasan_init_object(struct kmem_cache *cache, void *object) > > +{ > > + if (cache->flags & SLAB_KASAN) { > > + struct kasan_alloc_meta *allocp = get_alloc_info(cache, object); > > + union kasan_shadow_meta *shadow_meta = > get_shadow_meta(allocp); > > + > > + __memset(allocp, 0, sizeof(*allocp)); > I think we need initialize the lock first, then lock it in order to > touch *allocp. > > + shadow_meta->data = KASAN_KMALLOC_META; > Shouldn't this be a release store? Object at this point is being initialized by SLAB off a newly allocated page/slab. Concurrent access to the embryonic object is unlikely/not expected. I don't think locking here is of any benefit... > > default: > > + pr_err("invalid allocation state!\n"); > I suggest you also print the object pointer here. ok. > > + > > struct kasan_alloc_meta { > > + u32 alloc_size : 24; > Why reduce the alloc size? Thought to save some bits while still accounting for max object size instrumented by KASAN. But now, with the spectre of OOB writes, header real estate suddenly seems a lot less valuable ;-). A reset to u32 won’t be inappropriate. > > if (!(cache->flags & SLAB_KASAN)) > > return; > > - switch (alloc_info->state) { > > + if (info->access_size) > > + kasan_meta_lock(alloc_info); > In which case can info->access_size be zero? Guess even in that case > we need to lock the metadata. For a double-free error, access size is zero and lock is already held. Thank you very much for reviewing the patch! Kuthonuzo