linux-kernel - Re: [RFC PATCH v2 2/4] percpu: Assorted fixes found by strict percpu address space checks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFULd4YOf0Mz-JbR6LEWxM2M=4GTxqC9m-q_QAZJw8Ws16yrTA@mail.gmail.com>
Date: Mon, 12 Aug 2024 22:36:04 +0200
From: Uros Bizjak <ubizjak@...il.com>
To: Nadav Amit <nadav.amit@...il.com>
Cc: "open list:MEMORY MANAGEMENT" <linux-mm@...ck.org>, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Dennis Zhou <dennis@...nel.org>, 
	Tejun Heo <tj@...nel.org>, Christoph Lameter <cl@...ux.com>, Andy Lutomirski <luto@...nel.org>, 
	Ingo Molnar <mingo@...nel.org>, Brian Gerst <brgerst@...il.com>, 
	Denys Vlasenko <dvlasenk@...hat.com>, "H. Peter Anvin" <hpa@...or.com>, 
	Linus Torvalds <torvalds@...ux-foundation.org>, Peter Zijlstra <peterz@...radead.org>, 
	Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, 
	Luc Van Oostenryck <luc.vanoostenryck@...il.com>
Subject: Re: [RFC PATCH v2 2/4] percpu: Assorted fixes found by strict percpu
 address space checks

On Mon, Aug 12, 2024 at 9:09 PM Nadav Amit <nadav.amit@...il.com> wrote:
>
>
> > On 12 Aug 2024, at 14:57, Uros Bizjak <ubizjak@...il.com> wrote:
> > Assorted fixes to prevent defconfig build failures when
> > strict percpu address space checks will be enabled.
> >
> > These show effeciveness of strict percpu address space checks.
>
> [snip]
>
> > --- a/drivers/base/devres.c
> > +++ b/drivers/base/devres.c
> > @@ -1231,6 +1231,6 @@ void devm_free_percpu(struct device *dev, void __percpu *pdata)
> >        * devm_free_pages() does.
> >        */
> >       WARN_ON(devres_release(dev, devm_percpu_release, devm_percpu_match,
> > -                            (__force void *)pdata));
> > +                            (__force void *)(uintptr_t)pdata));
> >
>
> Since this pattern of casting appears multiple times (sometimes slightly
> different), I think it would be best to give a name for this operation
> and put it behind a macro.

The macro would not be flexible enough to also cover const qualified
(const void __percpu *)(const uintptr_t) casts, required in e.g. [1].

[1] https://lore.kernel.org/lkml/20240811161414.56744-1-ubizjak@gmail.com/

Also, some casts are decorated with __force. According to sparse
documentation [2], there is no need to use __force when the
destination type is uintptr_t or unsigned long, but sparse seems to
not be consistent with this exception, leading to spurious warnings
and  fixes like the one in [3].

[2] https://sparse.docs.kernel.org/en/latest/annotations.html#address-space-name
[3] https://lore.kernel.org/lkml/20240402175058.52649-1-ubizjak@gmail.com/

OTOH, in a full allyesconfig this pattern of casting appears maybe a
dozen of times (which is a surprisingly small number).

> This would allow both to audit the cases developers move data between
> address-spaces, and also make them think whether what they do makes
> sense.

Looking through the fixes required for allyesconfig build, the
remaining couple of casts are mostly required for ERR_PTR return with
__percpu return type function, like:

--cut here--
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6c2cb4e4f48d..d82fe78f0658 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -849,7 +849,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,

     cpu_events = alloc_percpu(typeof(*cpu_events));
     if (!cpu_events)
-        return (void __percpu __force *)ERR_PTR(-ENOMEM);
+        return (void __percpu __force *)(uintptr_t)ERR_PTR(-ENOMEM);

     cpus_read_lock();
     for_each_online_cpu(cpu) {
@@ -868,7 +868,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,
         return cpu_events;

     unregister_wide_hw_breakpoint(cpu_events);
-    return (void __percpu __force *)ERR_PTR(err);
+    return (void __percpu __force *)(uintptr_t)ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(register_wide_hw_breakpoint);

--cut here--

While the casts are somehow ugly, I think that the number of different
types (pcpu -> generic and generic -> pcpu casts with possible const
qualifier and still needed __force sparse attribute) and low number of
occurrences currently do not warrant a separate macro.

Uros.