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

Message

Uros Bizjak Sept. 9, 2024, 7:53 a.m. UTC

Resent due to missing linux-kernel@ mailing list inclusion.

There were several attempts to resolve circular include dependency
after the addition of percpu.h: 1c9df907da83 ("random: fix circular
include dependency on arm64 after addition of percpu.h"), c0842fbc1b18
("random32: move the pseudo-random 32-bit definitions to prandom.h") and
finally d9f29deb7fe8 ("prandom: Remove unused include") that completely
removes the inclusion of <linux/percpu.h>.

Due to legacy reasons, <linux/random.h> includes <linux/prandom.h>, but
with the commit entry remark:

--quote--
A further cleanup step would be to remove this from <linux/random.h>
entirely, and make people who use the prandom infrastructure include
just the new header file.  That's a bit of a churn patch, but grepping
for "prandom_" and "next_pseudo_random32" "struct rnd_state" should
catch most users.

But it turns out that that nice cleanup step is fairly painful, because
a _lot_ of code currently seems to depend on the implicit include of
<linux/random.h>, which can currently come in a lot of ways, including
such fairly core headfers as <linux/net.h>.

So the "nice cleanup" part may or may never happen.
--/quote--

We would like to include <linux/percpu.h> in <linux/prandom.h>.
In [1] we would like to repurpose __percpu tag as a named address space
qualifier, where __percpu macro uses defines from <linux/percpu.h>.

The major roadblock to inclusion of <linux/percpu.h> is the above
mentioned legacy inclusion of <linux/prandom.h> in <linux/random.h> that
causes circular include dependency that prevents <linux/percpu.h>
inclusion.

This patch series is the "nice cleanup" part that:

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).

The whole series was tested by compiling the kernel for x86_64 allconfig
and some popular architectures, namely arm64 defconfig, powerpc defconfig
and loongarch defconfig.

[1] https://lore.kernel.org/lkml/[e-mail]/

Cc: Dave Hansen <[e-mail]>
Cc: Andy Lutomirski <[e-mail]>
Cc: Peter Zijlstra <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: Borislav Petkov <[e-mail]>
Cc: [e-mail]
Cc: "H. Peter Anvin" <[e-mail]>
Cc: Jani Nikula <[e-mail]>
Cc: Joonas Lahtinen <[e-mail]>
Cc: Rodrigo Vivi <[e-mail]>
Cc: Tvrtko Ursulin <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: Hans Verkuil <[e-mail]>
Cc: Mauro Carvalho Chehab <[e-mail]>
Cc: Miquel Raynal <[e-mail]>
Cc: Richard Weinberger <[e-mail]>
Cc: Vignesh Raghavendra <[e-mail]>
Cc: Eric Biggers <[e-mail]>
Cc: "Theodore Y. Ts'o" <[e-mail]>
Cc: Jaegeuk Kim <[e-mail]>
Cc: "Jason A. Donenfeld" <[e-mail]>
Cc: Linus Torvalds <[e-mail]>
Cc: Hannes Reinecke <[e-mail]>
Cc: "James E.J. Bottomley" <[e-mail]>
Cc: "Martin K. Petersen" <[e-mail]>
Cc: Alexei Starovoitov <[e-mail]>
Cc: Daniel Borkmann <[e-mail]>
Cc: John Fastabend <[e-mail]>
Cc: Andrii Nakryiko <[e-mail]>
Cc: Martin KaFai Lau <[e-mail]>
Cc: Eduard Zingerman <[e-mail]>
Cc: Song Liu <[e-mail]>
Cc: Yonghong Song <[e-mail]>
Cc: KP Singh <[e-mail]>
Cc: Stanislav Fomichev <[e-mail]>
Cc: Hao Luo <[e-mail]>
Cc: Jiri Olsa <[e-mail]>
Cc: Andrew Morton <[e-mail]>
Cc: Brendan Higgins <[e-mail]>
Cc: David Gow <[e-mail]>
Cc: Rae Moar <[e-mail]>
Cc: "David S. Miller" <[e-mail]>
Cc: Eric Dumazet <[e-mail]>
Cc: Jakub Kicinski <[e-mail]>
Cc: Paolo Abeni <[e-mail]>
Cc: Jiri Pirko <[e-mail]>
Cc: Petr Mladek <[e-mail]>
Cc: Steven Rostedt <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Rasmus Villemoes <[e-mail]>
Cc: Sergey Senozhatsky <[e-mail]>
Cc: Stephen Hemminger <[e-mail]>
Cc: Jamal Hadi Salim <[e-mail]>
Cc: Cong Wang <[e-mail]>
Cc: Kent Overstreet <[e-mail]>
---
v2: - Reword commit messages to mention the removal of legacy inclusion
    of <linux/prandom.h> from <linux/random.h>
    - Add missing substitution in crypto/testmgr.c
    (reported by kernel test robot)
    - Add Acked-by:.

