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.
