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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6C403C43334 for ; Sat, 2 Jul 2022 17:31:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232221AbiGBRbz (ORCPT ); Sat, 2 Jul 2022 13:31:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39722 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231520AbiGBRbx (ORCPT ); Sat, 2 Jul 2022 13:31:53 -0400 Received: from mail-ej1-x631.google.com (mail-ej1-x631.google.com [IPv6:2a00:1450:4864:20::631]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 389C2BC9C for ; Sat, 2 Jul 2022 10:31:52 -0700 (PDT) Received: by mail-ej1-x631.google.com with SMTP id sb34so9321650ejc.11 for ; Sat, 02 Jul 2022 10:31:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=il+ZyDBxn0EXlULxf75c3oKM6Oz9Nzs2kT9ulcG3ntg=; b=AvpeJ2y3obDMtoTmhFwHDaHq3gpaWuFkFb9SFOECVzvF7Ut7PQt4V7wdroBR0/cx5M SxIzCPADDmpAnDfJ7WM0KQFaDT5+R57nujC4MFCdgjYwnwVx93plj8J4wWckxLtNwStl HXFMwtmdOD06ghqq5vTm5wQM0VyefeNQxnfuQ= 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=il+ZyDBxn0EXlULxf75c3oKM6Oz9Nzs2kT9ulcG3ntg=; b=pmSa0pN9mpMLNt9IofYX0qtUglN6043kFUPDMWC7plGQa9E+fnI8tn+qzl4oVYDWlV shMv7+fiFN+D7vz32Rn+ThGre8PAimYb66b9uEqqHyw5EYqSxTcBXOHj8huhNn4ue81l IKmU3TSmw8kSumrLt6p7HklBsCgB5cIDVwY7GgSZb0+yOmKs9hG9NgAAKWjaEc/Glqn+ yyiqQDDG4gYgkmmuKM/gGRhBJapZ8TgYN7g0/obpyVdOk6/OprtXTBDjdM3FtM+AQQJm xHjTxyZ8HAbRZhNMocgcJhOak8SpKK6Q9CWeVV1eLMjYSkAko1+sFt4QLY6gCGq1sHzn y9ig== X-Gm-Message-State: AJIora8i1Ey+rigfhmmY63dIVi9d9iLg2KYNOjY/RmYYKMwjjrgpx5jW mToJCOdlPiGuoOI6SxxWcJi0XVaD1KFt9CC6vt8= X-Google-Smtp-Source: AGRyM1shAq6esmoD0CCUtRY4FHHTKeLoB4FD67z+7lTdlfBG8o+0fJJn8NQ/fiINMh9n/i13Ezm76A== X-Received: by 2002:a17:906:7007:b0:6ff:8028:42e with SMTP id n7-20020a170906700700b006ff8028042emr20283006ejj.278.1656783110501; Sat, 02 Jul 2022 10:31:50 -0700 (PDT) Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com. [209.85.218.47]) by smtp.gmail.com with ESMTPSA id kw25-20020a170907771900b0072aa38d8938sm1082635ejc.149.2022.07.02.10.31.50 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 02 Jul 2022 10:31:50 -0700 (PDT) Received: by mail-ej1-f47.google.com with SMTP id fi2so9358219ejb.9 for ; Sat, 02 Jul 2022 10:31:50 -0700 (PDT) X-Received: by 2002:a5d:64e7:0:b0:21b:ad72:5401 with SMTP id g7-20020a5d64e7000000b0021bad725401mr18424110wri.442.1656782613069; Sat, 02 Jul 2022 10:23:33 -0700 (PDT) MIME-Version: 1.0 References: <20220701142310.2188015-1-glider@google.com> <20220701142310.2188015-44-glider@google.com> In-Reply-To: <20220701142310.2188015-44-glider@google.com> From: Linus Torvalds Date: Sat, 2 Jul 2022 10:23:16 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 43/45] namei: initialize parameters passed to step_into() To: Alexander Potapenko Cc: Alexander Viro , Alexei Starovoitov , Andrew Morton , Andrey Konovalov , Andy Lutomirski , 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 , Mark Rutland , Matthew Wilcox , "Michael S. Tsirkin" , Pekka Enberg , Peter Zijlstra , Petr Mladek , Steven Rostedt , Thomas Gleixner , Vasily Gorbik , Vegard Nossum , Vlastimil Babka , kasan-dev , Linux-MM , linux-arch , Linux Kernel Mailing List , Evgenii Stepanov , Nathan Chancellor , Nick Desaulniers , Segher Boessenkool , Vitaly Buka , linux-toolchains Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 1, 2022 at 7:25 AM Alexander Potapenko wrote: > > Under certain circumstances initialization of `unsigned seq` and > `struct inode *inode` passed into step_into() may be skipped. > In particular, if the call to lookup_fast() in walk_component() > returns NULL, and lookup_slow() returns a valid dentry, then the > `seq` and `inode` will remain uninitialized until the call to > step_into() (see [1] for more info). So while I think this needs to be fixed, I think I'd really prefer to make the initialization and/or usage rules stricter or at least clearer. For example, looking around, I think "handle_dotdot()" has the exact same kind of issue, where follow_dotdot[_rcu|() doesn't initialize seq/inode for certain cases, and it's *really* hard to see exactly what the rules are. It turns out that the rules are that seq/inode only get initialized if these routines return a non-NULL and non-error result. Now, that is true for all of these cases - both follow_dotdot*() and lookup_fast(). Possibly others. But the reason follow_dotdot*() doesn't cause the same issue is that the caller actually does the checks that avoid it, and doesn't pass down the uninitialized cases. Now, the other part of the rule is that they only get _used_ for LOOKUP_RCU cases, where they are used to validate the lookup after we've finalized things. Of course, sometimes the "only get used for LOOKUP_RCU" is very very unclear, because even without being an RCU lookup, step_into() will save it into nd->inode/seq. So the values were "used", and initializing them makes them valid, but then *that* copy must not then be used unless RCU was set. Also, sometimes the LOOKUP_RCU check is in the caller, and has actually been cleared, so by the time the actual use comes around, you just have to trust that it was a RCU lookup (ie legitimize_links/root()). So it all seems to work, and this patch then gets rid of one particular odd case, but I think this patch basically hides the compiler warning without really clarifying the code or the rules. Anyway, what I'm building up to here is that I think we should *document* this a bit more. and then make those initializations then be about that documentation. I also get the feeling that "nd->inode/nd->seq" should also be initialized. Right now we have those quite subtle rules about "set vs use", and while a lot of the uses are conditional on LOOKUP_RCU, that makes the code correct, but doesn't solve the "pass uninitialized values as arguments" case. I also think it's very unclear when nd->inode/nd->seq are initialized, and the compiler warning only caught the case where they were *set* (but by arguments that weren't initialized), but didn't necessarily catch the case where they weren't set at all in the first place and then passed around. End result: - I think I'd like path_init() (or set_nameidata) to actually initialize nd->inode and nd->seq unconditionally too. Right now, they get initialized only for that LOOKUP_RCU case. Pretty much exactly the same issue as the one this patch tries to solve, except the compiler didn't notice because it's all indirect through those structure fields and it just didn't track far enough. - I suspect it would be good to initialize them to actual invalid values (rather than NULL/0 - particularly the sequence number) - I look at that follow_dotdot*() caller case, and think "that looks very similar to the lookup_fast() case, but then we have *very* different initialization rules". Al - can you please take a quick look? Linus