Re: [PATCH RESEND v2 00/19] random: Resolve circular include dependency and include <linux/percpu.h>

[Uros Bizjak]

On Mon, Sep 9, 2024 at 5:57 PM Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:

> On Mon, Sep 09, 2024 at 09:53:43AM +0200, Uros Bizjak wrote:
> > a) Substitutes the inclusion of <linux/random.h> with the
> > inclusion of <linux/prandom.h> where needed (patches 1 - 17).
> >
> > b) Removes legacy inclusion of <linux/prandom.h> from
> > <linux/random.h> (patch 18).
> >
> > c) Includes <linux/percpu.h> in <linux/prandom.h> (patch 19).
>
> Thanks for doing this. That seems like a fine initiative to me. (I'm
> also curious about the future percpu changes you've got planned.)

As explained in the cover letter, recent GCCs are able to track
address space of variables in percpu address space from the
declaration to its usage site. There are certain rules regarding casts
of variables and their pointers (when this named address space is not
considered a subspace of the generic address space), so it is possible
to create much more effective checks for cast-from-as type casts than
what sparse can achieve.

Besides GCC, clang can define various named address space via
address_space attribute:

--cut here--
#define __as(N) __attribute__((address_space(N)))

void *foo(void __as(1) *x) { return x; }         // error

void *bar(void __as(1) *x) { return (void *)x; } // fine
--cut here--

When compiling this, the compiler returns:

clang-as.c:3:37: error: returning '__as(1) void *' from a function
with result type 'void *' changes address space of pointer

Although clang currently errors out when __seg_gs and __seg_fs named
address space designators are used, we can explore its named address
spaces functionality to implement percpu checks for all targets. The
percpu address space checks patchset, referred to in the cover letter,
also supports this functionality when per_cpu_qual is defined to
__attribute__((address_space(N))).

Perhaps we can use different address spaces to also handle __user,
__iomem and __rcu address spaces. This way the compiler will be able
to handle address space checks instead of sparse.

> Tree-wise, were you expecting me to take this through random.git? And if
> so, what timeframe did you have in mind? For 6.12 next week (can you
> poke folks for acks in time?), or punt it for 6.13? Or did you have a
> different tree in mind for treewide changes (in which case, I'll send
> you an ack for the [p]random.h changes).

I think that the best approach is to target this patchset for linux
6.13 via random.git tree. I will prepare a v3 after 6.12rc1, so when
committed to random.git, the patchset will be able to spend some time
in linux-next. This way, there will be plenty of time for CI robots to
do additional checks also for some less popular targets (although
individual patches are dead simple, removing these kinds of "legacy"
includes can be tricky), and I will also be able to collect Acked-by:s
in the meantime.

While the patchset is an improvement by itself, its inclusion is not
time sensitive. The follow up percpu named address checking
functionality requires a very recent feature (__typeof_unqual__
keyword), which is only supported in recent compilers (gcc-14 and
clang-20). Besides compiler support, sparse doesn't know about
__typeof_unqual__, resulting in broken type tracing and hundreds of
sparse errors with C=1 due to unknown keyword.

So, I think we are not in a hurry and can take the slow and safe path.

Thanks and best regards,
Uros.