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 EDC0BC4332F for ; Tue, 12 Dec 2023 19:29:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5F8FA6B0348; Tue, 12 Dec 2023 14:29:20 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5A85F6B0349; Tue, 12 Dec 2023 14:29:20 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 421E36B034A; Tue, 12 Dec 2023 14:29:20 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 2FA8A6B0348 for ; Tue, 12 Dec 2023 14:29:20 -0500 (EST) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 03C1740734 for ; Tue, 12 Dec 2023 19:29:19 +0000 (UTC) X-FDA: 81559154880.12.075218C Received: from mail-vs1-f42.google.com (mail-vs1-f42.google.com [209.85.217.42]) by imf21.hostedemail.com (Postfix) with ESMTP id 1A63B1C0016 for ; Tue, 12 Dec 2023 19:29:16 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=RL+v0ziv; spf=pass (imf21.hostedemail.com: domain of elver@google.com designates 209.85.217.42 as permitted sender) smtp.mailfrom=elver@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1702409357; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=+rCbqARyeZId11kuEaXWxOwQK5N8S8sXeciQALfHIFM=; b=R56FfL4zF+u/aKwe6sekQrwhO2REaBtUh0j/f14su8SUn2Crml2dvxIsvUrfmvH5sSv86T 99Plm7B0r/cTw49TLYKQ2QwoAzkBawUHYpeL6N/yW5QjSuGCZxlSudW9Rpa85rPw+5oavd 8PF/bATgLprgTfn4nLOcq4X9ghr48Bo= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702409357; a=rsa-sha256; cv=none; b=iOcYCS8PxHdDYc/ebvSmgN9p+0bT6uwr1T+Nw5OrLdx/1B1PEVjBVLTV5dO+hFiitHOE9X 9VvVQH0Ytn7ff9Vss4G/DSFdeedlNwkh8L26JaaF0SmTZS6F0p/2BspIs0Dyvh9DQZzDzP LPibSgdrUHsn7OJED+NKJkt9+314Wjg= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=RL+v0ziv; spf=pass (imf21.hostedemail.com: domain of elver@google.com designates 209.85.217.42 as permitted sender) smtp.mailfrom=elver@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-vs1-f42.google.com with SMTP id ada2fe7eead31-4649299d0a0so1667581137.2 for ; Tue, 12 Dec 2023 11:29:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1702409356; x=1703014156; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=+rCbqARyeZId11kuEaXWxOwQK5N8S8sXeciQALfHIFM=; b=RL+v0zivAHpvnfEMWeZRzUlREmWm8naY9KxhcO7O7RBGS9rmYZ+WjSJhFqfaUG0kpN 43V3wCBEzMemLb0ogfllqBARC20yQoWAZ9fKQdfMBWGzVJiXXiyD8+wXqFKZWhcnI1zV CduWnCcrXuQMg/X2B3eUUNnYxY4tIlTQYJK9DE7ynTi0YwWDbtT/9LHMGmdL9bl0oQ3J KQKryndJFtC1+seKqSkTWrab3g7pKI/TdA6o3pkk5m3Qu0W3uw796kRJ+zJjj/Jr15o3 nbFFLBFTxI+ibr/JCuAJ05JdW5YWsPERrmDzdrJvKKUh6U6h/WVxCUDoyCYZ8mkChAhc 2snQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702409356; x=1703014156; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=+rCbqARyeZId11kuEaXWxOwQK5N8S8sXeciQALfHIFM=; b=Kw0gRB8sgbZRpqQNru/hB7j6SzcK1PYSSJYEftjmylqeUPV6Finp5LmIB5koUUFIZo y72fsH5ZiJOrGKjp8qmXhGSznG7kpW//78P1j5UTMOM4vrKhBtzHnLwNvGXW4DKx/9w+ 2HHVrcubYyaIajuaSf/tYARokM5/sFlDOOTJmqT+I+SheBzDyu5N+R5x0+30WC67HLIy Sccu+uGTBhOrGiBEbiO+iiho9IIf1RiesY1b9d8/CMuGP4p/bkVuKwty1WnxSyL+Y9Xr 1OuTJhHcb5rwD2Awuox9whsEBCtSvbHlCnoFv3G7GGmmk1TMTGJDOfCASODWsC7UluWd pLqQ== X-Gm-Message-State: AOJu0YwMaxIgFpvJDP3wcBsrkQYaPrM/lhk52E1C5sXPwggeMQof84kf 3QbhF0bW7jydi72C9+diWfbSqLZxT9FVzqRdJnItoQ== X-Google-Smtp-Source: AGHT+IE2Gq+T080mI4W9k254GYp8ITOPgtjAZhh3RmjHvRQ2ZF9QpqQNSfW7Nkl4sinuKbhhMM63IsxN2Dmr8+nPclo= X-Received: by 2002:a05:6102:3752:b0:464:3cdb:856c with SMTP id u18-20020a056102375200b004643cdb856cmr5344796vst.9.1702409356064; Tue, 12 Dec 2023 11:29:16 -0800 (PST) MIME-Version: 1.0 References: <432a89fafce11244287c8af757e73a2eb22a5354.1702339432.git.andreyknvl@google.com> In-Reply-To: <432a89fafce11244287c8af757e73a2eb22a5354.1702339432.git.andreyknvl@google.com> From: Marco Elver Date: Tue, 12 Dec 2023 20:28:37 +0100 Message-ID: Subject: Re: [PATCH mm 2/4] kasan: handle concurrent kasan_record_aux_stack calls To: andrey.konovalov@linux.dev Cc: Andrew Morton , Andrey Konovalov , Alexander Potapenko , Dmitry Vyukov , Vlastimil Babka , kasan-dev@googlegroups.com, Evgenii Stepanov , Tetsuo Handa , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrey Konovalov , syzbot+186b55175d8360728234@syzkaller.appspotmail.com Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 1A63B1C0016 X-Rspam-User: X-Stat-Signature: dp56tq3uh67r5ak431wk9eqpjtsmrxwu X-Rspamd-Server: rspam03 X-HE-Tag: 1702409356-252455 X-HE-Meta: U2FsdGVkX1+06jzfz34+4ppjShgvC0xde1p1sIgCQY0gYVbI/UgQJpKF+HLEfRefJFNLCsShtj/oUtX0W4LZlbLhDtZZnQj6viimLjlwqkBnMWRTNfKrIOt0rgKVb8KOvtxcFamlolBe4MbRZSRL0vfiNifGX8vrQ7cm+Q0dKdEYxZ5D2cNEzXnrYOBVaZVq2hNocD2zsVzcrl5Xoiiekt64k6KNnZnOL/jUb4yJ2CF5lY2edE6fw2NuTfRf+7QHNTGoSTwdYDoXGGaekTns4PEnzTqWPNmTkK7FYgicWMdAvB9aeR7gPWoXLdBYDedRPVxSPleEHpVcbtd8CaqaMlQuvjoVlv3JN1HbiWkCx91PaanxfSFkv3jbvJpEJA1Bv92D+MfuCy51vIRtqFc9np4Z+ip8GGxl5WAkueuZaN1cly8fQYbwR8+hbIrfsn4icSTkYyEZOHxU8+ZOBB4xLIi18fYS4Mhr2lSIb+WtbdSMg1cO+CmmR879/9ZLkVUMV51uisVR1g9ErBlnC+Uw8AAJUJbm8/5i9Wag/0CVP8kO84hcgI9KIm+TBZBOTLv/z5xr0i+UGtoa5pbLuk7s4WJfp8B/C3Ko3B1JL4suQ0+1iQA74+R576YlCbGJDzMhmGTq8aVEBJML/BRHHWP7iArMktmazZQi3m+9fJ/XPtC6yUWP9oGUgQ8iPttKyzp2yCjJnQEl0fbQchFhutY3G9Zpr9t511aKoOV2ezxNQJLU52ft1h5EHVjyKcWETOTepZlET8Qrv1FgvS+B7NIu5pJZNmGkUPU24Yc9DNNw6d6StgiggMh9Af9gUtDtxD3POgmTbpuqH33Vsp/AU6zKKB70UsA22GZ2l4m/ioFTkRVpJoIVWdhynMiNBWDy3byLi5xIWCxHJW29BQzBA5JFUwqho/nm95wHH/mq+jh75ifjDGvq1BycsCyfBpEP8HcHQNiF+p84UMxnaau4+f4 L49a4cia IoRuTMJ1trg/bAEKEugME9jsjkVWmO7CLh30vLP/HjadvLyBNZttKs5o8uYbmj8pUWs5pAAXZSATn7CQTGYxYSLmv+ho/jfevS5Q3Ov7tbocYOLNJRujxfmAbxI6Z1RpVHjjW8nFRHC5NPi0DF06G9FFjKbeK7mmRrZCYogAZNU1Gm2aAEtCL5wBDHs7gb2k+92xS9xOHszcwDYS527Q3WWXYqyuVC2d19g3UhBJ2cMyLGNV5sjDXJyIfyIK+V/F2mBEHnUzw8vnlx2ILLhLCtYCiKT/ej4CvldvqOJf/VkeASYYooKt/xsNW62WjNOLM4NNSLE3lehsv5W/lsMEPveBek8RcrhAwPKcr9O68U+kYa4WubpzsgIW6hp8/W0C6qOMoe78p4vSItfVhjvHcpCxDYtJADNeIVzxwsLJS/YzmAw3XO4gw9WQshZ+X8FaQC+JPoHwamgksiB6PKP+JKW1BcopJYqaRomcPwdhU+OFwRvXJATrjvjJrCOmHOdy9jKwF 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: List-Subscribe: List-Unsubscribe: On Tue, 12 Dec 2023 at 01:14, wrote: > > From: Andrey Konovalov > > kasan_record_aux_stack can be called concurrently on the same object. > This might lead to a race condition when rotating the saved aux stack > trace handles. > > Fix by introducing a spinlock to protect the aux stack trace handles > in kasan_record_aux_stack. > > Reported-by: Tetsuo Handa > Reported-by: syzbot+186b55175d8360728234@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/all/000000000000784b1c060b0074a2@google.com/ > Signed-off-by: Andrey Konovalov > > --- > > This can be squashed into "kasan: use stack_depot_put for Generic mode" > or left standalone. > --- > mm/kasan/generic.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c > index 54e20b2bc3e1..ca5c75a1866c 100644 > --- a/mm/kasan/generic.c > +++ b/mm/kasan/generic.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -35,6 +36,8 @@ > #include "kasan.h" > #include "../slab.h" > > +DEFINE_SPINLOCK(aux_lock); No, please don't. > /* > * All functions below always inlined so compiler could > * perform better optimizations in each of __asan_loadX/__assn_storeX > @@ -502,6 +505,8 @@ static void __kasan_record_aux_stack(void *addr, depot_flags_t depot_flags) > struct kmem_cache *cache; > struct kasan_alloc_meta *alloc_meta; > void *object; > + depot_stack_handle_t new_handle, old_handle; > + unsigned long flags; > > if (is_kfence_address(addr) || !slab) > return; > @@ -512,9 +517,15 @@ static void __kasan_record_aux_stack(void *addr, depot_flags_t depot_flags) > if (!alloc_meta) > return; > > - stack_depot_put(alloc_meta->aux_stack[1]); > + new_handle = kasan_save_stack(0, depot_flags); > + > + spin_lock_irqsave(&aux_lock, flags); This is a unnecessary global lock. What's the problem here? As far as I can understand a race is possible where we may end up with duplicated or lost stack handles. Since storing this information is best effort anyway, and bugs are rare, a global lock protecting this is overkill. I'd just accept the racyness and use READ_ONCE() / WRITE_ONCE() just to make sure we don't tear any reads/writes and the depot handles are valid. There are other more complex schemes [1], but I think they are overkill as well. [1]: Since a depot stack handle is just an u32, we can have a union { depot_stack_handle_t handles[2]; atomic64_t atomic_handle; } aux_stack; (BUILD_BUG_ON somewhere if sizeof handles and atomic_handle mismatch.) Then in the code here create the same union and load atomic_handle. Swap handle[1] into handle[0] and write the new one in handles[1]. Then do a cmpxchg loop to store the new atomic_handle. > + old_handle = alloc_meta->aux_stack[1]; > alloc_meta->aux_stack[1] = alloc_meta->aux_stack[0]; > - alloc_meta->aux_stack[0] = kasan_save_stack(0, depot_flags); > + alloc_meta->aux_stack[0] = new_handle; > + spin_unlock_irqrestore(&aux_lock, flags); > + > + stack_depot_put(old_handle); > } > > void kasan_record_aux_stack(void *addr) > -- > 2.25.1 >