Linux ACPI
 help / color / mirror / Atom feed
From: marc.herbert@linux.intel.com
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Dan Williams <dan.j.williams@intel.com>,
	rafael.j.wysocki@intel.com, linux-cxl@vger.kernel.org,
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Ben Cheatham <Benjamin.Cheatham@amd.com>,
	Danilo Krummrich <dakr@kernel.org>
Cc: Marc Herbert <marc.herbert@linux.intel.com>
Subject: [PATCH] driver core: faux: fix Undefined Behavior in faux_device_destroy()
Date: Fri, 13 Jun 2025 19:15:56 +0000	[thread overview]
Message-ID: <20250613191556.4184103-1-marc.herbert@linux.intel.com> (raw)

From: Marc Herbert <marc.herbert@linux.intel.com>

Fixes undefined behavior that was spotted by Jonathan Cameron in
https://lore.kernel.org/linux-cxl/20250609170509.00003625@huawei.com/

The possible consequences of the undefined behavior fixed here are fairly
well documented across the Internet but to save research time and avoid
doubts, I include a very short and simple demo below. I imagine kernel
compilation flags and various other conditions may not make the
consequences as bad as this example, however those conditions could change
and this type of code is still Undefined Behavior no matter what.
One of the best articles - there are many others:
https://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html

Since commit b5ec6fd286dfa4 ("kbuild: Drop -Wdeclaration-after-statement"),
it's now possible to use C99 declarations; the kernel is not constrained
anymore to group all declarations at the top of a block like single-pass
compilers used to require. This allows combining declarations and
definitions in one place - like literally every other language and project
does - and trivially fix undefined behavior like this.  This also reduces
variable scope and avoids misuse between declaration and definition like
uninitialized reads or writing to the wrong variable by mistake. C99
declarations also allow using a lot more `const` (the default in some
languages) which avoids some misuse after legitimate use.
tl;dr: C99 declarations are not just a "codestyle" or "taste" issue;
they are an important (and not mandatory) feature.

cc --version
  cc (GCC) 15.1.1 20250425

for i in 0 1 2 g; do printf "gcc -O$i: "; gcc -O$i nullptrUB.c &&
   ./a.out; done

gcc -O0: Segmentation fault (core dumped)
gcc -O1: ptr is zero
gcc -O2: ptr is NOT zero!!!
gcc -O3: ptr is NOT zero!!!
gcc -Og: ptr is zero

clang --version
  clang version 19.1.7

clang -O0: Segmentation fault (core dumped)
clang -O1: ptr is NOT zero!!!
clang -O2: ptr is NOT zero!!!
clang -O3: ptr is NOT zero!!!
clang -Og: ptr is NOT zero!!!

int faux_device_destroy(int *ptr)
{
  int i = *ptr;  i++;

  // Because we dereferenced ptr, the compiler knows the pointer cannot
  // be null (even when it is!) and can optimize this away.
  if (!ptr) {
    printf("ptr is zero\n");
    return 0;
  }

  printf("ptr is NOT zero!!!\n");
  return 1;
}

int main()
{
  struct timespec t1, t2;
  clock_gettime(CLOCK_MONOTONIC, &t1);
  clock_gettime(CLOCK_MONOTONIC, &t2);

  // Use the clock to hide zero from the compiler
  int * zeroptr = (int *)(t2.tv_sec - t1.tv_sec);

  return faux_device_destroy(zeroptr);
}

Fixes: 35fa2d88ca94 ("driver core: add a faux bus for use when a simple device/bus is needed")
Signed-off-by: Marc Herbert <marc.herbert@linux.intel.com>
---
 drivers/base/faux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/faux.c b/drivers/base/faux.c
index 9054d346bd7f..94392d397986 100644
--- a/drivers/base/faux.c
+++ b/drivers/base/faux.c
@@ -218,11 +218,11 @@ EXPORT_SYMBOL_GPL(faux_device_create);
  */
 void faux_device_destroy(struct faux_device *faux_dev)
 {
-	struct device *dev = &faux_dev->dev;
-
 	if (!faux_dev)
 		return;
 
+	struct device *dev = &faux_dev->dev;
+
 	device_del(dev);
 
 	/* The final put_device() will clean up the memory we allocated for this device. */
-- 
2.49.0


             reply	other threads:[~2025-06-13 19:16 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-13 19:15 marc.herbert [this message]
2025-06-13 20:20 ` [PATCH] driver core: faux: fix Undefined Behavior in faux_device_destroy() Miguel Ojeda
2025-06-14  0:33 ` Greg KH
2025-06-14 10:50   ` Miguel Ojeda
2025-06-14 11:53     ` Greg KH
2025-06-14 14:53       ` Marc Herbert
2025-06-16  3:35         ` Greg KH
2025-06-16 14:02           ` Alice Ryhl
2025-06-18 23:43           ` Marc Herbert
2025-06-19  0:23             ` Dan Williams
2025-06-19  2:35               ` Dan Carpenter
2025-06-19  3:33               ` Marc Herbert
2025-06-19  4:02                 ` Dan Carpenter
2025-06-26  0:55                 ` Kent Overstreet
2025-06-30 23:24                   ` Marc Herbert
2025-06-25 15:20     ` Dan Carpenter
2025-06-25 22:30       ` Marc Herbert
2025-06-25 23:18         ` Dan Carpenter
2025-06-25 15:21 ` Dan Carpenter

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=20250613191556.4184103-1-marc.herbert@linux.intel.com \
    --to=marc.herbert@linux.intel.com \
    --cc=Benjamin.Cheatham@amd.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=dakr@kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=rafael@kernel.org \
    --cc=sudeep.holla@arm.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox