All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Potapenko <glider@google.com>
To: glider@google.com
Cc: Evgenii Stepanov <eugenis@google.com>,
	Kees Cook <keescook@chromium.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Marco Elver <elver@google.com>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Vitaly Buka <vitalybuka@google.com>,
	linux-kernel@vger.kernel.org, linux-toolchains@vger.kernel.org
Subject: [PATCH] [RFC] Initialization of unused function parameters
Date: Tue, 14 Jun 2022 16:48:53 +0200	[thread overview]
Message-ID: <20220614144853.3693273-1-glider@google.com> (raw)

Consider the patch below, where 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).

Right now step_into() correctly handles this and does not use the
uninitialized values, but explicitly initializing them will guarantee
their sanity in the case of future changes to step_into().

The bigger question I want to raise here is whether we want to
discourage passing uninitialized variables to functions in the kernel
altogether. While this is not required by the C standard (see the whole
discussion at [2]), KMSAN can help with detecting cases where uninits
are passed to functions (there is a more aggressive
-fsanitize-memory-param-retval compiler option for that). This is
somewhat similar to -Wmaybe-uninitialized, but works dynamically and has
no false positives.

Would initializing function parameters be a net benefit to the kernel?

[1] https://github.com/ClangBuiltLinux/linux/issues/1648#issuecomment-1146608063
[2] https://github.com/ClangBuiltLinux/linux/issues/1648

Cc: Evgenii Stepanov <eugenis@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Marco Elver <elver@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vitaly Buka <vitalybuka@google.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-toolchains@vger.kernel.org
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 fs/namei.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 509657fdf4f56..dde370338f5d6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2001,8 +2001,8 @@ static const char *handle_dots(struct nameidata *nd, int type)
 static const char *walk_component(struct nameidata *nd, int flags)
 {
 	struct dentry *dentry;
-	struct inode *inode;
-	unsigned seq;
+	struct inode *inode = NULL;
+	unsigned seq = 0;
 	/*
 	 * "." and ".." are special - ".." especially so because it has
 	 * to be able to know about the current root directory and
@@ -3349,8 +3349,8 @@ static const char *open_last_lookups(struct nameidata *nd,
 	struct dentry *dir = nd->path.dentry;
 	int open_flag = op->open_flag;
 	bool got_write = false;
-	unsigned seq;
-	struct inode *inode;
+	unsigned seq = 0;
+	struct inode *inode = NULL;
 	struct dentry *dentry;
 	const char *res;
 
-- 
2.36.1.476.g0c4daa206d-goog


             reply	other threads:[~2022-06-14 14:49 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-14 14:48 Alexander Potapenko [this message]
2022-06-14 16:48 ` [PATCH] [RFC] Initialization of unused function parameters Linus Torvalds
2022-06-14 17:11   ` Nick Desaulniers
2022-06-14 17:24     ` Linus Torvalds
2022-06-14 18:08       ` Nick Desaulniers
2022-06-14 22:27         ` Peter Zijlstra
2022-06-14 18:07   ` Alexander Potapenko
2022-06-14 18:30     ` Linus Torvalds
2022-06-14 20:19       ` Alexander Potapenko
2022-06-14 20:43         ` Linus Torvalds
2022-06-14 21:40         ` Segher Boessenkool
2022-06-14 22:08           ` Evgenii Stepanov
2022-06-15  8:30           ` Alexander Potapenko
2022-06-15 16:46             ` Segher Boessenkool

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220614144853.3693273-1-glider@google.com \
    --to=glider@google.com \
    --cc=elver@google.com \
    --cc=eugenis@google.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-toolchains@vger.kernel.org \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=vitalybuka@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.