Uros Bizjak (19):
  x86/kaslr: Include <linux/prandom.h> instead of <linux/random.h>
  crypto: testmgr: Include <linux/prandom.h> instead of <linux/random.h>
  drm/i915/selftests: Include <linux/prandom.h> instead of
    <linux/random.h>
  drm/lib: Include <linux/prandom.h> instead of <linux/random.h>
  media: vivid: Include <linux/prandom.h> in vivid-vid-cap.c
  mtd: tests: Include <linux/prandom.h> instead of <linux/random.h>
  fscrypt: Include <linux/once.h> in fs/crypto/keyring.c
  scsi: libfcoe: Include <linux/prandom.h> instead of <linux/random.h>
  bpf: Include <linux/prandom.h> instead of <linux/random.h>
  lib/interval_tree_test.c: Include <linux/prandom.h> instead of
    <linux/random.h>
  kunit: string-stream-test: Include <linux/prandom.h> instead of
    <linux/random.h>
  random32: Include <linux/prandom.h> instead of <linux/random.h>
  lib/rbtree-test: Include <linux/prandom.h> instead of <linux/random.h>
  bpf/tests: Include <linux/prandom.h> instead of <linux/random.h>
  lib/test_parman: Include <linux/prandom.h> instead of <linux/random.h>
  lib/test_scanf: Include <linux/prandom.h> instead of <linux/random.h>
  netem: Include <linux/prandom.h> in sch_netem.c
  random: Do not include <linux/prandom.h> in <linux/random.h>
  prandom: Include <linux/percpu.h> in <linux/prandom.h>

 arch/x86/mm/kaslr.c                              | 2 +-
 crypto/testmgr.c                                 | 2 +-
 drivers/gpu/drm/i915/selftests/i915_gem.c        | 2 +-
 drivers/gpu/drm/i915/selftests/i915_random.h     | 2 +-
 drivers/gpu/drm/i915/selftests/scatterlist.c     | 2 +-
 drivers/gpu/drm/lib/drm_random.h                 | 2 +-
 drivers/media/test-drivers/vivid/vivid-vid-cap.c | 1 +
 drivers/mtd/tests/oobtest.c                      | 2 +-
 drivers/mtd/tests/pagetest.c                     | 2 +-
 drivers/mtd/tests/subpagetest.c                  | 2 +-
 fs/crypto/keyring.c                              | 1 +
 include/linux/prandom.h                          | 1 +
 include/linux/random.h                           | 7 -------
 include/scsi/libfcoe.h                           | 2 +-
 kernel/bpf/core.c                                | 2 +-
 lib/interval_tree_test.c                         | 2 +-
 lib/kunit/string-stream-test.c                   | 1 +
 lib/random32.c                                   | 2 +-
 lib/rbtree_test.c                                | 2 +-
 lib/test_bpf.c                                   | 2 +-
 lib/test_parman.c                                | 2 +-
 lib/test_scanf.c                                 | 2 +-
 net/sched/sch_netem.c                            | 1 +
 23 files changed, 22 insertions(+), 24 deletions(-)

Comments

Jason A. Donenfeld Sept. 9, 2024, 3:57 p.m. UTC | #1

Hi Uros,

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.)

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).

Regards,
Jason

Uros Bizjak Sept. 9, 2024, 7:30 p.m. UTC | #2

On Mon, Sep 9, 2024 at 5:57 PM Jason A. Donenfeld <[e-mail]> 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.

Jason A. Donenfeld Sept. 10, 2024, 12:37 a.m. UTC | #3

Hi Uros,

On Mon, Sep 09, 2024 at 09:30:06PM +0200, Uros Bizjak wrote:
> 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

Super cool. Looking forward to having it all wired up and the bugs we'll
find with it. 

> 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.

Okay, sure, that sounds good to me. I'll keep my eyes open for v3
in a few weeks then.

Jason