| Message ID | 20240909075641.258968-1-ubizjak@gmail.com |
|---|---|
| Headers | show |
| Series | random: Resolve circular include dependency and include <linux/percpu.h> | expand |
On Mon, Sep 9, 2024 at 5:57 PM Jason A. Donenfeld <Jason@zx2c4.com> 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.
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
On Mon, Sep 09, 2024 at 09:53:50AM +0200, Uros Bizjak wrote: > Include <linux/once.h> header to allow the removal of legacy > inclusion of <linux/prandom.h> from <linux/random.h>. > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > Cc: Eric Biggers <ebiggers@kernel.org> > Cc: "Theodore Y. Ts'o" <tytso@mit.edu> > Cc: Jaegeuk Kim <jaegeuk@kernel.org> > --- > v2: Include <linux/once.h> instead of <linux/prandom.h> > --- > fs/crypto/keyring.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c > index 6681a71625f0..82fcc5683649 100644 > --- a/fs/crypto/keyring.c > +++ b/fs/crypto/keyring.c > @@ -22,6 +22,7 @@ > #include <crypto/skcipher.h> > #include <linux/key-type.h> > #include <linux/random.h> > +#include <linux/once.h> > #include <linux/seq_file.h> > > #include "fscrypt_private.h" Acked-by: Eric Biggers <ebiggers@google.com> - Eric
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/20240812115945.484051-4-ubizjak@gmail.com/ Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: Andy Lutomirski <luto@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: x86@kernel.org Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Tvrtko Ursulin <tursulin@ursulin.net> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Hans Verkuil <hverkuil@xs4all.nl> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Miquel Raynal <miquel.raynal@bootlin.com> Cc: Richard Weinberger <richard@nod.at> Cc: Vignesh Raghavendra <vigneshr@ti.com> Cc: Eric Biggers <ebiggers@kernel.org> Cc: "Theodore Y. Ts'o" <tytso@mit.edu> Cc: Jaegeuk Kim <jaegeuk@kernel.org> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Hannes Reinecke <hare@suse.de> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: John Fastabend <john.fastabend@gmail.com> Cc: Andrii Nakryiko <andrii@kernel.org> Cc: Martin KaFai Lau <martin.lau@linux.dev> Cc: Eduard Zingerman <eddyz87@gmail.com> Cc: Song Liu <song@kernel.org> Cc: Yonghong Song <yonghong.song@linux.dev> Cc: KP Singh <kpsingh@kernel.org> Cc: Stanislav Fomichev <sdf@fomichev.me> Cc: Hao Luo <haoluo@google.com> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Brendan Higgins <brendan.higgins@linux.dev> Cc: David Gow <davidgow@google.com> Cc: Rae Moar <rmoar@google.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Jiri Pirko <jiri@resnulli.us> Cc: Petr Mladek <pmladek@suse.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> Cc: Stephen Hemminger <stephen@networkplumber.org> Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Kent Overstreet <kent.overstreet@linux.dev> --- 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(-)