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 2B332C5478C for ; Sat, 24 Feb 2024 18:04:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6A6986B00A2; Sat, 24 Feb 2024 13:04:27 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 657146B00A3; Sat, 24 Feb 2024 13:04:27 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 51E006B00A4; Sat, 24 Feb 2024 13:04:27 -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 431876B00A2 for ; Sat, 24 Feb 2024 13:04:27 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 067AC1A0311 for ; Sat, 24 Feb 2024 18:04:27 +0000 (UTC) X-FDA: 81827472174.24.5295888 Received: from mail-oa1-f46.google.com (mail-oa1-f46.google.com [209.85.160.46]) by imf19.hostedemail.com (Postfix) with ESMTP id 69EE31A000B for ; Sat, 24 Feb 2024 18:04:24 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=JAcUAhJL; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf19.hostedemail.com: domain of elver@google.com designates 209.85.160.46 as permitted sender) smtp.mailfrom=elver@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1708797864; 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=102AgdscnmlaS5ECv7WvZDiPmEMRao2nvjNOKWRVZbk=; b=geMA5EBs0pokAeqDddt4HvAZMGMKMkNZJSigE20Yd1jZZpwzgLP8djIv5+Bw9M5LEiq6vU pg3CVnErCwaCG6MgmH4UDBmWZyoGXx+WRhrUDEVrvdRuqhEfWrqG70qd/6QVRimD4X8DPK pjFcdN+cAypwncrq33Vtj1wzrs7lPOk= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=JAcUAhJL; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf19.hostedemail.com: domain of elver@google.com designates 209.85.160.46 as permitted sender) smtp.mailfrom=elver@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708797864; a=rsa-sha256; cv=none; b=LRVH+vH90o/2Yl2a8wdQdyqx+dcVkBOlRCgBKLw4xRMrGg5PX1KIgA0T9QbPKsLwKXh2PA OguoMSLvQNztMNNffOEqMc6ErE4zS1ocwK2XD9Fj6wIeGiX8fZe4YjAf6CnDViEPn1RHvX sYMDI0PaJ07GHCoDcU+4rkpUi51umd4= Received: by mail-oa1-f46.google.com with SMTP id 586e51a60fabf-2184133da88so1029562fac.0 for ; Sat, 24 Feb 2024 10:04:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1708797863; x=1709402663; 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=102AgdscnmlaS5ECv7WvZDiPmEMRao2nvjNOKWRVZbk=; b=JAcUAhJL7xgHHOSAdTiofoJm5WR+gWVhdFTdeQicORoLVXQqsCrIxPIP4VcIOD1oKt TQOFsJ9WS2HDYjjUYF2ZTQ/qFaFV+AmTFQqClc22t4r2WIXRsejrQ+iIQCCP8pXWFkfD 9CIqrdaUkKkrjgwcDplKTfPD9oZ1F7pd7POyYwUjTeGXNl/SMD1GShhEuGAk+c0+vOoY MVy8mVQj9OgM5TBeKwfNJcYUNH00zyFkiZ+zsiWGyjbI0G05Kgok/2ivxIg1MsPsZpOj LtKWMPNI9ZNmZcUK2YUtKaek/BmkFWWlM/ZHyW27d5caa42d43NH4VNYv5sopCeUeZeT bpJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708797863; x=1709402663; 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=102AgdscnmlaS5ECv7WvZDiPmEMRao2nvjNOKWRVZbk=; b=GDLsNzTakM30LB1ou4fhja0kJYjT8FbZintIFdQnQDxLNx7bJHwt0kB5FJFWGPX5bS /jp+FewRYW9iwhDlhcodQ9Qk12nN8Z8oYETTVIYtsp/6F1WOV7Cpp4+9GKlW6qL4Dn08 BpR64cCD2B5/cYxfqlG1ScBAtCHP742W8sLjL08CckO9XAgKGf0qZvdEZ+Xk5V9SuLbw krA7oDPHnoViQVz17mqsQ7Oq9bIh6JCyLeAFLgLdN6KIIkNRxCBSyV/arZrCnxCnboKY nL0L9UAd2JuwRdgCzTK1p1x1paz9r1Wr/fe1x/u1tyGbUGqSUJnpnN3lUhaoxYTLZ5fN EYoQ== X-Forwarded-Encrypted: i=1; AJvYcCUb+PL3r54HrcKsBfbOdlQJUQWiZus0rlb5sVKpyAaEnd0Rw3oImOOgQ3DQYdxfv61MxfqiwzxlJpUWnDCHawo6HOY= X-Gm-Message-State: AOJu0YzAP5YGmKyVn+6XXZMG8Mra7j9e9BEnW4yzNeYB1LoIvSjVbY5Q jPz07yS6HFP9Bcsfc16qmy61NrppwGHMJi9Nb2JdMRjD5yAE0E+aEodaqgl9leRlyNbdW+soiw8 T/PnkbHNaNP5mevcIgVdHHPFSvkQ15My9YFyb X-Google-Smtp-Source: AGHT+IEueiJifIy5PBmEevZCFfKe/665gTKlu7uhEFMSmEb9/6gUQUr4fsknnVl2fwSxobspFmh+QKZ6NWBoelmQTio= X-Received: by 2002:a05:6870:9589:b0:21f:d09e:b185 with SMTP id k9-20020a056870958900b0021fd09eb185mr2256417oao.42.1708797863322; Sat, 24 Feb 2024 10:04:23 -0800 (PST) MIME-Version: 1.0 References: <20240118110216.2539519-1-elver@google.com> <20240118110216.2539519-2-elver@google.com> In-Reply-To: From: Marco Elver Date: Sat, 24 Feb 2024 19:03:44 +0100 Message-ID: Subject: Re: [PATCH 2/2] stackdepot: make fast paths lock-less again To: Tetsuo Handa Cc: Andrey Konovalov , Alexander Potapenko , Dmitry Vyukov , Vlastimil Babka , linux-mm@kvack.org, kasan-dev@googlegroups.com, Andi Kleen , Andrew Morton Content-Type: text/plain; charset="UTF-8" X-Rspam-User: X-Stat-Signature: kf4jmqyg9fnq9w3uak3sta7a8fswmfyq X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 69EE31A000B X-HE-Tag: 1708797864-801189 X-HE-Meta: U2FsdGVkX19o6PP1kr8fRP2wk1mpWI7bK/06OU5+kO/0j/YPoNZjlqA4wnoe2Hcc1I5ovTbbGyhk4QR3PRwTvoa1XVZyfya7YQa5/ERIB6omeFO/ajds27upl1+wUQZQzWIdy37J6YoOyxe8NCWttQNWVFIWT0JtV9rrkJ8p/fSUmCRVWVBmlW16O0HWsQz/H69RVJ2Et+Hpq8nskFnh3hJ601LmRvgbla2qfCuJ0ji8W9NQSWLaRqkMh2gBgM/yKIBg5zFZqCzTDIOskk424uw9fIcE+lbcdIM/Er4NiCrh5pZaGYHSH9zwCiOJWmLCoMj2wzPQDTLt1CSRymPBGAU0PJKidfS7y/smuPc3XiRQ1Am1l3ubCIIfYaHy2k2SNgqiauqrhNX6S8AQGeNRUTKiZ7KxEBqyy7R4IEDoOsphiNNNZdPSFBSI4gbDHt2YnT4hF16al6kPfFCCmgtZOL5h9932VGbayWPjh/ejXwl6TTFTDPiJtxoW9psxjvNqGeTRpDUDtj6R5P/+tcDp3/Yz6gukytx8Hll6pESmfB0jaTaMqYcHgqlsywBK0apsZZR3+bFvIGPGONy5TsPBWm+Jwa0yu8pjycoEAOowzH2tEZDyPMJ835tYtu6HUSxTz3RN9MoAJTtGKkrBMVbKMGC4dzb2EM8FVDTEdXqrjSDQPEf3IQd5ButQgTLB7JVk9lEQwpZ8S95tEWCvQfh8ufhNmn8sf0NL3x/EbvziOz/hdjruUPR2VsN300JvSgOfVBRo6Jg4qZ2RzpG5DQRfqFHxlRkBCes0FqXXg3dCYpBIX2prBJSsWuVGHvkKbaFTMOYODOz1P93YUO8pj7VTeugxv7MwwR024M/rwzAgUPu+4BbO3rOQ232bXZVcSBvAhR5e1s3CPFvwsbOlIm664Lfp9dCY13ymWsaB6SCyQhwCZ9FkZtlZrxj6D0R5wBK9MnK7M2BAOLWgCcNLsRP 2TA== 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 Sat, 24 Feb 2024 at 12:38, Tetsuo Handa wrote: > > Hello, stackdepot developers. > > I suspect that commit 4434a56ec209 ("stackdepot: make fast paths > lock-less again") is not safe, for > https://syzkaller.appspot.com/x/error.txt?x=1409f29a180000 is reporting > UAF at list_del() below from stack_depot_save_flags(). > > ---------- > + /* > + * We maintain the invariant that the elements in front are least > + * recently used, and are therefore more likely to be associated with an > + * RCU grace period in the past. Consequently it is sufficient to only > + * check the first entry. > + */ > + stack = list_first_entry(&free_stacks, struct stack_record, free_list); > + if (stack->size && !poll_state_synchronize_rcu(stack->rcu_state)) > + return NULL; > + > + list_del(&stack->free_list); > + counters[DEPOT_COUNTER_FREELIST_SIZE]--; > ---------- > > Commit 4434a56ec209 says that race is handled by refcount_inc_not_zero(), but > refcount_inc_not_zero() is called only if STACK_DEPOT_FLAG_GET is specified. Correct. Because it is invalid stackdepot usage to have unbalanced GET and stack_depot_put(). > ---------- > + list_for_each_entry_rcu(stack, bucket, hash_list) { > + if (stack->hash != hash || stack->size != size) > + continue; > > - lockdep_assert_held(&pool_rwlock); > + /* > + * This may race with depot_free_stack() accessing the freelist > + * management state unioned with @entries. The refcount is zero > + * in that case and the below refcount_inc_not_zero() will fail. > + */ > + if (data_race(stackdepot_memcmp(entries, stack->entries, size))) > + continue; > > - list_for_each(pos, bucket) { > - found = list_entry(pos, struct stack_record, list); > - if (found->hash == hash && > - found->size == size && > - !stackdepot_memcmp(entries, found->entries, size)) > - return found; > + /* > + * Try to increment refcount. If this succeeds, the stack record > + * is valid and has not yet been freed. > + * > + * If STACK_DEPOT_FLAG_GET is not used, it is undefined behavior > + * to then call stack_depot_put() later, and we can assume that > + * a stack record is never placed back on the freelist. > + */ > + if ((flags & STACK_DEPOT_FLAG_GET) && !refcount_inc_not_zero(&stack->count)) > + continue; > + > + ret = stack; > + break; > } > ---------- > > I worried that if we race when STACK_DEPOT_FLAG_GET is not specified, > depot_alloc_stack() by error overwrites stack->free_list via memcpy(stack->entries, ...), > and invalid memory access happens when stack->free_list.next is read. > Therefore, I tried https://syzkaller.appspot.com/text?tag=Patch&x=17a12a30180000 > but did not help ( https://syzkaller.appspot.com/x/error.txt?x=1423a4ac180000 ). > > Therefore, I started to suspect how stack_depot_save() (which does not set > STACK_DEPOT_FLAG_GET) can be safe. Don't all callers need to set STACK_DEPOT_FLAG_GET > when calling stack_depot_save_flags() and need to call stack_depot_put() ? stackdepot users who do not use STACK_DEPOT_FLAG_GET must never call stack_depot_put() on such entries. Violation of this contract will lead to UAF errors. >From the report I see this is a KMSAN error. There is a high chance this is a false positive. Have you tried it with this patch: https://lore.kernel.org/all/20240124173134.1165747-1-glider@google.com/T/#u