From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762702AbbA3QEs (ORCPT ); Fri, 30 Jan 2015 11:04:48 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:61033 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756643AbbA3QEq (ORCPT ); Fri, 30 Jan 2015 11:04:46 -0500 X-AuditID: cbfec7f4-b7f126d000001e9a-7e-54cbab09927a Message-id: <54CBAB92.6090108@samsung.com> Date: Fri, 30 Jan 2015 19:04:34 +0300 From: Andrey Ryabinin User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-version: 1.0 To: Andrew Morton Cc: linux-kernel@vger.kernel.org, Dmitry Vyukov , Konstantin Serebryany , Dmitry Chernenkov , Andrey Konovalov , Yuri Gribov , Konstantin Khlebnikov , Sasha Levin , Christoph Lameter , Joonsoo Kim , Dave Hansen , Andi Kleen , x86@kernel.org, linux-mm@kvack.org, Jonathan Corbet , Michal Marek , Ingo Molnar , Peter Zijlstra , "open list:DOCUMENTATION" , "open list:KERNEL BUILD + fi..." Subject: Re: [PATCH v10 01/17] Add kernel address sanitizer infrastructure. References: <1404905415-9046-1-git-send-email-a.ryabinin@samsung.com> <1422544321-24232-1-git-send-email-a.ryabinin@samsung.com> <1422544321-24232-2-git-send-email-a.ryabinin@samsung.com> <20150129151213.09d1f9e0a01490712d0eb071@linux-foundation.org> In-reply-to: <20150129151213.09d1f9e0a01490712d0eb071@linux-foundation.org> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrIIsWRmVeSWpSXmKPExsVy+t/xq7qcq0+HGKz9LGvxe+9MVos569ew WRy59p3d4vq3N4wWTw60M1p8evmA0eL5w4fsFhMetrFbrOxuZrPY/uwtk8XKzgesFgvblrBY /Nm1g8ni8q45bBb31vxntbh0YAGTRcu+C0wWx3sPMFksPnKb2eLds8nMFj82PGZ1EPWYv/Mj o8fOWXfZPRZsKvXYvELLY/Gel0wem1Z1snls+jSJ3aPr7RUmjxMzfrN4PLkyncljcd9kVo+P T2+xeLzfd5XN48yCI+wenzfJBfBHcdmkpOZklqUW6dslcGV0//zGWjBHsuLSiT9MDYytIl2M HBwSAiYSDe9Cuxg5gUwxiQv31rN1MXJxCAksZZSYNe8BlNPMJHH19mt2kCpeAS2JmT9msoDY LAKqEk+f/QCLswnoSfybtZ0NxBYViJD4sOorG0S9oMSPyffA6kUEdCVWPd/FDGIzC3xllWic FQZiCwt4S/w4eoEJxBYSaGCSOPlMEsTmBIrPa9/ODnIoM9D8+xe1IFrlJTavecs8gVFgFpIN sxCqZiGpWsDIvIpRNLU0uaA4KT3XUK84Mbe4NC9dLzk/dxMjJJq/7GBcfMzqEKMAB6MSD++N xNMhQqyJZcWVuYcYJTiYlUR4p0wCCvGmJFZWpRblxxeV5qQWH2Jk4uCUamDs4frPq18S/vzn x9PT1T5vaTBo+ru648BGh9xv3uEn1092bekTU2axfs14Vma6qpZ5fs6djxLqB3+eMMitq4gW 7Vp1cmYr74ZVFk46y3vjw/VPcyo3yr1Sfcyeb/tjqvetiZoWdXULNh8yTguRnc7x47bd6h2L nvPaHF02+frngLhU7ZmbTW2VWIozEg21mIuKEwFLZDEhxAIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/30/2015 02:12 AM, Andrew Morton wrote: > On Thu, 29 Jan 2015 18:11:45 +0300 Andrey Ryabinin wrote: > >> Kernel Address sanitizer (KASan) is a dynamic memory error detector. It provides >> fast and comprehensive solution for finding use-after-free and out-of-bounds bugs. >> >> KASAN uses compile-time instrumentation for checking every memory access, >> therefore GCC >= v4.9.2 required. >> >> ... >> >> Based on work by Andrey Konovalov > > Can we obtain Andrey's signed-off-by: please? > I'll ask. ... >> +static __always_inline bool memory_is_poisoned_1(unsigned long addr) > > What's with all the __always_inline in this file? When I remove them > all, kasan.o .text falls from 8294 bytes down to 4543 bytes. That's > massive, and quite possibly faster. > > If there's some magical functional reason for this then can we please > get a nice prominent comment into this code apologetically explaining > it? > The main reason is performance. __always_inline especially needed for check_memory_region() and memory_is_poisoned() to optimize away switch in memory_is_poisoned(): if (__builtin_constant_p(size)) { switch (size) { case 1: return memory_is_poisoned_1(addr); case 2: return memory_is_poisoned_2(addr); case 4: return memory_is_poisoned_4(addr); case 8: return memory_is_poisoned_8(addr); case 16: return memory_is_poisoned_16(addr); default: BUILD_BUG(); } } Always inlining memory_is_poisoned_x() gives additionally about 7%-10%. According to my simple testing __always_inline gives about 20% versus not inlined version of kasan.c ... >> + >> +void __asan_loadN(unsigned long addr, size_t size) >> +{ >> + check_memory_region(addr, size, false); >> +} >> +EXPORT_SYMBOL(__asan_loadN); >> + >> +__attribute__((alias("__asan_loadN"))) > > Maybe we need a __alias. Like __packed and various other helpers. > Ok. .... >> + >> +static __always_inline void kasan_report(unsigned long addr, >> + size_t size, >> + bool is_write) > > Again, why the inline? This is presumably not a hotpath and > kasan_report has sixish call sites. > The reason of __always_inline here is to get correct _RET_IP_. I could pass it from above and drop always inline here. > >> +{ >> + struct access_info info; >> + >> + if (likely(!kasan_enabled())) >> + return; >> + >> + info.access_addr = addr; >> + info.access_size = size; >> + info.is_write = is_write; >> + info.ip = _RET_IP_; >> + kasan_report_error(&info); >> +} >> ... >> + >> +static void print_address_description(struct access_info *info) >> +{ >> + dump_stack(); >> +} > > dump_stack() uses KERN_INFO but the callers or > print_address_description() use KERN_ERR. This means that at some > settings of `dmesg -n', the kasan output will have large missing > chunks. > > Please test this and deide how bad it is. A proper fix will be > somewhat messy (new_dump_stack(KERN_ERR)). > This new_dump_stack() could be useful in other places. E.g. object_err()/slab_err() in SLUB also use pr_err() + dump_stack() combination.