From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 206E2C433F5 for ; Wed, 15 Dec 2021 16:29:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 752F16B0071; Wed, 15 Dec 2021 11:29:11 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6DB486B0073; Wed, 15 Dec 2021 11:29:11 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 52E0F6B0074; Wed, 15 Dec 2021 11:29:11 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0190.hostedemail.com [216.40.44.190]) by kanga.kvack.org (Postfix) with ESMTP id 3E20C6B0071 for ; Wed, 15 Dec 2021 11:29:11 -0500 (EST) Received: from smtpin18.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 02D1D80877 for ; Wed, 15 Dec 2021 16:29:01 +0000 (UTC) X-FDA: 78920562882.18.6B7FD44 Received: from mail-qt1-f179.google.com (mail-qt1-f179.google.com [209.85.160.179]) by imf01.hostedemail.com (Postfix) with ESMTP id EDCD140010 for ; Wed, 15 Dec 2021 16:28:54 +0000 (UTC) Received: by mail-qt1-f179.google.com with SMTP id o17so22374100qtk.1 for ; Wed, 15 Dec 2021 08:28:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qcP01/b9cBNlxBc/sDfkZgSKlphIrcjvX4JKV4TqS64=; b=sV8QbRxgoANmSNSGI0aYUKfO0q1jF/TCKDrGYTsSoT725dNGZRJ6Y4+Y6u3NlgNqGi DES/osX7c856H4zuQ7HkFveyRa1UItoQoC5eryCtl47qkP/L/y0D0+60BiQ1fIzHtuIj H9D+u0GM6+mJyswvExYAeIiKNkrZg4gyu3Cc7kTol7eEjV3vC4prJSoi1+RojXRa2UFy XlORyhY6TIOkoo9sU7znKuU3MShaxLJ6lyQYdl8G4hcyZpr6L97oLr95thnG/62cgr1c GnM7SwwMWMj3b1YC69uIBfyw57jaJ+I3Xow6jLB49fyCCrkXrFj/P0bhBzMitLTlRJtY EowQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qcP01/b9cBNlxBc/sDfkZgSKlphIrcjvX4JKV4TqS64=; b=tsGAK67+ZHzVCLSUOffy8USL99ylt3EBmn7N4Lp3+VP9rXYc2MweLWx8ZXb6j6Tusv Vi+Wg/NgNpKH8f0LKMIxhyFNGfhAir0M6Zsp+SrTl1pkG9eCvU6sRqNVr7yvJ9bVDd0k gaTLA8jxSthx9kkbYFoP5C9VUCrCLmblBIA9wgLsg4OnEhSbVDrq7ECROKx8QOVB2hg0 QQSnCBJ078JQg2onQLWo8+pGpyo7u7EL3DlV8spiEOL3115nZv9IGvSciB7pjW6NpTC+ gDCMguXHF2U827N357pkwV2CwyZEAnqmmhhDDYKJ7FJ5NtoS7Nh9OpG/1EKENkRCkFL9 8yQA== X-Gm-Message-State: AOAM532/jBI78Cg6Vte8a1FVm3ovEkRiTKui223ZPbSJXlTkv5AslZPs yZbOQyioNNlAHcYPSpisNS4sdijTIk5VoychaHu5TA== X-Google-Smtp-Source: ABdhPJy7M0jihSRz6oU87lAajvH4Pb+IxBTZVxkdRp+wYdU4GaQRaMhYcrjUkzXa+GnU9D7a8BF9WhM5llb3k31ruW0= X-Received: by 2002:ac8:4e56:: with SMTP id e22mr12971191qtw.72.1639585737678; Wed, 15 Dec 2021 08:28:57 -0800 (PST) MIME-Version: 1.0 References: <20211214162050.660953-1-glider@google.com> <20211214162050.660953-26-glider@google.com> In-Reply-To: From: Alexander Potapenko Date: Wed, 15 Dec 2021 17:28:21 +0100 Message-ID: Subject: Re: [PATCH 25/43] kmsan: skip shadow checks in files doing context switches To: Mark Rutland Cc: Alexander Viro , Andrew Morton , Andrey Konovalov , Andy Lutomirski , Ard Biesheuvel , Arnd Bergmann , Borislav Petkov , Christoph Hellwig , Christoph Lameter , David Rientjes , Dmitry Vyukov , Eric Dumazet , Greg Kroah-Hartman , Herbert Xu , Ilya Leoshkevich , Ingo Molnar , Jens Axboe , Joonsoo Kim , Kees Cook , Marco Elver , Matthew Wilcox , "Michael S. Tsirkin" , Pekka Enberg , Peter Zijlstra , Petr Mladek , Steven Rostedt , Thomas Gleixner , Vasily Gorbik , Vegard Nossum , Vlastimil Babka , linux-mm@kvack.org, linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: EDCD140010 X-Stat-Signature: jyjskqybmy96pesnxh5m6m7o141nj5rg Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=sV8QbRxg; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf01.hostedemail.com: domain of glider@google.com designates 209.85.160.179 as permitted sender) smtp.mailfrom=glider@google.com X-HE-Tag: 1639585734-251715 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Dec 15, 2021 at 3:13 PM Mark Rutland wrote: > > On Tue, Dec 14, 2021 at 05:20:32PM +0100, Alexander Potapenko wrote: > > When instrumenting functions, KMSAN obtains the per-task state (mostly > > pointers to metadata for function arguments and return values) once per > > function at its beginning. > > How does KMSAN instrumentation acquire the per-task state? What's used as the > base for that? > To preserve kernel ABI (so that instrumented functions can call non-instrumented ones and vice versa) KMSAN uses a per-task struct that keeps shadow values of function call parameters and return values: struct kmsan_context_state { char param_tls[...]; char retval_tls[...]; char va_arg_tls[...]; char va_arg_origin_tls[...]; u64 va_arg_overflow_size_tls; depot_stack_handle_t param_origin_tls[...]; depot_stack_handle_t retval_origin_tls; }; It is mostly dealt with by the compiler, so its layout isn't really important. The compiler inserts a call to __msan_get_context_state() at the beginning of every instrumented function to obtain a pointer to that struct. Then, every time a function pointer is used, a value is returned, or another function is called, the compiler adds code that updates the shadow values in this struct. E.g. the following function: int sum(int a, int b) { ... result = a + b; return result; } will now look as follows: int sum(int a, int b) { struct kmsan_context_state *kcs = __msan_get_context_state(); int s_a = ((int)kcs->param_tls)[0]; // shadow of a int s_b = ((int)kcs->param_tls)[1]; // shadow of b ... result = a + b; s_result = s_a | s_b; ((int)kcs->retval_tls)[0] = s_result; // returned shadow return result; } > > > > To deal with that, we need to apply __no_kmsan_checks to the functions > > performing context switching - this will result in skipping all KMSAN > > shadow checks and marking newly created values as initialized, > > preventing all false positive reports in those functions. False negatives > > are still possible, but we expect them to be rare and impersistent. > > > > To improve maintainability, we choose to apply __no_kmsan_checks not > > just to a handful of functions, but to the whole files that may perform > > context switching - this is done via KMSAN_ENABLE_CHECKS:=n. > > This decision can be reconsidered in the future, when KMSAN won't need > > so much attention. > > I worry this might be the wrong approach (and I've given some rationale below), > but it's not clear to me exactly how this goes wrong. Could you give an example > flow where stale data gets used? The scheme I described above works well until a context switch occurs. Then, IIUC, at some point `current` changes, so that the previously fetched KMSAN context state becomes stale: void foo(...) { baz(...); // context switch here changes `current` baz(...); } In this case we'll have foo() setting up kmsan_context_state for the old task when calling bar(), but bar() taking shadow for its arguments from the new task's kmsan_context_state. Does this make sense? > As above, the actual context-switch occurs in arch code --I assume the > out-of-line call *must* act as a clobber from the instrumentation's PoV or we'd > have many more problems. Treating a function call as a clobber of kmsan_context_state() is actually an interesting idea. Adding yet another call to __msan_get_context_state() after every function call may sound harsh, but we already instrument every memory access anyway. What remains unclear is handling the return value of the innermost function that performed the switch: it will be saved to the old task's state, but taken from that of the new task. > I also didn't spot any *explciit* state switching > being added there that would seem to affect KMSAN. > > ... so I don't understand why checks need to be inhibited for the core sched code. In fact for a long time there were only three functions annotated with __no_kmsan_checks right in arch/x86/kernel/process_64.c and kernel/sched/core.c We decided to apply this attribute to every function in both files, just to make sure nothing breaks too early while upstreaming KMSAN.