From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932149AbdCFRZm (ORCPT ); Mon, 6 Mar 2017 12:25:42 -0500 Received: from foss.arm.com ([217.140.101.70]:38386 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932086AbdCFRZZ (ORCPT ); Mon, 6 Mar 2017 12:25:25 -0500 Date: Mon, 6 Mar 2017 17:25:12 +0000 From: Mark Rutland To: Dmitry Vyukov Cc: Peter Zijlstra , Andrew Morton , Andrey Ryabinin , Ingo Molnar , kasan-dev , "linux-mm@kvack.org" , LKML , "x86@kernel.org" , Will Deacon Subject: Re: [PATCH] x86, kasan: add KASAN checks to atomic operations Message-ID: <20170306172508.GG18519@leverpostej> References: <20170306124254.77615-1-dvyukov@google.com> <20170306125851.GL6500@twins.programming.kicks-ass.net> <20170306130107.GK6536@twins.programming.kicks-ass.net> <20170306162018.GC18519@leverpostej> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 06, 2017 at 05:27:44PM +0100, Dmitry Vyukov wrote: > On Mon, Mar 6, 2017 at 5:20 PM, Mark Rutland wrote: > > On Mon, Mar 06, 2017 at 03:24:23PM +0100, Dmitry Vyukov wrote: > >> On Mon, Mar 6, 2017 at 2:01 PM, Peter Zijlstra wrote: > >> > On Mon, Mar 06, 2017 at 01:58:51PM +0100, Peter Zijlstra wrote: > >> >> On Mon, Mar 06, 2017 at 01:50:47PM +0100, Dmitry Vyukov wrote: > >> >> > On Mon, Mar 6, 2017 at 1:42 PM, Dmitry Vyukov wrote: > >> >> > > KASAN uses compiler instrumentation to intercept all memory accesses. > >> >> > > But it does not see memory accesses done in assembly code. > >> >> > > One notable user of assembly code is atomic operations. Frequently, > >> >> > > for example, an atomic reference decrement is the last access to an > >> >> > > object and a good candidate for a racy use-after-free. > >> >> > > > >> >> > > Add manual KASAN checks to atomic operations. > >> >> > > Note: we need checks only before asm blocks and don't need them > >> >> > > in atomic functions composed of other atomic functions > >> >> > > (e.g. load-cmpxchg loops). > >> >> > > >> >> > Peter, also pointed me at arch/x86/include/asm/bitops.h. Will add them in v2. > >> >> > > >> >> > >> >> > > static __always_inline void atomic_add(int i, atomic_t *v) > >> >> > > { > >> >> > > + kasan_check_write(v, sizeof(*v)); > >> >> > > asm volatile(LOCK_PREFIX "addl %1,%0" > >> >> > > : "+m" (v->counter) > >> >> > > : "ir" (i)); > >> Bottom line: > >> 1. Involving compiler looks quite complex, hard to deploy, and it's > >> unclear if it will actually make things easier. > >> 2. This patch is the simplest short-term option (I am leaning towards > >> adding bitops to this patch and leaving percpu out for now). > >> 3. Providing an implementation of atomic ops based on compiler > >> builtins looks like a nice option for other archs and tools, but is > >> more work. If you consider this as a good solution, we can move > >> straight to this option. > > > > Having *only* seen the assembly snippet at the top of this mail, I can't > > say whether this is the simplest implementation. > > > > However, I do think that annotation of this sort is the only reasonable > > way to handle this. > > Here is the whole patch: > https://groups.google.com/d/msg/kasan-dev/3sNHjjb4GCI/X76pwg_tAwAJ I see. Given we'd have to instrument each architecture's atomics in an identical fashion, maybe we should follow the example of spinlocks, and add an arch_ prefix to the arch-specific implementation, and place the instrumentation in a common wrapper. i.e. have something like: static __always_inline void atomic_inc(atomic_t *v) { kasan_check_write(v, sizeof(*v)); arch_atomic_inc(v); } ... in asm-generic somewhere. It's more churn initially, but it should bea saving overall, and I imagine for KMSAN or other things we may want more instrumentation anyway... Thanks, Mark.