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

[Uros Bizjak]

On Mon, Aug 12, 2024 at 9:09 PM Nadav Amit <nadav.amit@xxxxxxxxx> wrote:
>
>
> > On 12 Aug 2024, at 14:57, Uros Bizjak <ubizjak@xxxxxxxxx> 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@xxxxxxxxx/

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@xxxxxxxxx/

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.