Re: [PATCH v4 2/6] treewide: use prandom_u32_max() when possible
On Fri, Oct 07, 2022 at 03:47:44PM -0700, Kees Cook wrote:
> On Fri, Oct 07, 2022 at 12:01:03PM -0600, Jason A. Donenfeld wrote:
> > Rather than incurring a division or requesting too many random bytes for
> > the given range, use the prandom_u32_max() function, which only takes
> > the minimum required bytes from the RNG and avoids divisions.
>
> I actually meant splitting the by-hand stuff by subsystem, but nearly
> all of these can be done mechanically too, so it shouldn't be bad. Notes
> below...
Oh, cool, more coccinelle. You're basically giving me a class on these
recipes. Much appreciated.
> > [...]
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 92bcc1768f0b..87203429f802 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -595,7 +595,7 @@ unsigned long __get_wchan(struct task_struct *p)
> > unsigned long arch_align_stack(unsigned long sp)
> > {
> > if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
> > - sp -= get_random_int() & ~PAGE_MASK;
> > + sp -= prandom_u32_max(PAGE_SIZE);
> > return sp & ~0xf;
> > }
> >
>
> @mask@
> expression MASK;
> @@
>
> - (get_random_int() & ~(MASK))
> + prandom_u32_max(MASK)
Not quite! PAGE_MASK != PAGE_SIZE. In this case, things get a litle
more complicated where you can do:
get_random_int() & MASK == prandom_u32_max(MASK + 1)
*only if all the top bits of MASK are set* That is, if MASK one less
than a power of two. Or if MASK & (MASK + 1) == 0.
(If those top bits aren't set, you can technically do
prandom_u32_max(MASK >> n + 1) << n. That'd be a nice thing to work out.
But yeesh, maybe a bit much for the time being and probably a bit beyond
coccinelle.)
This case here, though, is a bit more special, where we can just rely on
an obvious given kernel identity. Namely, PAGE_MASK == ~(PAGE_SIZE - 1).
So ~PAGE_MASK == PAGE_SIZE - 1.
So get_random_int() & ~PAGE_MASK == prandom_u32_max(PAGE_SIZE - 1 + 1).
So get_random_int() & ~PAGE_MASK == prandom_u32_max(PAGE_SIZE).
And most importantly, this makes the code more readable, since everybody
knows what bounding by PAGE_SIZE means, where as what on earth is
happening with the &~PAGE_MASK thing. So it's a good change. I'll try to
teach coccinelle about that special case.
> > diff --git a/arch/loongarch/kernel/vdso.c b/arch/loongarch/kernel/vdso.c
> > index f32c38abd791..8c9826062652 100644
> > --- a/arch/loongarch/kernel/vdso.c
> > +++ b/arch/loongarch/kernel/vdso.c
> > @@ -78,7 +78,7 @@ static unsigned long vdso_base(void)
> > unsigned long base = STACK_TOP;
> >
> > if (current->flags & PF_RANDOMIZE) {
> > - base += get_random_int() & (VDSO_RANDOMIZE_SIZE - 1);
> > + base += prandom_u32_max(VDSO_RANDOMIZE_SIZE);
> > base = PAGE_ALIGN(base);
> > }
> >
>
> @minus_one@
> expression FULL;
> @@
>
> - (get_random_int() & ((FULL) - 1)
> + prandom_u32_max(FULL)
Ahh, well, okay, this is the example I mentioned above. Only works if
FULL is saturated. Any clever way to get coccinelle to prove that? Can
it look at the value of constants?
>
> > diff --git a/arch/parisc/kernel/vdso.c b/arch/parisc/kernel/vdso.c
> > index 63dc44c4c246..47e5960a2f96 100644
> > --- a/arch/parisc/kernel/vdso.c
> > +++ b/arch/parisc/kernel/vdso.c
> > @@ -75,7 +75,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm,
> >
> > map_base = mm->mmap_base;
> > if (current->flags & PF_RANDOMIZE)
> > - map_base -= (get_random_int() & 0x1f) * PAGE_SIZE;
> > + map_base -= prandom_u32_max(0x20) * PAGE_SIZE;
> >
> > vdso_text_start = get_unmapped_area(NULL, map_base, vdso_text_len, 0,
> > 0);
> >
>
> These are more fun, but Coccinelle can still do them with a little
> Pythonic help:
>
> // Find a potential literal
> @literal_mask@
> expression LITERAL;
> identifier randfunc =~ "get_random_int|prandom_u32|get_random_u32";
> position p;
> @@
>
> (randfunc()@p & (LITERAL))
>
> // Add one to the literal.
> @script:python add_one@
> literal << literal_mask.LITERAL;
> RESULT;
> @@
>
> if literal.startswith('0x'):
> value = int(literal, 16) + 1
> coccinelle.RESULT = cocci.make_expr("0x%x" % (value))
> elif literal[0] in '123456789':
> value = int(literal, 10) + 1
> coccinelle.RESULT = cocci.make_expr("%d" % (value))
> else:
> print("I don't know how to handle: %s" % (literal))
>
> // Replace the literal mask with the calculated result.
> @plus_one@
> expression literal_mask.LITERAL;
> position literal_mask.p;
> expression add_one.RESULT;
> identifier FUNC;
> @@
>
> - (FUNC()@p & (LITERAL))
> + prandom_u32_max(RESULT)
Oh that's pretty cool. I can do the saturation check in python, since
`value` holds the parsed result. Neat.
> > diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
> > index 998dd2ac8008..f4944c4dee60 100644
> > --- a/fs/ext2/ialloc.c
> > +++
Re: [PATCH v4 4/6] treewide: use get_random_u32() when possible
On Fri, Oct 07, 2022 at 10:34:47PM +0200, Rolf Eike Beer wrote:
> > diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
> > index 7c37e09c92da..18c4f0e3e906 100644
> > --- a/arch/parisc/kernel/process.c
> > +++ b/arch/parisc/kernel/process.c
> > @@ -288,7 +288,7 @@ __get_wchan(struct task_struct *p)
> >
> > static inline unsigned long brk_rnd(void)
> > {
> > - return (get_random_int() & BRK_RND_MASK) << PAGE_SHIFT;
> > + return (get_random_u32() & BRK_RND_MASK) << PAGE_SHIFT;
> > }
>
> Can't this be
>
> prandom_u32_max(BRK_RND_MASK + 1) << PAGE_SHIFT
>
> ? More similar code with other masks follows below.
I guess it can, because BRK_RND_MASK happens to have all its lower bits
set. But as a "_MASK" maybe this isn't a given, and I don't want to
change intended semantics in this patchset. It's also not more
efficient, because BRK_RND_MASK is actually an expression:
#define BRK_RND_MASK(is_32bit_task() ? 0x07ffUL : 0x3UL)
So at compile-time, the compiler can't prove that it's <= U16_MAX, since
it isn't always the case, so it'll use get_random_u32() anyway.
[Side note: maybe that compile-time check should become a runtime check,
but I'll need to do some benchmarking before changing that and
introducing two added branches to every non-constant invocation, so for
now it's a compile-time check. Fortunately the vast majority of uses
are done on inputs the compiler can prove something about.]
>
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 329ff75b80b9..7bd1861ddbdf
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -137,12 +137,12 @@ static u64 random_offset(u64 start, u64 end, u64 len,
> > u64 align) range = round_down(end - len, align) - round_up(start, align);
> > if (range) {
> > if (sizeof(unsigned long) == sizeof(u64)) {
> > - addr = get_random_long();
> > + addr = get_random_u64();
> > } else {
> > - addr = get_random_int();
> > + addr = get_random_u32();
> > if (range > U32_MAX) {
> > addr <<= 32;
> > - addr |= get_random_int();
> > + addr |= get_random_u32();
> > }
> > }
> > div64_u64_rem(addr, range, );
>
> How about
>
> if (sizeof(unsigned long) == sizeof(u64) || range >
> U32_MAX)
> addr = get_random_u64();
> else
> addr = get_random_u32();
>
Yes, maybe, probably, indeed... But I don't want to go wild and start
fixing all the weird algorithms everywhere. My goal is to only make
changes that are "obviously right". But maybe after this lands this is
something that you or I can submit to the i915 people as an
optimization.
> > diff --git a/drivers/infiniband/hw/cxgb4/cm.c
> > b/drivers/infiniband/hw/cxgb4/cm.c index 14392c942f49..499a425a3379 100644
> > --- a/drivers/infiniband/hw/cxgb4/cm.c
> > +++ b/drivers/infiniband/hw/cxgb4/cm.c
> > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
> >>com.remote_addr;
> > int ret;
> > enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> > - u32 isn = (prandom_u32() & ~7UL) - 1;
> > + u32 isn = (get_random_u32() & ~7UL) - 1;
> > struct net_device *netdev;
> > u64 params;
> >
> > @@ -2469,7 +2469,7 @@ static int accept_cr(struct c4iw_ep *ep, struct
> > sk_buff *skb, }
> >
> > if (!is_t4(adapter_type)) {
> > - u32 isn = (prandom_u32() & ~7UL) - 1;
> > + u32 isn = (get_random_u32() & ~7UL) - 1;
>
> u32 isn = get_random_u32() | 0x7;
Again, maybe so, but same rationale as above.
> > static void ns_do_bit_flips(struct nandsim *ns, int num)
> > {
> > - if (bitflips && prandom_u32() < (1 << 22)) {
> > + if (bitflips && get_random_u32() < (1 << 22)) {
>
> Doing "get_random_u16() < (1 << 6)" should have the same probability with
> only
> 2 bytes of random, no?
That's very clever. (1<<22)/(1<<32) == (1<<6)/(1<<16). But also, same
rationale as above for not doing that.
Anyway, I realize this is probably disappointing to read. But also, we
can come back to those optimization cases later pretty easily.
Jason
Re: [PATCH v4 2/6] treewide: use prandom_u32_max() when possible
On Fri, Oct 07, 2022 at 02:17:22PM -0700, Darrick J. Wong wrote:
> On Fri, Oct 07, 2022 at 12:01:03PM -0600, Jason A. Donenfeld wrote:
> > Rather than incurring a division or requesting too many random bytes for
> > the given range, use the prandom_u32_max() function, which only takes
> > the minimum required bytes from the RNG and avoids divisions.
> >
> > Reviewed-by: Kees Cook
> > Reviewed-by: KP Singh
> > Reviewed-by: Christoph Böhmwalder # for
> > drbd
> > Reviewed-by: Jan Kara # for ext2, ext4, and sbitmap
> > Signed-off-by: Jason A. Donenfeld
> > ---
>
>
>
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index e2bdf089c0a3..6261599bb389 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -1520,7 +1520,7 @@ xfs_alloc_ag_vextent_lastblock(
> >
> > #ifdef DEBUG
> > /* Randomly don't execute the first algorithm. */
> > - if (prandom_u32() & 1)
> > + if (prandom_u32_max(2))
>
> I wonder if these usecases (picking 0 or 1 randomly) ought to have a
> trivial wrapper to make it more obvious that we want boolean semantics:
>
> static inline bool prandom_bool(void)
> {
> return prandom_u32_max(2);
> }
>
> if (prandom_bool())
> use_crazy_algorithm(...);
>
Yea, I've had a lot of similar ideas there. Part of doing this (initial)
patchset is to get an intuitive sense of what's actually used and how
often. On my list for investigation are a get_random_u32_max() to return
uniform numbers by rejection sampling (prandom_u32_max() doesn't do
that uniformly) and adding a function for booleans or bits < 8. Possible
ideas for the latter include:
bool get_random_bool(void);
bool get_random_bool(unsigned int probability);
bool get_random_bits(u8 bits_less_than_eight);
With the core of all of those involving the same batching as the current
get_random_u{8,16,32,64}() functions, but also buffering the latest byte
and managing how many bits are left in it that haven't been shifted out
yet.
So API-wise, there are a few ways to go, so hopefully this series will
start to give a good picture of what's needed.
One thing I've noticed is that most of the prandom_u32_max(2)
invocations are in debug or test code, so that doesn't need to be
optimized. But kfence does that too in its hot path, so a
get_random_bool() function there would in theory lead to an 8x speed-up.
But I guess I just have to try some things and see.
Anyway, that is a long way to say, I share you curiosity on the matter
and I'm looking into it.
Jason
[PATCH] soc: fsl: qe: Add check for ioremap
As ioremap can return NULL pointer, it should
be better to check the return value return error
if fails.
Moreover, the return value of qe_reset should be
checked by cascade.
Fixes: 68f047e3d62e ("fsl/qe: add rx_sync and tx_sync for TDM mode")
Signed-off-by: Jiasheng Jiang
---
drivers/soc/fsl/qe/qe.c | 23 ++-
include/soc/fsl/qe/qe.h | 4 ++--
2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index b3c226eb5292..88e335e8eef7 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -83,10 +83,13 @@ static phys_addr_t get_qe_base(void)
return qebase;
}
-void qe_reset(void)
+int qe_reset(void)
{
- if (qe_immr == NULL)
+ if (qe_immr == NULL) {
qe_immr = ioremap(get_qe_base(), QE_IMMAP_SIZE);
+ if (qe_immr == NULL)
+ return -ENOMEM;
+ }
qe_snums_init();
@@ -98,6 +101,8 @@ void qe_reset(void)
if (qe_sdma_init())
panic("sdma init failed!");
+
+ return 0;
}
int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol, u32 cmd_input)
@@ -640,11 +645,14 @@ EXPORT_SYMBOL(qe_get_num_of_snums);
static int __init qe_init(void)
{
struct device_node *np;
+ int ret;
np = of_find_compatible_node(NULL, NULL, "fsl,qe");
if (!np)
return -ENODEV;
- qe_reset();
+ ret = qe_reset();
+ if (ret)
+ return ret;
of_node_put(np);
return 0;
}
@@ -653,8 +661,13 @@ subsys_initcall(qe_init);
#if defined(CONFIG_SUSPEND) && defined(CONFIG_PPC_85xx)
static int qe_resume(struct platform_device *ofdev)
{
- if (!qe_alive_during_sleep())
- qe_reset();
+ int ret;
+
+ if (!qe_alive_during_sleep()) {
+ ret = qe_reset();
+ if (ret)
+ return ret;
+ }
return 0;
}
diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index b02e9fe69146..71129b8a5807 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h
@@ -84,9 +84,9 @@ extern spinlock_t cmxgcr_lock;
/* Export QE common operations */
#ifdef CONFIG_QUICC_ENGINE
-extern void qe_reset(void);
+extern int qe_reset(void);
#else
-static inline void qe_reset(void) {}
+static inline int qe_reset(void) {}
#endif
int cpm_muram_init(void);
--
2.25.1
Re: [PATCH v4 2/6] treewide: use prandom_u32_max() when possible
On Fri, Oct 07, 2022 at 12:01:03PM -0600, Jason A. Donenfeld wrote:
> Rather than incurring a division or requesting too many random bytes for
> the given range, use the prandom_u32_max() function, which only takes
> the minimum required bytes from the RNG and avoids divisions.
I actually meant splitting the by-hand stuff by subsystem, but nearly
all of these can be done mechanically too, so it shouldn't be bad. Notes
below...
>
> [...]
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 92bcc1768f0b..87203429f802 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -595,7 +595,7 @@ unsigned long __get_wchan(struct task_struct *p)
> unsigned long arch_align_stack(unsigned long sp)
> {
> if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
> - sp -= get_random_int() & ~PAGE_MASK;
> + sp -= prandom_u32_max(PAGE_SIZE);
> return sp & ~0xf;
> }
>
@mask@
expression MASK;
@@
- (get_random_int() & ~(MASK))
+ prandom_u32_max(MASK)
> diff --git a/arch/loongarch/kernel/vdso.c b/arch/loongarch/kernel/vdso.c
> index f32c38abd791..8c9826062652 100644
> --- a/arch/loongarch/kernel/vdso.c
> +++ b/arch/loongarch/kernel/vdso.c
> @@ -78,7 +78,7 @@ static unsigned long vdso_base(void)
> unsigned long base = STACK_TOP;
>
> if (current->flags & PF_RANDOMIZE) {
> - base += get_random_int() & (VDSO_RANDOMIZE_SIZE - 1);
> + base += prandom_u32_max(VDSO_RANDOMIZE_SIZE);
> base = PAGE_ALIGN(base);
> }
>
@minus_one@
expression FULL;
@@
- (get_random_int() & ((FULL) - 1)
+ prandom_u32_max(FULL)
> diff --git a/arch/parisc/kernel/vdso.c b/arch/parisc/kernel/vdso.c
> index 63dc44c4c246..47e5960a2f96 100644
> --- a/arch/parisc/kernel/vdso.c
> +++ b/arch/parisc/kernel/vdso.c
> @@ -75,7 +75,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm,
>
> map_base = mm->mmap_base;
> if (current->flags & PF_RANDOMIZE)
> - map_base -= (get_random_int() & 0x1f) * PAGE_SIZE;
> + map_base -= prandom_u32_max(0x20) * PAGE_SIZE;
>
> vdso_text_start = get_unmapped_area(NULL, map_base, vdso_text_len, 0,
> 0);
>
These are more fun, but Coccinelle can still do them with a little
Pythonic help:
// Find a potential literal
@literal_mask@
expression LITERAL;
identifier randfunc =~ "get_random_int|prandom_u32|get_random_u32";
position p;
@@
(randfunc()@p & (LITERAL))
// Add one to the literal.
@script:python add_one@
literal << literal_mask.LITERAL;
RESULT;
@@
if literal.startswith('0x'):
value = int(literal, 16) + 1
coccinelle.RESULT = cocci.make_expr("0x%x" % (value))
elif literal[0] in '123456789':
value = int(literal, 10) + 1
coccinelle.RESULT = cocci.make_expr("%d" % (value))
else:
print("I don't know how to handle: %s" % (literal))
// Replace the literal mask with the calculated result.
@plus_one@
expression literal_mask.LITERAL;
position literal_mask.p;
expression add_one.RESULT;
identifier FUNC;
@@
- (FUNC()@p & (LITERAL))
+ prandom_u32_max(RESULT)
> diff --git a/drivers/mtd/tests/stresstest.c b/drivers/mtd/tests/stresstest.c
> index cb29c8c1b370..d2faaca7f19d 100644
> --- a/drivers/mtd/tests/stresstest.c
> +++ b/drivers/mtd/tests/stresstest.c
> @@ -45,9 +45,8 @@ static int rand_eb(void)
> unsigned int eb;
>
> again:
> - eb = prandom_u32();
> /* Read or write up 2 eraseblocks at a time - hence 'ebcnt - 1' */
> - eb %= (ebcnt - 1);
> + eb = prandom_u32_max(ebcnt - 1);
> if (bbt[eb])
> goto again;
> return eb;
This can also be done mechanically:
@multi_line@
identifier randfunc =~ "get_random_int|prandom_u32|get_random_u32";
identifier RAND;
expression E;
@@
- RAND = randfunc();
... when != RAND
- RAND %= (E);
+ RAND = prandom_u32_max(E);
@collapse_ret@
type TYPE;
identifier VAR;
expression E;
@@
{
- TYPE VAR;
- VAR = (E);
- return VAR;
+ return E;
}
@drop_var@
type TYPE;
identifier VAR;
@@
{
- TYPE VAR;
... when != VAR
}
> diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
> index 998dd2ac8008..f4944c4dee60 100644
> --- a/fs/ext2/ialloc.c
> +++ b/fs/ext2/ialloc.c
> @@ -277,8 +277,7 @@ static int find_group_orlov(struct super_block *sb,
> struct inode *parent)
> int best_ndir = inodes_per_group;
> int best_group = -1;
>
> - group = prandom_u32();
> - parent_group = (unsigned)group % ngroups;
> + parent_group = prandom_u32_max(ngroups);
> for (i = 0; i < ngroups; i++) {
> group = (parent_group + i) % ngroups;
> desc = ext2_get_group_desc (sb, group, NULL);
Okay, that one is too much for me -- checking that group is never used
after the assignment removal is likely possible, but beyond my cocci
Re: [PATCH v4 4/6] treewide: use get_random_u32() when possible
On Fri, Oct 07, 2022 at 12:01:05PM -0600, Jason A. Donenfeld wrote: > The prandom_u32() function has been a deprecated inline wrapper around > get_random_u32() for several releases now, and compiles down to the > exact same code. Replace the deprecated wrapper with a direct call to > the real function. The same also applies to get_random_int(), which is > just a wrapper around get_random_u32(). > > Reviewed-by: Kees Cook > Acked-by: Toke Høiland-Jørgensen # for sch_cake > Acked-by: Chuck Lever # for nfsd > Reviewed-by: Jan Kara # for ext4 > Acked-by: Mika Westerberg # for thunderbolt > Signed-off-by: Jason A. Donenfeld For the XFS parts, Acked-by: Darrick J. Wong --D > --- > Documentation/networking/filter.rst| 2 +- > arch/parisc/kernel/process.c | 2 +- > arch/parisc/kernel/sys_parisc.c| 4 ++-- > arch/s390/mm/mmap.c| 2 +- > arch/x86/kernel/cpu/amd.c | 2 +- > drivers/gpu/drm/i915/i915_gem_gtt.c| 6 +++--- > drivers/gpu/drm/i915/selftests/i915_selftest.c | 2 +- > drivers/gpu/drm/selftests/test-drm_buddy.c | 2 +- > drivers/gpu/drm/selftests/test-drm_mm.c| 2 +- > drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- > drivers/infiniband/hw/hfi1/tid_rdma.c | 2 +- > drivers/infiniband/hw/mlx4/mad.c | 2 +- > drivers/infiniband/ulp/ipoib/ipoib_cm.c| 2 +- > drivers/md/raid5-cache.c | 2 +- > .../media/test-drivers/vivid/vivid-touch-cap.c | 4 ++-- > drivers/misc/habanalabs/gaudi2/gaudi2.c| 2 +- > drivers/mtd/nand/raw/nandsim.c | 2 +- > drivers/net/bonding/bond_main.c| 2 +- > drivers/net/ethernet/broadcom/cnic.c | 2 +- > .../chelsio/inline_crypto/chtls/chtls_cm.c | 2 +- > drivers/net/ethernet/rocker/rocker_main.c | 6 +++--- > .../wireless/broadcom/brcm80211/brcmfmac/pno.c | 2 +- > .../net/wireless/marvell/mwifiex/cfg80211.c| 4 ++-- > .../net/wireless/microchip/wilc1000/cfg80211.c | 2 +- > .../net/wireless/quantenna/qtnfmac/cfg80211.c | 2 +- > drivers/net/wireless/ti/wlcore/main.c | 2 +- > drivers/nvme/common/auth.c | 2 +- > drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 4 ++-- > drivers/target/iscsi/cxgbit/cxgbit_cm.c| 2 +- > drivers/thunderbolt/xdomain.c | 2 +- > drivers/video/fbdev/uvesafb.c | 2 +- > fs/exfat/inode.c | 2 +- > fs/ext4/ialloc.c | 2 +- > fs/ext4/ioctl.c| 4 ++-- > fs/ext4/mmp.c | 2 +- > fs/f2fs/namei.c| 2 +- > fs/fat/inode.c | 2 +- > fs/nfsd/nfs4state.c| 4 ++-- > fs/ntfs3/fslog.c | 6 +++--- > fs/ubifs/journal.c | 2 +- > fs/xfs/libxfs/xfs_ialloc.c | 2 +- > fs/xfs/xfs_icache.c| 2 +- > fs/xfs/xfs_log.c | 2 +- > include/net/netfilter/nf_queue.h | 2 +- > include/net/red.h | 2 +- > include/net/sock.h | 2 +- > kernel/bpf/bloom_filter.c | 2 +- > kernel/bpf/core.c | 2 +- > kernel/bpf/hashtab.c | 2 +- > kernel/bpf/verifier.c | 2 +- > kernel/kcsan/selftest.c| 2 +- > lib/random32.c | 2 +- > lib/reed_solomon/test_rslib.c | 6 +++--- > lib/test_fprobe.c | 2 +- > lib/test_kprobes.c | 2 +- > lib/test_min_heap.c| 6 +++--- > lib/test_rhashtable.c | 6 +++--- > mm/shmem.c | 2 +- > mm/slab.c | 2 +- > net/802/garp.c | 2 +- > net/802/mrp.c | 2 +- > net/core/pktgen.c | 4 ++-- > net/ipv4/route.c | 2 +- > net/ipv4/tcp_cdg.c | 2 +- > net/ipv4/udp.c | 2 +- > net/ipv6/ip6_flowlabel.c | 2 +- > net/ipv6/output_core.c | 2 +- > net/netfilter/ipvs/ip_vs_conn.c| 2 +- > net/netfilter/xt_statistic.c | 2 +- > net/openvswitch/actions.c | 2 +- > net/rds/bind.c | 2 +- > net/sched/sch_cake.c | 2 +- > net/sched/sch_netem.c | 18 +- >
Re: [PATCH v4 2/6] treewide: use prandom_u32_max() when possible
On Fri, Oct 07, 2022 at 12:01:03PM -0600, Jason A. Donenfeld wrote:
> Rather than incurring a division or requesting too many random bytes for
> the given range, use the prandom_u32_max() function, which only takes
> the minimum required bytes from the RNG and avoids divisions.
>
> Reviewed-by: Kees Cook
> Reviewed-by: KP Singh
> Reviewed-by: Christoph Böhmwalder # for
> drbd
> Reviewed-by: Jan Kara # for ext2, ext4, and sbitmap
> Signed-off-by: Jason A. Donenfeld
> ---
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index e2bdf089c0a3..6261599bb389 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -1520,7 +1520,7 @@ xfs_alloc_ag_vextent_lastblock(
>
> #ifdef DEBUG
> /* Randomly don't execute the first algorithm. */
> - if (prandom_u32() & 1)
> + if (prandom_u32_max(2))
I wonder if these usecases (picking 0 or 1 randomly) ought to have a
trivial wrapper to make it more obvious that we want boolean semantics:
static inline bool prandom_bool(void)
{
return prandom_u32_max(2);
}
if (prandom_bool())
use_crazy_algorithm(...);
But this translation change looks correct to me, so for the XFS parts:
Acked-by: Darrick J. Wong
--D
> return 0;
> #endif
>
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 6cdfd64bc56b..7838b31126e2 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -636,7 +636,7 @@ xfs_ialloc_ag_alloc(
> /* randomly do sparse inode allocations */
> if (xfs_has_sparseinodes(tp->t_mountp) &&
> igeo->ialloc_min_blks < igeo->ialloc_blks)
> - do_sparse = prandom_u32() & 1;
> + do_sparse = prandom_u32_max(2);
> #endif
>
> /*
> diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
> index 4b71a96190a8..66ee9b4b7925 100644
> --- a/include/linux/nodemask.h
> +++ b/include/linux/nodemask.h
> @@ -509,7 +509,7 @@ static inline int node_random(const nodemask_t *maskp)
> w = nodes_weight(*maskp);
> if (w)
> bit = bitmap_ord_to_pos(maskp->bits,
> - get_random_int() % w, MAX_NUMNODES);
> + prandom_u32_max(w), MAX_NUMNODES);
> return bit;
> #else
> return 0;
> diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
> index e6a31c927b06..a72a2c16066e 100644
> --- a/lib/cmdline_kunit.c
> +++ b/lib/cmdline_kunit.c
> @@ -76,7 +76,7 @@ static void cmdline_test_lead_int(struct kunit *test)
> int rc = cmdline_test_values[i];
> int offset;
>
> - sprintf(in, "%u%s", prandom_u32_max(256), str);
> + sprintf(in, "%u%s", get_random_int() % 256, str);
> /* Only first '-' after the number will advance the pointer */
> offset = strlen(in) - strlen(str) + !!(rc == 2);
> cmdline_do_one_test(test, in, rc, offset);
> @@ -94,7 +94,7 @@ static void cmdline_test_tail_int(struct kunit *test)
> int rc = strcmp(str, "") ? (strcmp(str, "-") ? 0 : 1) : 1;
> int offset;
>
> - sprintf(in, "%s%u", str, prandom_u32_max(256));
> + sprintf(in, "%s%u", str, get_random_int() % 256);
> /*
>* Only first and leading '-' not followed by integer
>* will advance the pointer.
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 5f0e71ab292c..a0b2dbfcfa23 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -694,7 +694,7 @@ static void kobject_release(struct kref *kref)
> {
> struct kobject *kobj = container_of(kref, struct kobject, kref);
> #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> - unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
> + unsigned long delay = HZ + HZ * prandom_u32_max(4);
> pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
>kobject_name(kobj), kobj, __func__, kobj->parent, delay);
> INIT_DELAYED_WORK(>release, kobject_delayed_cleanup);
> diff --git a/lib/reed_solomon/test_rslib.c b/lib/reed_solomon/test_rslib.c
> index 6faf9c9a6215..4d241bdc88aa 100644
> --- a/lib/reed_solomon/test_rslib.c
> +++ b/lib/reed_solomon/test_rslib.c
> @@ -199,7 +199,7 @@ static int get_rcw_we(struct rs_control *rs, struct
> wspace *ws,
>
> derrlocs[i] = errloc;
>
> - if (ewsc && (prandom_u32() & 1)) {
> + if (ewsc && prandom_u32_max(2)) {
> /* Erasure with the symbol intact */
> errlocs[errloc] = 2;
> } else {
> diff --git a/lib/sbitmap.c b/lib/sbitmap.c
> index c4f04edf3ee9..ef0661504561 100644
> --- a/lib/sbitmap.c
> +++ b/lib/sbitmap.c
> @@ -21,7 +21,7 @@ static int init_alloc_hint(struct sbitmap *sb, gfp_t flags)
> int i;
>
> for_each_possible_cpu(i)
> - *per_cpu_ptr(sb->alloc_hint, i) = prandom_u32() % depth;
> +
Re: [PATCH v4 1/6] treewide: use prandom_u32_max() when possible, mechanically
On Fri, Oct 07, 2022 at 12:01:02PM -0600, Jason A. Donenfeld wrote: > Rather than incurring a division or requesting too many random bytes for > the given range, use the prandom_u32_max() function, which only takes > the minimum required bytes from the RNG and avoids divisions. This was > done mechanically with these coccinelle scripts: > > @no_modulo@ > expression E; > @@ > - (prandom_u32() % (E)) > + prandom_u32_max(E) > @no_modulo@ > expression E; > @@ > - (get_random_u32() % (E)) > + prandom_u32_max(E) > @no_modulo@ > expression E; > @@ > - (get_random_int() % (E)) > + prandom_u32_max(E) > > Reviewed-by: Kees Cook > Reviewed-by: KP Singh > Reviewed-by: Jan Kara # for ext4 and sbitmap > Acked-by: Ulf Hansson # for mmc > Signed-off-by: Jason A. Donenfeld For the XFS part, Acked-by: Darrick J. Wong --D > --- > arch/arm/kernel/process.c | 2 +- > arch/s390/kernel/vdso.c | 2 +- > arch/um/kernel/process.c | 2 +- > arch/x86/entry/vdso/vma.c | 2 +- > arch/x86/kernel/module.c | 2 +- > arch/x86/kernel/process.c | 2 +- > arch/x86/mm/pat/cpa-test.c| 4 +- > crypto/testmgr.c | 86 +-- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +- > drivers/infiniband/core/cma.c | 2 +- > drivers/infiniband/hw/cxgb4/id_table.c| 4 +- > drivers/infiniband/hw/hns/hns_roce_ah.c | 5 +- > drivers/infiniband/ulp/rtrs/rtrs-clt.c| 3 +- > .../test-drivers/vivid/vivid-touch-cap.c | 2 +- > drivers/mmc/core/core.c | 4 +- > drivers/mmc/host/dw_mmc.c | 2 +- > drivers/mtd/nand/raw/nandsim.c| 4 +- > drivers/mtd/tests/mtd_nandecctest.c | 10 +-- > drivers/mtd/ubi/debug.c | 2 +- > .../chelsio/inline_crypto/chtls/chtls_io.c| 4 +- > drivers/net/hamradio/baycom_epp.c | 2 +- > drivers/net/hamradio/hdlcdrv.c| 2 +- > drivers/net/hamradio/yam.c| 2 +- > drivers/net/phy/at803x.c | 2 +- > .../broadcom/brcm80211/brcmfmac/p2p.c | 2 +- > .../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c | 2 +- > drivers/scsi/fcoe/fcoe_ctlr.c | 4 +- > drivers/scsi/qedi/qedi_main.c | 2 +- > fs/ceph/inode.c | 2 +- > fs/ceph/mdsmap.c | 2 +- > fs/ext4/super.c | 7 +- > fs/f2fs/gc.c | 2 +- > fs/f2fs/segment.c | 8 +- > fs/ubifs/debug.c | 8 +- > fs/ubifs/lpt_commit.c | 12 +-- > fs/xfs/xfs_error.c| 2 +- > kernel/bpf/core.c | 4 +- > kernel/locking/test-ww_mutex.c| 4 +- > kernel/time/clocksource.c | 2 +- > lib/cmdline_kunit.c | 4 +- > lib/fault-inject.c| 2 +- > lib/find_bit_benchmark.c | 4 +- > lib/reed_solomon/test_rslib.c | 4 +- > lib/sbitmap.c | 2 +- > lib/test-string_helpers.c | 2 +- > lib/test_hexdump.c| 10 +-- > lib/test_kasan.c | 6 +- > lib/test_list_sort.c | 2 +- > mm/migrate.c | 2 +- > mm/slub.c | 2 +- > net/ceph/mon_client.c | 2 +- > net/ceph/osd_client.c | 2 +- > net/core/neighbour.c | 2 +- > net/core/pktgen.c | 39 - > net/core/stream.c | 2 +- > net/ipv4/igmp.c | 6 +- > net/ipv4/inet_connection_sock.c | 2 +- > net/ipv6/addrconf.c | 8 +- > net/ipv6/mcast.c | 10 +-- > net/netfilter/ipvs/ip_vs_twos.c | 4 +- > net/packet/af_packet.c| 2 +- > net/sched/act_gact.c | 2 +- > net/sched/act_sample.c| 2 +- > net/sched/sch_netem.c | 4 +- > net/sctp/socket.c | 2 +- > net/tipc/socket.c | 2 +- > net/xfrm/xfrm_state.c | 2 +- > 67 files changed, 173 insertions(+), 177 deletions(-) > > diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c > index 3d9cace63884..35129ae36067 100644 > --- a/arch/arm/kernel/process.c > +++
Re: [PATCH v4 1/3] block: sed-opal: SED Opal keystore
Reviewed-by: Jonathan Derrick
On 8/19/2022 4:31 PM, gjo...@linux.vnet.ibm.com wrote:
> From: Greg Joyce
>
> Add read and write functions that allow SED Opal keys to stored
> in a permanent keystore.
>
> Signed-off-by: Greg Joyce
> ---
> block/Makefile | 2 +-
> block/sed-opal-key.c | 23 +++
> include/linux/sed-opal-key.h | 15 +++
> 3 files changed, 39 insertions(+), 1 deletion(-)
> create mode 100644 block/sed-opal-key.c
> create mode 100644 include/linux/sed-opal-key.h
>
> diff --git a/block/Makefile b/block/Makefile
> index 4e01bb71ad6e..464a9f209552 100644
> --- a/block/Makefile
> +++ b/block/Makefile
> @@ -35,7 +35,7 @@ obj-$(CONFIG_BLK_DEV_ZONED) += blk-zoned.o
> obj-$(CONFIG_BLK_WBT)+= blk-wbt.o
> obj-$(CONFIG_BLK_DEBUG_FS) += blk-mq-debugfs.o
> obj-$(CONFIG_BLK_DEBUG_FS_ZONED)+= blk-mq-debugfs-zoned.o
> -obj-$(CONFIG_BLK_SED_OPAL) += sed-opal.o
> +obj-$(CONFIG_BLK_SED_OPAL) += sed-opal.o sed-opal-key.o
> obj-$(CONFIG_BLK_PM) += blk-pm.o
> obj-$(CONFIG_BLK_INLINE_ENCRYPTION) += blk-crypto.o blk-crypto-profile.o \
> blk-crypto-sysfs.o
> diff --git a/block/sed-opal-key.c b/block/sed-opal-key.c
> new file mode 100644
> index ..32ef988cd53b
> --- /dev/null
> +++ b/block/sed-opal-key.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SED key operations.
> + *
> + * Copyright (C) 2022 IBM Corporation
> + *
> + * These are the accessor functions (read/write) for SED Opal
> + * keys. Specific keystores can provide overrides.
> + *
> + */
> +
> +#include
> +#include
> +
> +int __weak sed_read_key(char *keyname, char *key, u_int *keylen)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +int __weak sed_write_key(char *keyname, char *key, u_int keylen)
> +{
> + return -EOPNOTSUPP;
> +}
> diff --git a/include/linux/sed-opal-key.h b/include/linux/sed-opal-key.h
> new file mode 100644
> index ..c9b1447986d8
> --- /dev/null
> +++ b/include/linux/sed-opal-key.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * SED key operations.
> + *
> + * Copyright (C) 2022 IBM Corporation
> + *
> + * These are the accessor functions (read/write) for SED Opal
> + * keys. Specific keystores can provide overrides.
> + *
> + */
> +
> +#include
> +
> +int sed_read_key(char *keyname, char *key, u_int *keylen);
> +int sed_write_key(char *keyname, char *key, u_int keylen);
Re: [PATCH v4 2/3] powerpc/pseries: PLPKS SED Opal keystore support
LGTM
Reviewed-by: Jonathan Derrick
On 8/19/2022 4:31 PM, gjo...@linux.vnet.ibm.com wrote:
> From: Greg Joyce
>
> Define operations for SED Opal to read/write keys
> from POWER LPAR Platform KeyStore(PLPKS). This allows
> for non-volatile storage of SED Opal keys.
>
> Signed-off-by: Greg Joyce
> ---
> arch/powerpc/platforms/pseries/Makefile | 1 +
> .../powerpc/platforms/pseries/plpks_sed_ops.c | 124 ++
> 2 files changed, 125 insertions(+)
> create mode 100644 arch/powerpc/platforms/pseries/plpks_sed_ops.c
>
> diff --git a/arch/powerpc/platforms/pseries/Makefile
> b/arch/powerpc/platforms/pseries/Makefile
> index 14e143b946a3..b7fea9e48a58 100644
> --- a/arch/powerpc/platforms/pseries/Makefile
> +++ b/arch/powerpc/platforms/pseries/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_PPC_SPLPAR)+= vphn.o
> obj-$(CONFIG_PPC_SVM)+= svm.o
> obj-$(CONFIG_FA_DUMP)+= rtas-fadump.o
> obj-$(CONFIG_PSERIES_PLPKS) += plpks.o
> +obj-$(CONFIG_PSERIES_PLPKS) += plpks_sed_ops.o
>
> obj-$(CONFIG_SUSPEND)+= suspend.o
> obj-$(CONFIG_PPC_VAS)+= vas.o vas-sysfs.o
> diff --git a/arch/powerpc/platforms/pseries/plpks_sed_ops.c
> b/arch/powerpc/platforms/pseries/plpks_sed_ops.c
> new file mode 100644
> index ..833226738448
> --- /dev/null
> +++ b/arch/powerpc/platforms/pseries/plpks_sed_ops.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * POWER Platform specific code for non-volatile SED key access
> + * Copyright (C) 2022 IBM Corporation
> + *
> + * Define operations for SED Opal to read/write keys
> + * from POWER LPAR Platform KeyStore(PLPKS).
> + *
> + * Self Encrypting Drives(SED) key storage using PLPKS
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "plpks.h"
> +
> +/*
> + * structure that contains all SED data
> + */
> +struct plpks_sed_object_data {
> + u_char version;
> + u_char pad1[7];
> + u_long authority;
> + u_long range;
> + u_int key_len;
> + u_char key[32];
> +};
> +
> +#define PLPKS_PLATVAR_POLICYWORLDREADABLE
> +#define PLPKS_PLATVAR_OS_COMMON 4
> +
> +#define PLPKS_SED_OBJECT_DATA_V00
> +#define PLPKS_SED_MANGLED_LABEL "/default/pri"
> +#define PLPKS_SED_COMPONENT "sed-opal"
> +#define PLPKS_SED_KEY "opal-boot-pin"
> +
> +/*
> + * authority is admin1 and range is global
> + */
> +#define PLPKS_SED_AUTHORITY 0x000900010001
> +#define PLPKS_SED_RANGE 0x08020001
> +
> +void plpks_init_var(struct plpks_var *var, char *keyname)
> +{
> + var->name = keyname;
> + var->namelen = strlen(keyname);
> + if (strcmp(PLPKS_SED_KEY, keyname) == 0) {
> + var->name = PLPKS_SED_MANGLED_LABEL;
> + var->namelen = strlen(keyname);
> + }
> + var->policy = PLPKS_PLATVAR_POLICY;
> + var->os = PLPKS_PLATVAR_OS_COMMON;
> + var->data = NULL;
> + var->datalen = 0;
> + var->component = PLPKS_SED_COMPONENT;
> +}
> +
> +/*
> + * Read the SED Opal key from PLPKS given the label
> + */
> +int sed_read_key(char *keyname, char *key, u_int *keylen)
> +{
> + struct plpks_var var;
> + struct plpks_sed_object_data *data;
> + u_int offset = 0;
> + int ret;
> +
> + plpks_init_var(, keyname);
> +
> + offset = offsetof(struct plpks_sed_object_data, key);
> +
> + ret = plpks_read_os_var();
> + if (ret != 0)
> + return ret;
> +
> + if (offset > var.datalen)
> + offset = 0;
> +
> + data = (struct plpks_sed_object_data *)var.data;
> + *keylen = be32_to_cpu(data->key_len);
> +
> + if (var.data) {
> + memcpy(key, var.data + offset, var.datalen - offset)> +
> key[*keylen] = '\0';
> + kfree(var.data);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Write the SED Opal key to PLPKS given the label
> + */
> +int sed_write_key(char *keyname, char *key, u_int keylen)
> +{
> + struct plpks_var var;
> + struct plpks_sed_object_data data;
> + struct plpks_var_name vname;
> +
> + plpks_init_var(, keyname);
> +
> + var.datalen = sizeof(struct plpks_sed_object_data);
> + var.data = (u8 *)
> +
> + /* initialize SED object */
> + data.version = PLPKS_SED_OBJECT_DATA_V0;
> + data.authority = cpu_to_be64(PLPKS_SED_AUTHORITY);
> + data.range = cpu_to_be64(PLPKS_SED_RANGE);
> + memset(, '\0', sizeof(data.pad1));
> + data.key_len = cpu_to_be32(keylen);
> + memcpy(data.key, (char *)key, keylen);
> +
> + /*
> + * Key update requires remove first. The return value
> + * is ignored since it's okay if the key doesn't exist.
> + */
> + vname.namelen = var.namelen;
> + vname.name = var.name;
> + plpks_remove_var(var.component, var.os, vname);
> +
> + return plpks_write_var(var);
> +}
Re: [PATCH v4 3/3] block: sed-opal: keystore access for SED Opal keys
LGTM besides comment below
Reviewed-by: Jonathan Derrick
On 8/19/2022 4:31 PM, gjo...@linux.vnet.ibm.com wrote:
> From: Greg Joyce
>
> Allow for permanent SED authentication keys by
> reading/writing to the SED Opal non-volatile keystore.
>
> Signed-off-by: Greg Joyce
> ---
> block/sed-opal.c | 18 --
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index 3bdb31cf3e7c..11b0eb3a656b 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -2697,7 +2698,13 @@ static int opal_set_new_pw(struct opal_dev *dev,
> struct opal_new_pw *opal_pw)
> if (ret)
> return ret;
>
> - /* update keyring with new password */
> + /* update keyring and arch var with new password */
> + ret = sed_write_key(OPAL_AUTH_KEY,
> + opal_pw->new_user_pw.opal_key.key,
> + opal_pw->new_user_pw.opal_key.key_len);
> + if (ret != -EOPNOTSUPP)
> + pr_warn("error updating SED key: %d\n", ret);
I cant see any reason this would fail and make the keys inconsistent, but it
seems
like update_sed_opal_key() should be dependent on sed_write_key() succeeding
> +
> ret = update_sed_opal_key(OPAL_AUTH_KEY,
> opal_pw->new_user_pw.opal_key.key,
> opal_pw->new_user_pw.opal_key.key_len);
> @@ -2920,6 +2927,8 @@ EXPORT_SYMBOL_GPL(sed_ioctl);
> static int __init sed_opal_init(void)
> {
> struct key *kr;
> + char init_sed_key[OPAL_KEY_MAX];
> + int keylen = OPAL_KEY_MAX;
>
> kr = keyring_alloc(".sed_opal",
> GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
> @@ -2932,6 +2941,11 @@ static int __init sed_opal_init(void)
>
> sed_opal_keyring = kr;
>
> - return 0;
> + if (sed_read_key(OPAL_AUTH_KEY, init_sed_key, ) < 0) {
> + memset(init_sed_key, '\0', sizeof(init_sed_key));
> + keylen = OPAL_KEY_MAX;
> + }
> +
> + return update_sed_opal_key(OPAL_AUTH_KEY, init_sed_key, keylen);
> }
> late_initcall(sed_opal_init);
Re: [PATCH v2 2/3 RESEND] block: sed-opal: Implement IOC_OPAL_REVERT_LSP
Reviewed-by: Jonathan Derrick
On 8/18/2022 8:30 AM, gjo...@linux.vnet.ibm.com wrote:
> From: Greg Joyce
>
> This is used in conjunction with IOC_OPAL_REVERT_TPR to return a drive to
> Original Factory State without erasing the data. If IOC_OPAL_REVERT_LSP
> is called with opal_revert_lsp.options bit OPAL_PRESERVE set prior
> to calling IOC_OPAL_REVERT_TPR, the drive global locking range will not
> be erased.
>
> Signed-off-by: Greg Joyce
> ---
> block/opal_proto.h| 4
> block/sed-opal.c | 40 +++
> include/linux/sed-opal.h | 1 +
> include/uapi/linux/sed-opal.h | 11 ++
> 4 files changed, 56 insertions(+)
>
> diff --git a/block/opal_proto.h b/block/opal_proto.h
> index b486b3ec7dc4..6127c08267f8 100644
> --- a/block/opal_proto.h
> +++ b/block/opal_proto.h
> @@ -210,6 +210,10 @@ enum opal_parameter {
> OPAL_SUM_SET_LIST = 0x06,
> };
>
> +enum opal_revertlsp {
> + OPAL_KEEP_GLOBAL_RANGE_KEY = 0x06,
> +};
> +
> /* Packets derived from:
> * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
> * Secion: 3.2.3 ComPackets, Packets & Subpackets
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index e4d8fbdc9dad..2916b9539b84 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -1593,6 +1593,26 @@ static int internal_activate_user(struct opal_dev
> *dev, void *data)
> return finalize_and_send(dev, parse_and_check_status);
> }
>
> +static int revert_lsp(struct opal_dev *dev, void *data)
> +{
> + struct opal_revert_lsp *rev = data;
> + int err;
> +
> + err = cmd_start(dev, opaluid[OPAL_THISSP_UID],
> + opalmethod[OPAL_REVERTSP]);
> + add_token_u8(, dev, OPAL_STARTNAME);
> + add_token_u64(, dev, OPAL_KEEP_GLOBAL_RANGE_KEY);
> + add_token_u8(, dev, (rev->options & OPAL_PRESERVE) ?
> + OPAL_TRUE : OPAL_FALSE);
> + add_token_u8(, dev, OPAL_ENDNAME);
> + if (err) {
> + pr_debug("Error building REVERT SP command.\n");
> + return err;
> + }
> +
> + return finalize_and_send(dev, parse_and_check_status);
> +}
> +
> static int erase_locking_range(struct opal_dev *dev, void *data)
> {
> struct opal_session_info *session = data;
> @@ -2208,6 +2228,23 @@ static int opal_get_discv(struct opal_dev *dev, struct
> opal_discovery *discv)
> return discv->size; /* modified to actual length of data */
> }
>
> +static int opal_revertlsp(struct opal_dev *dev, struct opal_revert_lsp *rev)
> +{
> + /* controller will terminate session */
> + const struct opal_step steps[] = {
> + { start_admin1LSP_opal_session, >key },
> + { revert_lsp, rev }
> + };
> + int ret;
> +
> + mutex_lock(>dev_lock);
> + setup_opal_dev(dev);
> + ret = execute_steps(dev, steps, ARRAY_SIZE(steps));
> + mutex_unlock(>dev_lock);
> +
> + return ret;
> +}
> +
> static int opal_erase_locking_range(struct opal_dev *dev,
> struct opal_session_info *opal_session)
> {
> @@ -2714,6 +2751,9 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd,
> void __user *arg)
> case IOC_OPAL_GENERIC_TABLE_RW:
> ret = opal_generic_read_write_table(dev, p);
> break;
> + case IOC_OPAL_REVERT_LSP:
> + ret = opal_revertlsp(dev, p);
> + break;
> case IOC_OPAL_DISCOVERY:
> ret = opal_get_discv(dev, p);
> break;
> diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
> index 9197b7a628f2..3a6082ff97e7 100644
> --- a/include/linux/sed-opal.h
> +++ b/include/linux/sed-opal.h
> @@ -43,6 +43,7 @@ static inline bool is_sed_ioctl(unsigned int cmd)
> case IOC_OPAL_MBR_DONE:
> case IOC_OPAL_WRITE_SHADOW_MBR:
> case IOC_OPAL_GENERIC_TABLE_RW:
> + case IOC_OPAL_REVERT_LSP:
> case IOC_OPAL_DISCOVERY:
> return true;
> }
> diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
> index 89dd108b426f..bc91987a6328 100644
> --- a/include/uapi/linux/sed-opal.h
> +++ b/include/uapi/linux/sed-opal.h
> @@ -51,6 +51,10 @@ struct opal_key {
> __u8 key[OPAL_KEY_MAX];
> };
>
> +enum opal_revert_lsp_opts {
> + OPAL_PRESERVE = 0x01,
> +};
> +
> struct opal_lr_act {
> struct opal_key key;
> __u32 sum;
> @@ -137,6 +141,12 @@ struct opal_discovery {
> __u64 size;
> };
>
> +struct opal_revert_lsp {
> + struct opal_key key;
> + __u32 options;
> + __u32 __pad;
> +};
> +
> #define IOC_OPAL_SAVE_IOW('p', 220, struct
> opal_lock_unlock)
> #define IOC_OPAL_LOCK_UNLOCK _IOW('p', 221, struct opal_lock_unlock)
> #define IOC_OPAL_TAKE_OWNERSHIP _IOW('p', 222, struct opal_key)
> @@ -154,5 +164,6 @@ struct opal_discovery {
> #define IOC_OPAL_WRITE_SHADOW_MBR _IOW('p', 234, struct opal_shadow_mbr)
> #define IOC_OPAL_GENERIC_TABLE_RW
Re: [PATCH v2 3/3 RESEND] block: sed-opal: keyring support for SED keys
LGTM thank you!
Reviewed-by: Jonathan Derrick
On 8/18/2022 8:30 AM, gjo...@linux.vnet.ibm.com wrote:
> From: Greg Joyce
>
> Extend the SED block driver so it can alternatively
> obtain a key from a sed-opal kernel keyring. The SED
> ioctls will indicate the source of the key, either
> directly in the ioctl data or from the keyring.
>
> This allows the use of SED commands in scripts such as
> udev scripts so that drives may be automatically unlocked
> as they become available.
>
> Signed-off-by: Greg Joyce
> ---
> block/Kconfig | 1 +
> block/sed-opal.c | 174 +-
> include/linux/sed-opal.h | 3 +
> include/uapi/linux/sed-opal.h | 8 +-
> 4 files changed, 183 insertions(+), 3 deletions(-)
>
> diff --git a/block/Kconfig b/block/Kconfig
> index 50b17e260fa2..f65169e9356b 100644
> --- a/block/Kconfig
> +++ b/block/Kconfig
> @@ -182,6 +182,7 @@ config BLK_DEBUG_FS_ZONED
>
> config BLK_SED_OPAL
> bool "Logic for interfacing with Opal enabled SEDs"
> + depends on KEYS
> help
> Builds Logic for interfacing with Opal enabled controllers.
> Enabling this option enables users to setup/unlock/lock
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index 2916b9539b84..3bdb31cf3e7c 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -20,6 +20,9 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
>
> #include "opal_proto.h"
>
> @@ -29,6 +32,8 @@
> /* Number of bytes needed by cmd_finalize. */
> #define CMD_FINALIZE_BYTES_NEEDED 7
>
> +static struct key *sed_opal_keyring;
> +
> struct opal_step {
> int (*fn)(struct opal_dev *dev, void *data);
> void *data;
> @@ -266,6 +271,101 @@ static void print_buffer(const u8 *ptr, u32 length)
> #endif
> }
>
> +/*
> + * Allocate/update a SED Opal key and add it to the SED Opal keyring.
> + */
> +static int update_sed_opal_key(const char *desc, u_char *key_data, int
> keylen)
> +{
> + key_ref_t kr;
> +
> + if (!sed_opal_keyring)
> + return -ENOKEY;
> +
> + kr = key_create_or_update(make_key_ref(sed_opal_keyring, true), "user",
> + desc, (const void *)key_data, keylen,
> + KEY_USR_VIEW | KEY_USR_SEARCH | KEY_USR_WRITE,
> + KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN |
> + KEY_ALLOC_BYPASS_RESTRICTION);
> + if (IS_ERR(kr)) {
> + pr_err("Error adding SED key (%ld)\n", PTR_ERR(kr));
> + return PTR_ERR(kr);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Read a SED Opal key from the SED Opal keyring.
> + */
> +static int read_sed_opal_key(const char *key_name, u_char *buffer, int
> buflen)
> +{
> + int ret;
> + key_ref_t kref;
> + struct key *key;
> +
> + if (!sed_opal_keyring)
> + return -ENOKEY;
> +
> + kref = keyring_search(make_key_ref(sed_opal_keyring, true),
> + _type_user, key_name, true);
> +
> + if (IS_ERR(kref))
> + ret = PTR_ERR(kref);
> +
> + key = key_ref_to_ptr(kref);
> + down_read(>sem);
> + ret = key_validate(key);
> + if (ret == 0) {
> + if (buflen > key->datalen)
> + buflen = key->datalen;
> +
> + ret = key->type->read(key, (char *)buffer, buflen);
> + }
> + up_read(>sem);
> +
> + key_ref_put(kref);
> +
> + return ret;
> +}
> +
> +static int opal_get_key(struct opal_dev *dev, struct opal_key *key)
> +{
> + int ret = 0;
> +
> + switch (key->key_type) {
> + case OPAL_INCLUDED:
> + /* the key is ready to use */
> + break;
> + case OPAL_KEYRING:
> + /* the key is in the keyring */
> + ret = read_sed_opal_key(OPAL_AUTH_KEY, key->key, OPAL_KEY_MAX);
> + if (ret > 0) {
> + if (ret > 255) {
> + ret = -ENOSPC;
> + goto error;
> + }
> + key->key_len = ret;
> + key->key_type = OPAL_INCLUDED;
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + if (ret < 0)
> + goto error;
> +
> + /* must have a PEK by now or it's an error */
> + if (key->key_type != OPAL_INCLUDED || key->key_len == 0) {
> + ret = -EINVAL;
> + goto error;
> + }
> + return 0;
> +error:
> + pr_debug("Error getting password: %d\n", ret);
> + return ret;
> +}
> +
> static bool check_tper(const void *data)
> {
> const struct d0_tper_features *tper = data;
> @@ -2204,6 +2304,9 @@ static int opal_secure_erase_locking_range(struct
> opal_dev *dev,
> };
> int ret;
>
> + ret = opal_get_key(dev, _session->opal_key);
> + if (ret)
> + return
Re: [PATCH v2 1/3 RESEND] block: sed-opal: Implement IOC_OPAL_DISCOVERY
Useful. Thank you
Reviewed-by: Jonathan Derrick
On 8/18/2022 8:30 AM, gjo...@linux.vnet.ibm.com wrote:
> From: Greg Joyce
>
> Add IOC_OPAL_DISCOVERY ioctl to return raw discovery data to a SED Opal
> application. This allows the application to display drive capabilities
> and state.
>
> Signed-off-by: Greg Joyce
> ---
> block/sed-opal.c | 38 ---
> include/linux/sed-opal.h | 1 +
> include/uapi/linux/sed-opal.h | 6 ++
> 3 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index 9700197000f2..e4d8fbdc9dad 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -426,8 +426,12 @@ static int execute_steps(struct opal_dev *dev,
> return error;
> }
>
> -static int opal_discovery0_end(struct opal_dev *dev)
> +static int opal_discovery0_end(struct opal_dev *dev, void *data)
> {
> + struct opal_discovery *discv_out = data; /* may be NULL */
> + u8 __user *buf_out;
> + u64 len_out;
> +
> bool found_com_id = false, supported = true, single_user = false;
> const struct d0_header *hdr = (struct d0_header *)dev->resp;
> const u8 *epos = dev->resp, *cpos = dev->resp;
> @@ -443,6 +447,15 @@ static int opal_discovery0_end(struct opal_dev *dev)
> return -EFAULT;
> }
>
> + if (discv_out) {
> + buf_out = (u8 __user *)(uintptr_t)discv_out->data;
> + len_out = min_t(u64, discv_out->size, hlen);
> + if (buf_out && copy_to_user(buf_out, dev->resp, len_out))
> + return -EFAULT;
> +
> + discv_out->size = hlen; /* actual size of data */
> + }
> +
> epos += hlen; /* end of buffer */
> cpos += sizeof(*hdr); /* current position on buffer */
>
> @@ -517,13 +530,13 @@ static int opal_discovery0(struct opal_dev *dev, void
> *data)
> if (ret)
> return ret;
>
> - return opal_discovery0_end(dev);
> + return opal_discovery0_end(dev, data);
> }
>
> static int opal_discovery0_step(struct opal_dev *dev)
> {
> const struct opal_step discovery0_step = {
> - opal_discovery0,
> + opal_discovery0, NULL
> };
>
> return execute_step(dev, _step, 0);
> @@ -2179,6 +2192,22 @@ static int opal_secure_erase_locking_range(struct
> opal_dev *dev,
> return ret;
> }
>
> +static int opal_get_discv(struct opal_dev *dev, struct opal_discovery *discv)
> +{
> + const struct opal_step discovery0_step = {
> + opal_discovery0, discv
> + };
> + int ret = 0;
> +
> + mutex_lock(>dev_lock);
> + setup_opal_dev(dev);
> + ret = execute_step(dev, _step, 0);
> + mutex_unlock(>dev_lock);
> + if (ret)
> + return ret;
> + return discv->size; /* modified to actual length of data */
> +}
> +
> static int opal_erase_locking_range(struct opal_dev *dev,
> struct opal_session_info *opal_session)
> {
> @@ -2685,6 +2714,9 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd,
> void __user *arg)
> case IOC_OPAL_GENERIC_TABLE_RW:
> ret = opal_generic_read_write_table(dev, p);
> break;
> + case IOC_OPAL_DISCOVERY:
> + ret = opal_get_discv(dev, p);
> + break;
> default:
> break;
> }
> diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
> index 1ac0d712a9c3..9197b7a628f2 100644
> --- a/include/linux/sed-opal.h
> +++ b/include/linux/sed-opal.h
> @@ -43,6 +43,7 @@ static inline bool is_sed_ioctl(unsigned int cmd)
> case IOC_OPAL_MBR_DONE:
> case IOC_OPAL_WRITE_SHADOW_MBR:
> case IOC_OPAL_GENERIC_TABLE_RW:
> + case IOC_OPAL_DISCOVERY:
> return true;
> }
> return false;
> diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
> index 6f5af1a84213..89dd108b426f 100644
> --- a/include/uapi/linux/sed-opal.h
> +++ b/include/uapi/linux/sed-opal.h
> @@ -132,6 +132,11 @@ struct opal_read_write_table {
> __u64 priv;
> };
>
> +struct opal_discovery {
> + __u64 data;
> + __u64 size;
> +};
> +
> #define IOC_OPAL_SAVE_IOW('p', 220, struct
> opal_lock_unlock)
> #define IOC_OPAL_LOCK_UNLOCK _IOW('p', 221, struct opal_lock_unlock)
> #define IOC_OPAL_TAKE_OWNERSHIP _IOW('p', 222, struct opal_key)
> @@ -148,5 +153,6 @@ struct opal_read_write_table {
> #define IOC_OPAL_MBR_DONE _IOW('p', 233, struct opal_mbr_done)
> #define IOC_OPAL_WRITE_SHADOW_MBR _IOW('p', 234, struct opal_shadow_mbr)
> #define IOC_OPAL_GENERIC_TABLE_RW _IOW('p', 235, struct
> opal_read_write_table)
> +#define IOC_OPAL_DISCOVERY _IOW('p', 236, struct opal_discovery)
>
> #endif /* _UAPI_SED_OPAL_H */
[PATCH v4 6/6] prandom: remove unused functions
With no callers left of prandom_u32() and prandom_bytes(), as well as
get_random_int(), remove these deprecated wrappers, in favor of
get_random_u32() and get_random_bytes().
Reviewed-by: Kees Cook
Signed-off-by: Jason A. Donenfeld
---
drivers/char/random.c | 11 +--
include/linux/prandom.h | 12
include/linux/random.h | 5 -
3 files changed, 5 insertions(+), 23 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 01acf235f263..2fe28eeb2f38 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -97,7 +97,7 @@ MODULE_PARM_DESC(ratelimit_disable, "Disable random ratelimit
suppression");
* Returns whether or not the input pool has been seeded and thus guaranteed
* to supply cryptographically secure random numbers. This applies to: the
* /dev/urandom device, the get_random_bytes function, and the get_random_{u8,
- * u16,u32,u64,int,long} family of functions.
+ * u16,u32,u64,long} family of functions.
*
* Returns: true if the input pool has been seeded.
* false if the input pool has not been seeded.
@@ -161,15 +161,14 @@ EXPORT_SYMBOL(wait_for_random_bytes);
* u16 get_random_u16()
* u32 get_random_u32()
* u64 get_random_u64()
- * unsigned int get_random_int()
* unsigned long get_random_long()
*
* These interfaces will return the requested number of random bytes
* into the given buffer or as a return value. This is equivalent to
- * a read from /dev/urandom. The u8, u16, u32, u64, int, and long
- * family of functions may be higher performance for one-off random
- * integers, because they do a bit of buffering and do not invoke
- * reseeding until the buffer is emptied.
+ * a read from /dev/urandom. The u8, u16, u32, u64, long family of
+ * functions may be higher performance for one-off random integers,
+ * because they do a bit of buffering and do not invoke reseeding
+ * until the buffer is emptied.
*
*/
diff --git a/include/linux/prandom.h b/include/linux/prandom.h
index 78db003bc290..e0a0759dd09c 100644
--- a/include/linux/prandom.h
+++ b/include/linux/prandom.h
@@ -12,18 +12,6 @@
#include
#include
-/* Deprecated: use get_random_u32 instead. */
-static inline u32 prandom_u32(void)
-{
- return get_random_u32();
-}
-
-/* Deprecated: use get_random_bytes instead. */
-static inline void prandom_bytes(void *buf, size_t nbytes)
-{
- return get_random_bytes(buf, nbytes);
-}
-
struct rnd_state {
__u32 s1, s2, s3, s4;
};
diff --git a/include/linux/random.h b/include/linux/random.h
index 08322f700cdc..147a5e0d0b8e 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -42,10 +42,6 @@ u8 get_random_u8(void);
u16 get_random_u16(void);
u32 get_random_u32(void);
u64 get_random_u64(void);
-static inline unsigned int get_random_int(void)
-{
- return get_random_u32();
-}
static inline unsigned long get_random_long(void)
{
#if BITS_PER_LONG == 64
@@ -100,7 +96,6 @@ declare_get_random_var_wait(u8, u8)
declare_get_random_var_wait(u16, u16)
declare_get_random_var_wait(u32, u32)
declare_get_random_var_wait(u64, u32)
-declare_get_random_var_wait(int, unsigned int)
declare_get_random_var_wait(long, unsigned long)
#undef declare_get_random_var
--
2.37.3
[PATCH v4 5/6] treewide: use get_random_bytes when possible
The prandom_bytes() function has been a deprecated inline wrapper around
get_random_bytes() for several releases now, and compiles down to the
exact same code. Replace the deprecated wrapper with a direct call to
the real function.
Reviewed-by: Kees Cook
Reviewed-by: Christophe Leroy # powerpc
Signed-off-by: Jason A. Donenfeld
---
arch/powerpc/crypto/crc-vpmsum_test.c | 2 +-
block/blk-crypto-fallback.c | 2 +-
crypto/async_tx/raid6test.c | 2 +-
drivers/dma/dmatest.c | 2 +-
drivers/mtd/nand/raw/nandsim.c | 2 +-
drivers/mtd/tests/mtd_nandecctest.c | 2 +-
drivers/mtd/tests/speedtest.c | 2 +-
drivers/mtd/tests/stresstest.c | 2 +-
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
drivers/net/ethernet/rocker/rocker_main.c | 2 +-
drivers/net/wireguard/selftest/allowedips.c | 12 ++--
fs/ubifs/debug.c| 2 +-
kernel/kcsan/selftest.c | 2 +-
lib/random32.c | 2 +-
lib/test_objagg.c | 2 +-
lib/uuid.c | 2 +-
net/ipv4/route.c| 2 +-
net/mac80211/rc80211_minstrel_ht.c | 2 +-
net/sched/sch_pie.c | 2 +-
19 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/crypto/crc-vpmsum_test.c
b/arch/powerpc/crypto/crc-vpmsum_test.c
index c1c1ef9457fb..273c527868db 100644
--- a/arch/powerpc/crypto/crc-vpmsum_test.c
+++ b/arch/powerpc/crypto/crc-vpmsum_test.c
@@ -82,7 +82,7 @@ static int __init crc_test_init(void)
if (len <= offset)
continue;
- prandom_bytes(data, len);
+ get_random_bytes(data, len);
len -= offset;
crypto_shash_update(crct10dif_shash, data+offset, len);
diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index 621abd1b0e4d..ad9844c5b40c 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -539,7 +539,7 @@ static int blk_crypto_fallback_init(void)
if (blk_crypto_fallback_inited)
return 0;
- prandom_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
+ get_random_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
err = bioset_init(_bio_split, 64, 0, 0);
if (err)
diff --git a/crypto/async_tx/raid6test.c b/crypto/async_tx/raid6test.c
index c9d218e53bcb..f74505f2baf0 100644
--- a/crypto/async_tx/raid6test.c
+++ b/crypto/async_tx/raid6test.c
@@ -37,7 +37,7 @@ static void makedata(int disks)
int i;
for (i = 0; i < disks; i++) {
- prandom_bytes(page_address(data[i]), PAGE_SIZE);
+ get_random_bytes(page_address(data[i]), PAGE_SIZE);
dataptrs[i] = data[i];
dataoffs[i] = 0;
}
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index 9fe2ae794316..ffe621695e47 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -312,7 +312,7 @@ static unsigned long dmatest_random(void)
{
unsigned long buf;
- prandom_bytes(, sizeof(buf));
+ get_random_bytes(, sizeof(buf));
return buf;
}
diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c
index 4bdaf4aa7007..c941a5a41ea6 100644
--- a/drivers/mtd/nand/raw/nandsim.c
+++ b/drivers/mtd/nand/raw/nandsim.c
@@ -1393,7 +1393,7 @@ static int ns_do_read_error(struct nandsim *ns, int num)
unsigned int page_no = ns->regs.row;
if (ns_read_error(page_no)) {
- prandom_bytes(ns->buf.byte, num);
+ get_random_bytes(ns->buf.byte, num);
NS_WARN("simulating read error in page %u\n", page_no);
return 1;
}
diff --git a/drivers/mtd/tests/mtd_nandecctest.c
b/drivers/mtd/tests/mtd_nandecctest.c
index 1c7201b0f372..440988562cfd 100644
--- a/drivers/mtd/tests/mtd_nandecctest.c
+++ b/drivers/mtd/tests/mtd_nandecctest.c
@@ -266,7 +266,7 @@ static int nand_ecc_test_run(const size_t size)
goto error;
}
- prandom_bytes(correct_data, size);
+ get_random_bytes(correct_data, size);
ecc_sw_hamming_calculate(correct_data, size, correct_ecc, sm_order);
for (i = 0; i < ARRAY_SIZE(nand_ecc_test); i++) {
nand_ecc_test[i].prepare(error_data, error_ecc,
diff --git a/drivers/mtd/tests/speedtest.c b/drivers/mtd/tests/speedtest.c
index c9ec7086bfa1..075bce32caa5 100644
--- a/drivers/mtd/tests/speedtest.c
+++ b/drivers/mtd/tests/speedtest.c
@@ -223,7 +223,7 @@ static int __init mtd_speedtest_init(void)
if (!iobuf)
goto out;
- prandom_bytes(iobuf, mtd->erasesize);
+ get_random_bytes(iobuf, mtd->erasesize);
bbt = kzalloc(ebcnt, GFP_KERNEL);
if (!bbt)
diff --git
[PATCH v4 4/6] treewide: use get_random_u32() when possible
The prandom_u32() function has been a deprecated inline wrapper around get_random_u32() for several releases now, and compiles down to the exact same code. Replace the deprecated wrapper with a direct call to the real function. The same also applies to get_random_int(), which is just a wrapper around get_random_u32(). Reviewed-by: Kees Cook Acked-by: Toke Høiland-Jørgensen # for sch_cake Acked-by: Chuck Lever # for nfsd Reviewed-by: Jan Kara # for ext4 Acked-by: Mika Westerberg # for thunderbolt Signed-off-by: Jason A. Donenfeld --- Documentation/networking/filter.rst| 2 +- arch/parisc/kernel/process.c | 2 +- arch/parisc/kernel/sys_parisc.c| 4 ++-- arch/s390/mm/mmap.c| 2 +- arch/x86/kernel/cpu/amd.c | 2 +- drivers/gpu/drm/i915/i915_gem_gtt.c| 6 +++--- drivers/gpu/drm/i915/selftests/i915_selftest.c | 2 +- drivers/gpu/drm/selftests/test-drm_buddy.c | 2 +- drivers/gpu/drm/selftests/test-drm_mm.c| 2 +- drivers/infiniband/hw/cxgb4/cm.c | 4 ++-- drivers/infiniband/hw/hfi1/tid_rdma.c | 2 +- drivers/infiniband/hw/mlx4/mad.c | 2 +- drivers/infiniband/ulp/ipoib/ipoib_cm.c| 2 +- drivers/md/raid5-cache.c | 2 +- .../media/test-drivers/vivid/vivid-touch-cap.c | 4 ++-- drivers/misc/habanalabs/gaudi2/gaudi2.c| 2 +- drivers/mtd/nand/raw/nandsim.c | 2 +- drivers/net/bonding/bond_main.c| 2 +- drivers/net/ethernet/broadcom/cnic.c | 2 +- .../chelsio/inline_crypto/chtls/chtls_cm.c | 2 +- drivers/net/ethernet/rocker/rocker_main.c | 6 +++--- .../wireless/broadcom/brcm80211/brcmfmac/pno.c | 2 +- .../net/wireless/marvell/mwifiex/cfg80211.c| 4 ++-- .../net/wireless/microchip/wilc1000/cfg80211.c | 2 +- .../net/wireless/quantenna/qtnfmac/cfg80211.c | 2 +- drivers/net/wireless/ti/wlcore/main.c | 2 +- drivers/nvme/common/auth.c | 2 +- drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 4 ++-- drivers/target/iscsi/cxgbit/cxgbit_cm.c| 2 +- drivers/thunderbolt/xdomain.c | 2 +- drivers/video/fbdev/uvesafb.c | 2 +- fs/exfat/inode.c | 2 +- fs/ext4/ialloc.c | 2 +- fs/ext4/ioctl.c| 4 ++-- fs/ext4/mmp.c | 2 +- fs/f2fs/namei.c| 2 +- fs/fat/inode.c | 2 +- fs/nfsd/nfs4state.c| 4 ++-- fs/ntfs3/fslog.c | 6 +++--- fs/ubifs/journal.c | 2 +- fs/xfs/libxfs/xfs_ialloc.c | 2 +- fs/xfs/xfs_icache.c| 2 +- fs/xfs/xfs_log.c | 2 +- include/net/netfilter/nf_queue.h | 2 +- include/net/red.h | 2 +- include/net/sock.h | 2 +- kernel/bpf/bloom_filter.c | 2 +- kernel/bpf/core.c | 2 +- kernel/bpf/hashtab.c | 2 +- kernel/bpf/verifier.c | 2 +- kernel/kcsan/selftest.c| 2 +- lib/random32.c | 2 +- lib/reed_solomon/test_rslib.c | 6 +++--- lib/test_fprobe.c | 2 +- lib/test_kprobes.c | 2 +- lib/test_min_heap.c| 6 +++--- lib/test_rhashtable.c | 6 +++--- mm/shmem.c | 2 +- mm/slab.c | 2 +- net/802/garp.c | 2 +- net/802/mrp.c | 2 +- net/core/pktgen.c | 4 ++-- net/ipv4/route.c | 2 +- net/ipv4/tcp_cdg.c | 2 +- net/ipv4/udp.c | 2 +- net/ipv6/ip6_flowlabel.c | 2 +- net/ipv6/output_core.c | 2 +- net/netfilter/ipvs/ip_vs_conn.c| 2 +- net/netfilter/xt_statistic.c | 2 +- net/openvswitch/actions.c | 2 +- net/rds/bind.c | 2 +- net/sched/sch_cake.c | 2 +- net/sched/sch_netem.c | 18 +- net/sunrpc/auth_gss/gss_krb5_wrap.c| 4 ++-- net/sunrpc/xprt.c | 2 +- net/unix/af_unix.c | 2 +- 76 files changed, 105 insertions(+), 105 deletions(-) diff --git a/Documentation/networking/filter.rst
[PATCH v4 3/6] treewide: use get_random_{u8,u16}() when possible
Rather than truncate a 32-bit value to a 16-bit value or an 8-bit value,
simply use the get_random_{u8,u16}() functions, which are faster than
wasting the additional bytes from a 32-bit value.
Reviewed-by: Kees Cook
Acked-by: Toke Høiland-Jørgensen # for sch_cake
Signed-off-by: Jason A. Donenfeld
---
arch/arm/kernel/signal.c | 2 +-
arch/arm64/kernel/syscall.c | 2 +-
arch/s390/kernel/process.c| 2 +-
arch/sparc/vdso/vma.c | 2 +-
crypto/testmgr.c | 8
drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 2 +-
drivers/media/test-drivers/vivid/vivid-radio-rx.c | 4 ++--
.../net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c | 2 +-
drivers/net/hamradio/baycom_epp.c | 2 +-
drivers/net/hamradio/hdlcdrv.c| 2 +-
drivers/net/hamradio/yam.c| 2 +-
drivers/net/wireguard/selftest/allowedips.c | 4 ++--
drivers/net/wireless/st/cw1200/wsm.c | 2 +-
drivers/scsi/lpfc/lpfc_hbadisc.c | 6 +++---
lib/cmdline_kunit.c | 4 ++--
lib/test_vmalloc.c| 2 +-
net/dccp/ipv4.c | 4 ++--
net/ipv4/datagram.c | 2 +-
net/ipv4/ip_output.c | 2 +-
net/ipv4/tcp_ipv4.c | 4 ++--
net/mac80211/scan.c | 2 +-
net/netfilter/nf_nat_core.c | 4 ++--
net/sched/sch_cake.c | 6 +++---
net/sched/sch_sfb.c | 2 +-
net/sctp/socket.c | 2 +-
25 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index ea128e32e8ca..e07f359254c3 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -655,7 +655,7 @@ struct page *get_signal_page(void)
PAGE_SIZE / sizeof(u32));
/* Give the signal return code some randomness */
- offset = 0x200 + (get_random_int() & 0x7fc);
+ offset = 0x200 + (get_random_u16() & 0x7fc);
signal_return_offset = offset;
/* Copy signal return handlers into the page */
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index 733451fe7e41..d72e8f23422d 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -67,7 +67,7 @@ static void invoke_syscall(struct pt_regs *regs, unsigned int
scno,
*
* The resulting 5 bits of entropy is seen in SP[8:4].
*/
- choose_random_kstack_offset(get_random_int() & 0x1FF);
+ choose_random_kstack_offset(get_random_u16() & 0x1FF);
}
static inline bool has_syscall_work(unsigned long flags)
diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index 5ec78555dd2e..42af4b3aa02b 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -230,7 +230,7 @@ unsigned long arch_align_stack(unsigned long sp)
static inline unsigned long brk_rnd(void)
{
- return (get_random_int() & BRK_RND_MASK) << PAGE_SHIFT;
+ return (get_random_u16() & BRK_RND_MASK) << PAGE_SHIFT;
}
unsigned long arch_randomize_brk(struct mm_struct *mm)
diff --git a/arch/sparc/vdso/vma.c b/arch/sparc/vdso/vma.c
index cc19e09b0fa1..04ee726859ca 100644
--- a/arch/sparc/vdso/vma.c
+++ b/arch/sparc/vdso/vma.c
@@ -354,7 +354,7 @@ static unsigned long vdso_addr(unsigned long start,
unsigned int len)
unsigned int offset;
/* This loses some more bits than a modulo, but is cheaper */
- offset = get_random_int() & (PTRS_PER_PTE - 1);
+ offset = get_random_u16() & (PTRS_PER_PTE - 1);
return start + (offset << PAGE_SHIFT);
}
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index be45217acde4..981c637fa2ed 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -927,7 +927,7 @@ static void generate_random_bytes(u8 *buf, size_t count)
b = 0xff;
break;
default:
- b = (u8)prandom_u32();
+ b = get_random_u8();
break;
}
memset(buf, b, count);
@@ -935,8 +935,8 @@ static void generate_random_bytes(u8 *buf, size_t count)
break;
case 2:
/* Ascending or descending bytes, plus optional mutations */
- increment = (u8)prandom_u32();
- b = (u8)prandom_u32();
+ increment = get_random_u8();
+ b = get_random_u8();
for (i = 0; i < count; i++, b +=
[PATCH v4 2/6] treewide: use prandom_u32_max() when possible
Rather than incurring a division or requesting too many random bytes for
the given range, use the prandom_u32_max() function, which only takes
the minimum required bytes from the RNG and avoids divisions.
Reviewed-by: Kees Cook
Reviewed-by: KP Singh
Reviewed-by: Christoph Böhmwalder # for drbd
Reviewed-by: Jan Kara # for ext2, ext4, and sbitmap
Signed-off-by: Jason A. Donenfeld
---
arch/arm64/kernel/process.c | 2 +-
arch/loongarch/kernel/process.c | 2 +-
arch/loongarch/kernel/vdso.c | 2 +-
arch/mips/kernel/process.c | 2 +-
arch/mips/kernel/vdso.c | 2 +-
arch/parisc/kernel/vdso.c| 2 +-
arch/powerpc/kernel/process.c| 2 +-
arch/s390/kernel/process.c | 2 +-
drivers/block/drbd/drbd_receiver.c | 4 ++--
drivers/md/bcache/request.c | 2 +-
drivers/mtd/tests/stresstest.c | 17 -
drivers/mtd/ubi/debug.h | 6 +++---
drivers/net/ethernet/broadcom/cnic.c | 3 +--
fs/ext2/ialloc.c | 3 +--
fs/ext4/ialloc.c | 5 ++---
fs/ubifs/lpt_commit.c| 2 +-
fs/ubifs/tnc_commit.c| 2 +-
fs/xfs/libxfs/xfs_alloc.c| 2 +-
fs/xfs/libxfs/xfs_ialloc.c | 2 +-
include/linux/nodemask.h | 2 +-
lib/cmdline_kunit.c | 4 ++--
lib/kobject.c| 2 +-
lib/reed_solomon/test_rslib.c| 2 +-
lib/sbitmap.c| 2 +-
lib/test_hexdump.c | 2 +-
lib/test_vmalloc.c | 17 -
net/core/pktgen.c| 4 ++--
net/ipv4/inet_hashtables.c | 2 +-
net/sunrpc/cache.c | 2 +-
net/sunrpc/xprtsock.c| 2 +-
30 files changed, 42 insertions(+), 63 deletions(-)
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 92bcc1768f0b..87203429f802 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -595,7 +595,7 @@ unsigned long __get_wchan(struct task_struct *p)
unsigned long arch_align_stack(unsigned long sp)
{
if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
- sp -= get_random_int() & ~PAGE_MASK;
+ sp -= prandom_u32_max(PAGE_SIZE);
return sp & ~0xf;
}
diff --git a/arch/loongarch/kernel/process.c b/arch/loongarch/kernel/process.c
index 660492f064e7..1256e3582475 100644
--- a/arch/loongarch/kernel/process.c
+++ b/arch/loongarch/kernel/process.c
@@ -293,7 +293,7 @@ unsigned long stack_top(void)
unsigned long arch_align_stack(unsigned long sp)
{
if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
- sp -= get_random_int() & ~PAGE_MASK;
+ sp -= prandom_u32_max(PAGE_SIZE);
return sp & STACK_ALIGN;
}
diff --git a/arch/loongarch/kernel/vdso.c b/arch/loongarch/kernel/vdso.c
index f32c38abd791..8c9826062652 100644
--- a/arch/loongarch/kernel/vdso.c
+++ b/arch/loongarch/kernel/vdso.c
@@ -78,7 +78,7 @@ static unsigned long vdso_base(void)
unsigned long base = STACK_TOP;
if (current->flags & PF_RANDOMIZE) {
- base += get_random_int() & (VDSO_RANDOMIZE_SIZE - 1);
+ base += prandom_u32_max(VDSO_RANDOMIZE_SIZE);
base = PAGE_ALIGN(base);
}
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 35b912bce429..bbe9ce471791 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -711,7 +711,7 @@ unsigned long mips_stack_top(void)
unsigned long arch_align_stack(unsigned long sp)
{
if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
- sp -= get_random_int() & ~PAGE_MASK;
+ sp -= prandom_u32_max(PAGE_SIZE);
return sp & ALMASK;
}
diff --git a/arch/mips/kernel/vdso.c b/arch/mips/kernel/vdso.c
index b2cc2c2dd4bf..5fd9bf1d596c 100644
--- a/arch/mips/kernel/vdso.c
+++ b/arch/mips/kernel/vdso.c
@@ -79,7 +79,7 @@ static unsigned long vdso_base(void)
}
if (current->flags & PF_RANDOMIZE) {
- base += get_random_int() & (VDSO_RANDOMIZE_SIZE - 1);
+ base += prandom_u32_max(VDSO_RANDOMIZE_SIZE);
base = PAGE_ALIGN(base);
}
diff --git a/arch/parisc/kernel/vdso.c b/arch/parisc/kernel/vdso.c
index 63dc44c4c246..47e5960a2f96 100644
--- a/arch/parisc/kernel/vdso.c
+++ b/arch/parisc/kernel/vdso.c
@@ -75,7 +75,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm,
map_base = mm->mmap_base;
if (current->flags & PF_RANDOMIZE)
- map_base -= (get_random_int() & 0x1f) * PAGE_SIZE;
+ map_base -= prandom_u32_max(0x20) * PAGE_SIZE;
vdso_text_start = get_unmapped_area(NULL, map_base, vdso_text_len, 0,
0);
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index
[PATCH v4 1/6] treewide: use prandom_u32_max() when possible, mechanically
Rather than incurring a division or requesting too many random bytes for the given range, use the prandom_u32_max() function, which only takes the minimum required bytes from the RNG and avoids divisions. This was done mechanically with these coccinelle scripts: @no_modulo@ expression E; @@ - (prandom_u32() % (E)) + prandom_u32_max(E) @no_modulo@ expression E; @@ - (get_random_u32() % (E)) + prandom_u32_max(E) @no_modulo@ expression E; @@ - (get_random_int() % (E)) + prandom_u32_max(E) Reviewed-by: Kees Cook Reviewed-by: KP Singh Reviewed-by: Jan Kara # for ext4 and sbitmap Acked-by: Ulf Hansson # for mmc Signed-off-by: Jason A. Donenfeld --- arch/arm/kernel/process.c | 2 +- arch/s390/kernel/vdso.c | 2 +- arch/um/kernel/process.c | 2 +- arch/x86/entry/vdso/vma.c | 2 +- arch/x86/kernel/module.c | 2 +- arch/x86/kernel/process.c | 2 +- arch/x86/mm/pat/cpa-test.c| 4 +- crypto/testmgr.c | 86 +-- .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +- drivers/infiniband/core/cma.c | 2 +- drivers/infiniband/hw/cxgb4/id_table.c| 4 +- drivers/infiniband/hw/hns/hns_roce_ah.c | 5 +- drivers/infiniband/ulp/rtrs/rtrs-clt.c| 3 +- .../test-drivers/vivid/vivid-touch-cap.c | 2 +- drivers/mmc/core/core.c | 4 +- drivers/mmc/host/dw_mmc.c | 2 +- drivers/mtd/nand/raw/nandsim.c| 4 +- drivers/mtd/tests/mtd_nandecctest.c | 10 +-- drivers/mtd/ubi/debug.c | 2 +- .../chelsio/inline_crypto/chtls/chtls_io.c| 4 +- drivers/net/hamradio/baycom_epp.c | 2 +- drivers/net/hamradio/hdlcdrv.c| 2 +- drivers/net/hamradio/yam.c| 2 +- drivers/net/phy/at803x.c | 2 +- .../broadcom/brcm80211/brcmfmac/p2p.c | 2 +- .../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c | 2 +- drivers/scsi/fcoe/fcoe_ctlr.c | 4 +- drivers/scsi/qedi/qedi_main.c | 2 +- fs/ceph/inode.c | 2 +- fs/ceph/mdsmap.c | 2 +- fs/ext4/super.c | 7 +- fs/f2fs/gc.c | 2 +- fs/f2fs/segment.c | 8 +- fs/ubifs/debug.c | 8 +- fs/ubifs/lpt_commit.c | 12 +-- fs/xfs/xfs_error.c| 2 +- kernel/bpf/core.c | 4 +- kernel/locking/test-ww_mutex.c| 4 +- kernel/time/clocksource.c | 2 +- lib/cmdline_kunit.c | 4 +- lib/fault-inject.c| 2 +- lib/find_bit_benchmark.c | 4 +- lib/reed_solomon/test_rslib.c | 4 +- lib/sbitmap.c | 2 +- lib/test-string_helpers.c | 2 +- lib/test_hexdump.c| 10 +-- lib/test_kasan.c | 6 +- lib/test_list_sort.c | 2 +- mm/migrate.c | 2 +- mm/slub.c | 2 +- net/ceph/mon_client.c | 2 +- net/ceph/osd_client.c | 2 +- net/core/neighbour.c | 2 +- net/core/pktgen.c | 39 - net/core/stream.c | 2 +- net/ipv4/igmp.c | 6 +- net/ipv4/inet_connection_sock.c | 2 +- net/ipv6/addrconf.c | 8 +- net/ipv6/mcast.c | 10 +-- net/netfilter/ipvs/ip_vs_twos.c | 4 +- net/packet/af_packet.c| 2 +- net/sched/act_gact.c | 2 +- net/sched/act_sample.c| 2 +- net/sched/sch_netem.c | 4 +- net/sctp/socket.c | 2 +- net/tipc/socket.c | 2 +- net/xfrm/xfrm_state.c | 2 +- 67 files changed, 173 insertions(+), 177 deletions(-) diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c index 3d9cace63884..35129ae36067 100644 --- a/arch/arm/kernel/process.c +++ b/arch/arm/kernel/process.c @@ -375,7 +375,7 @@ static unsigned long sigpage_addr(const struct mm_struct *mm, slots = ((last - first) >> PAGE_SHIFT) + 1; - offset = get_random_int() % slots; + offset = prandom_u32_max(slots); addr = first + (offset << PAGE_SHIFT); diff --git
[PATCH v4 0/6] treewide cleanup of random integer usage
Changes v3->v4: - Split coccinelle mechanical step from non-mechanical step. - Handle `get_random_int() & ~PAGE_MASK` -> `prandom_u32_max(PAGE_SIZE)`. Hi folks, This is a five part treewide cleanup of random integer handling. The rules for random integers are: - If you want a secure or an insecure random u64, use get_random_u64(). - If you want a secure or an insecure random u32, use get_random_u32(). * The old function prandom_u32() has been deprecated for a while now and is just a wrapper around get_random_u32(). Same for get_random_int(). - If you want a secure or an insecure random u16, use get_random_u16(). - If you want a secure or an insecure random u8, use get_random_u8(). - If you want secure or insecure random bytes, use get_random_bytes(). * The old function prandom_bytes() has been deprecated for a while now and has long been a wrapper around get_random_bytes(). - If you want a non-uniform random u32, u16, or u8 bounded by a certain open interval maximum, use prandom_u32_max(). * I say "non-uniform", because it doesn't do any rejection sampling or divisions. Hence, it stays within the prandom_* namespace. These rules ought to be applied uniformly, so that we can clean up the deprecated functions, and earn the benefits of using the modern functions. In particular, in addition to the boring substitutions, this patchset accomplishes a few nice effects: - By using prandom_u32_max() with an upper-bound that the compiler can prove at compile-time is ≤65536 or ≤256, internally get_random_u16() or get_random_u8() is used, which wastes fewer batched random bytes, and hence has higher throughput. - By using prandom_u32_max() instead of %, when the upper-bound is not a constant, division is still avoided, because prandom_u32_max() uses a faster multiplication-based trick instead. - By using get_random_u16() or get_random_u8() in cases where the return value is intended to indeed be a u16 or a u8, we waste fewer batched random bytes, and hence have higher throughput. So, based on those rules and benefits from following them, this patchset breaks down into the following five steps, which were done mostly manually, but the first step was split into two patches, the first of which is cocinelle-able: 1) Replace `prandom_u32() % max` and variants thereof with prandom_u32_max(max). 2) Replace `(type)get_random_u32()` and variants thereof with get_random_u16() or get_random_u8(). I took the pains to actually look and see what every lvalue type was across the entire tree. 3) Replace remaining deprecated uses of prandom_u32() and get_random_int() with get_random_u32(). 4) Replace remaining deprecated uses of prandom_bytes() with get_random_bytes(). 5) Remove the deprecated and now-unused prandom_u32() and prandom_bytes() inline wrapper functions. I was thinking of taking this through my random.git tree (on which this series is currently based) and submitting it near the end of the merge window, or waiting for the very end of the 6.1 cycle when there will be the fewest new patches brewing. If somebody with some treewide-cleanup experience might share some wisdom about what the best timing usually winds up being, I'm all ears. Please take a look! At "379 insertions(+), 422 deletions(-)", this should be a somewhat small patchset to review, despite it having the scary "treewide" moniker on it. Thanks, Jason Cc: Andreas Noever Cc: Andrew Morton Cc: Andy Shevchenko Cc: Borislav Petkov Cc: Catalin Marinas Cc: Christoph Böhmwalder Cc: Christoph Hellwig Cc: Christophe Leroy Cc: Daniel Borkmann Cc: Dave Airlie Cc: Dave Hansen Cc: David S. Miller Cc: Eric Dumazet Cc: Florian Westphal Cc: Greg Kroah-Hartman , Cc: H. Peter Anvin Cc: Heiko Carstens Cc: Helge Deller Cc: Herbert Xu Cc: Huacai Chen Cc: Hugh Dickins Cc: Jakub Kicinski Cc: James E.J. Bottomley Cc: Jan Kara Cc: Jason Gunthorpe Cc: Jens Axboe Cc: Johannes Berg Cc: Jonathan Corbet Cc: Jozsef Kadlecsik Cc: KP Singh Cc: Kees Cook Cc: Marco Elver Cc: Mauro Carvalho Chehab Cc: Michael Ellerman Cc: Pablo Neira Ayuso Cc: Paolo Abeni Cc: Peter Zijlstra Cc: Richard Weinberger Cc: Russell King Cc: Theodore Ts'o Cc: Thomas Bogendoerfer Cc: Thomas Gleixner Cc: Thomas Graf Cc: Ulf Hansson Cc: Vignesh Raghavendra Cc: WANG Xuerui Cc: Will Deacon Cc: Yury Norov Cc: dri-de...@lists.freedesktop.org Cc: kasan-...@googlegroups.com Cc: kernel-janit...@vger.kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-bl...@vger.kernel.org Cc: linux-cry...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-fsde...@vger.kernel.org Cc: linux-me...@vger.kernel.org Cc: linux-m...@vger.kernel.org Cc: linux...@kvack.org Cc: linux-...@vger.kernel.org Cc: linux-...@lists.infradead.org Cc: linux-n...@lists.infradead.org Cc: linux-par...@vger.kernel.org Cc: linux-r...@vger.kernel.org Cc: linux-s...@vger.kernel.org Cc: linux...@lists.infradead.org Cc: linux-...@vger.kernel.org
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Fri, Oct 07, 2022 at 10:12:42AM -0700, Kees Cook wrote:
> On Fri, Oct 07, 2022 at 08:07:58AM -0600, Jason A. Donenfeld wrote:
> > On Fri, Oct 07, 2022 at 04:57:24AM +, Christophe Leroy wrote:
> > >
> > >
> > > Le 07/10/2022 à 01:36, Jason A. Donenfeld a écrit :
> > > > On 10/6/22, Christophe Leroy wrote:
> > > >>
> > > >>
> > > >> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
> > > >>>
> > > >>>
> > > >>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
> > > Hi Christophe,
> > >
> > > On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
> > > wrote:
> > > > Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> > > >> The prandom_u32() function has been a deprecated inline wrapper
> > > >> around
> > > >> get_random_u32() for several releases now, and compiles down to the
> > > >> exact same code. Replace the deprecated wrapper with a direct call
> > > >> to
> > > >> the real function. The same also applies to get_random_int(),
> > > >> which is
> > > >> just a wrapper around get_random_u32().
> > > >>
> > > >> Reviewed-by: Kees Cook
> > > >> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> > > >> Acked-by: Chuck Lever # for nfsd
> > > >> Reviewed-by: Jan Kara # for ext4
> > > >> Signed-off-by: Jason A. Donenfeld
> > > >> ---
> > > >
> > > >> diff --git a/arch/powerpc/kernel/process.c
> > > >> b/arch/powerpc/kernel/process.c
> > > >> index 0fbda89cd1bb..9c4c15afbbe8 100644
> > > >> --- a/arch/powerpc/kernel/process.c
> > > >> +++ b/arch/powerpc/kernel/process.c
> > > >> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
> > > >> unsigned long arch_align_stack(unsigned long sp)
> > > >> {
> > > >> if (!(current->personality & ADDR_NO_RANDOMIZE) &&
> > > >> randomize_va_space)
> > > >> - sp -= get_random_int() & ~PAGE_MASK;
> > > >> + sp -= get_random_u32() & ~PAGE_MASK;
> > > >> return sp & ~0xf;
> > > >
> > > > Isn't that a candidate for prandom_u32_max() ?
> > > >
> > > > Note that sp is deemed to be 16 bytes aligned at all time.
> > >
> > > Yes, probably. It seemed non-trivial to think about, so I didn't. But
> > > let's see here... maybe it's not too bad:
> > >
> > > If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
> > > (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
> > > thing? Is that accurate? And holds across platforms (this comes up a
> > > few places)? If so, I'll do that for a v4.
> > >
> > > >>>
> > > >>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
> > > >>>
> > > >>> /*
> > > >>>* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if
> > > >>> we
> > > >>>* assign PAGE_MASK to a larger type it gets extended the way we
> > > >>> want
> > > >>>* (i.e. with 1s in the high bits)
> > > >>>*/
> > > >>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
> > > >>>
> > > >>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
> > > >>>
> > > >>>
> > > >>> So it would work I guess.
> > > >>
> > > >> But taking into account that sp must remain 16 bytes aligned, would it
> > > >> be better to do something like ?
> > > >>
> > > >>sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
> > > >>
> > > >>return sp;
> > > >
> > > > Does this assume that sp is already aligned at the beginning of the
> > > > function? I'd assume from the function's name that this isn't the
> > > > case?
> > >
> > > Ah you are right, I overlooked it.
> >
> > So I think to stay on the safe side, I'm going to go with
> > `prandom_u32_max(PAGE_SIZE)`. Sound good?
>
> Given these kinds of less mechanical changes, it may make sense to split
> these from the "trivial" conversions in a treewide patch. The chance of
> needing a revert from the simple 1:1 conversions is much lower than the
> need to revert by-hand changes.
>
> The Cocci script I suggested in my v1 review gets 80% of the first
> patch, for example.
I'll split things up into a mechanical step and a non-mechanical step.
Good idea.
Jason
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Fri, Oct 07, 2022 at 08:07:58AM -0600, Jason A. Donenfeld wrote:
> On Fri, Oct 07, 2022 at 04:57:24AM +, Christophe Leroy wrote:
> >
> >
> > Le 07/10/2022 à 01:36, Jason A. Donenfeld a écrit :
> > > On 10/6/22, Christophe Leroy wrote:
> > >>
> > >>
> > >> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
> > >>>
> > >>>
> > >>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
> > Hi Christophe,
> >
> > On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
> > wrote:
> > > Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> > >> The prandom_u32() function has been a deprecated inline wrapper
> > >> around
> > >> get_random_u32() for several releases now, and compiles down to the
> > >> exact same code. Replace the deprecated wrapper with a direct call to
> > >> the real function. The same also applies to get_random_int(), which
> > >> is
> > >> just a wrapper around get_random_u32().
> > >>
> > >> Reviewed-by: Kees Cook
> > >> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> > >> Acked-by: Chuck Lever # for nfsd
> > >> Reviewed-by: Jan Kara # for ext4
> > >> Signed-off-by: Jason A. Donenfeld
> > >> ---
> > >
> > >> diff --git a/arch/powerpc/kernel/process.c
> > >> b/arch/powerpc/kernel/process.c
> > >> index 0fbda89cd1bb..9c4c15afbbe8 100644
> > >> --- a/arch/powerpc/kernel/process.c
> > >> +++ b/arch/powerpc/kernel/process.c
> > >> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
> > >> unsigned long arch_align_stack(unsigned long sp)
> > >> {
> > >> if (!(current->personality & ADDR_NO_RANDOMIZE) &&
> > >> randomize_va_space)
> > >> - sp -= get_random_int() & ~PAGE_MASK;
> > >> + sp -= get_random_u32() & ~PAGE_MASK;
> > >> return sp & ~0xf;
> > >
> > > Isn't that a candidate for prandom_u32_max() ?
> > >
> > > Note that sp is deemed to be 16 bytes aligned at all time.
> >
> > Yes, probably. It seemed non-trivial to think about, so I didn't. But
> > let's see here... maybe it's not too bad:
> >
> > If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
> > (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
> > thing? Is that accurate? And holds across platforms (this comes up a
> > few places)? If so, I'll do that for a v4.
> >
> > >>>
> > >>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
> > >>>
> > >>> /*
> > >>>* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
> > >>>* assign PAGE_MASK to a larger type it gets extended the way we want
> > >>>* (i.e. with 1s in the high bits)
> > >>>*/
> > >>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
> > >>>
> > >>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
> > >>>
> > >>>
> > >>> So it would work I guess.
> > >>
> > >> But taking into account that sp must remain 16 bytes aligned, would it
> > >> be better to do something like ?
> > >>
> > >> sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
> > >>
> > >> return sp;
> > >
> > > Does this assume that sp is already aligned at the beginning of the
> > > function? I'd assume from the function's name that this isn't the
> > > case?
> >
> > Ah you are right, I overlooked it.
>
> So I think to stay on the safe side, I'm going to go with
> `prandom_u32_max(PAGE_SIZE)`. Sound good?
Given these kinds of less mechanical changes, it may make sense to split
these from the "trivial" conversions in a treewide patch. The chance of
needing a revert from the simple 1:1 conversions is much lower than the
need to revert by-hand changes.
The Cocci script I suggested in my v1 review gets 80% of the first
patch, for example.
--
Kees Cook
[RFC PATCH 5/5] treewide: Rename and trace arch-definitions of smp_send_reschedule()
To be able to trace invocations of smp_send_reschedule(), rename the
arch-specific definitions of it to arch_smp_send_reschedule() and wrap it
into an smp_send_reschedule() that contains a tracepoint.
Signed-off-by: Valentin Schneider
---
arch/alpha/kernel/smp.c | 2 +-
arch/arc/kernel/smp.c| 2 +-
arch/arm/kernel/smp.c| 2 +-
arch/arm64/kernel/smp.c | 2 +-
arch/csky/kernel/smp.c | 2 +-
arch/hexagon/kernel/smp.c| 2 +-
arch/ia64/kernel/smp.c | 4 ++--
arch/loongarch/include/asm/smp.h | 2 +-
arch/mips/include/asm/smp.h | 2 +-
arch/openrisc/kernel/smp.c | 2 +-
arch/parisc/kernel/smp.c | 4 ++--
arch/powerpc/kernel/smp.c| 4 ++--
arch/riscv/kernel/smp.c | 4 ++--
arch/s390/kernel/smp.c | 2 +-
arch/sh/kernel/smp.c | 2 +-
arch/sparc/kernel/smp_32.c | 2 +-
arch/sparc/kernel/smp_64.c | 2 +-
arch/x86/include/asm/smp.h | 2 +-
arch/xtensa/kernel/smp.c | 2 +-
include/linux/smp.h | 1 +
kernel/smp.c | 6 ++
21 files changed, 30 insertions(+), 23 deletions(-)
diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
index f4e20f75438f..38637eb9eebd 100644
--- a/arch/alpha/kernel/smp.c
+++ b/arch/alpha/kernel/smp.c
@@ -562,7 +562,7 @@ handle_ipi(struct pt_regs *regs)
}
void
-smp_send_reschedule(int cpu)
+arch_smp_send_reschedule(int cpu)
{
#ifdef DEBUG_IPI_MSG
if (cpu == hard_smp_processor_id())
diff --git a/arch/arc/kernel/smp.c b/arch/arc/kernel/smp.c
index ab9e75e90f72..ae2e6a312361 100644
--- a/arch/arc/kernel/smp.c
+++ b/arch/arc/kernel/smp.c
@@ -292,7 +292,7 @@ static void ipi_send_msg(const struct cpumask *callmap,
enum ipi_msg_type msg)
ipi_send_msg_one(cpu, msg);
}
-void smp_send_reschedule(int cpu)
+void arch_smp_send_reschedule(int cpu)
{
ipi_send_msg_one(cpu, IPI_RESCHEDULE);
}
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 3b280d55c1c4..f216ac890b6f 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -745,7 +745,7 @@ void __init set_smp_ipi_range(int ipi_base, int n)
ipi_setup(smp_processor_id());
}
-void smp_send_reschedule(int cpu)
+void arch_smp_send_reschedule(int cpu)
{
smp_cross_call(cpumask_of(cpu), IPI_RESCHEDULE);
}
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 937d2623e06b..8d108edc4a89 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -976,7 +976,7 @@ void __init set_smp_ipi_range(int ipi_base, int n)
ipi_setup(smp_processor_id());
}
-void smp_send_reschedule(int cpu)
+void arch_smp_send_reschedule(int cpu)
{
smp_cross_call(cpumask_of(cpu), IPI_RESCHEDULE);
}
diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
index 4b605aa2e1d6..fd7f81be16dd 100644
--- a/arch/csky/kernel/smp.c
+++ b/arch/csky/kernel/smp.c
@@ -140,7 +140,7 @@ void smp_send_stop(void)
on_each_cpu(ipi_stop, NULL, 1);
}
-void smp_send_reschedule(int cpu)
+void arch_smp_send_reschedule(int cpu)
{
send_ipi_message(cpumask_of(cpu), IPI_RESCHEDULE);
}
diff --git a/arch/hexagon/kernel/smp.c b/arch/hexagon/kernel/smp.c
index 4ba93e59370c..4e8bee25b8c6 100644
--- a/arch/hexagon/kernel/smp.c
+++ b/arch/hexagon/kernel/smp.c
@@ -217,7 +217,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
}
}
-void smp_send_reschedule(int cpu)
+void arch_smp_send_reschedule(int cpu)
{
send_ipi(cpumask_of(cpu), IPI_RESCHEDULE);
}
diff --git a/arch/ia64/kernel/smp.c b/arch/ia64/kernel/smp.c
index e2cc59db86bc..ea4f009a232b 100644
--- a/arch/ia64/kernel/smp.c
+++ b/arch/ia64/kernel/smp.c
@@ -220,11 +220,11 @@ kdump_smp_send_init(void)
* Called with preemption disabled.
*/
void
-smp_send_reschedule (int cpu)
+arch_smp_send_reschedule (int cpu)
{
ia64_send_ipi(cpu, IA64_IPI_RESCHEDULE, IA64_IPI_DM_INT, 0);
}
-EXPORT_SYMBOL_GPL(smp_send_reschedule);
+EXPORT_SYMBOL_GPL(arch_smp_send_reschedule);
/*
* Called with preemption disabled.
diff --git a/arch/loongarch/include/asm/smp.h b/arch/loongarch/include/asm/smp.h
index 71189b28bfb2..3fcca134dfb1 100644
--- a/arch/loongarch/include/asm/smp.h
+++ b/arch/loongarch/include/asm/smp.h
@@ -83,7 +83,7 @@ extern void show_ipi_list(struct seq_file *p, int prec);
* it goes straight through and wastes no time serializing
* anything. Worst case is that we lose a reschedule ...
*/
-static inline void smp_send_reschedule(int cpu)
+static inline void arch_smp_send_reschedule(int cpu)
{
loongson3_send_ipi_single(cpu, SMP_RESCHEDULE);
}
diff --git a/arch/mips/include/asm/smp.h b/arch/mips/include/asm/smp.h
index 5d9ff61004ca..9806e79895d9 100644
--- a/arch/mips/include/asm/smp.h
+++ b/arch/mips/include/asm/smp.h
@@ -66,7 +66,7 @@ extern void calculate_cpu_foreign_map(void);
* it goes straight through and wastes no time serializing
* anything. Worst case is that we lose a
[RFC PATCH 4/5] irq_work: Trace calls to arch_irq_work_raise()
Adding a tracepoint to send_call_function_single_ipi() covers
irq_work_queue_on() when the CPU isn't the local one - add a tracepoint to
irq_work_raise() to cover the other cases.
Signed-off-by: Valentin Schneider
---
kernel/irq_work.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/kernel/irq_work.c b/kernel/irq_work.c
index 7afa40fe5cc4..5a550b681878 100644
--- a/kernel/irq_work.c
+++ b/kernel/irq_work.c
@@ -22,6 +22,8 @@
#include
#include
+#include
+
static DEFINE_PER_CPU(struct llist_head, raised_list);
static DEFINE_PER_CPU(struct llist_head, lazy_list);
static DEFINE_PER_CPU(struct task_struct *, irq_workd);
@@ -74,6 +76,14 @@ void __weak arch_irq_work_raise(void)
*/
}
+static inline void irq_work_raise(void)
+{
+ if (arch_irq_work_has_interrupt())
+ trace_ipi_send_cpu(_RET_IP_, smp_processor_id());
+
+ arch_irq_work_raise();
+}
+
/* Enqueue on current CPU, work must already be claimed and preempt disabled */
static void __irq_work_queue_local(struct irq_work *work)
{
@@ -99,7 +109,7 @@ static void __irq_work_queue_local(struct irq_work *work)
/* If the work is "lazy", handle it from next tick if any */
if (!lazy_work || tick_nohz_tick_stopped())
- arch_irq_work_raise();
+ irq_work_raise();
}
/* Enqueue the irq work @work on the current CPU */
--
2.31.1
[RFC PATCH 3/5] smp: Add a multi-CPU variant to send_call_function_single_ipi()
This simply wraps around the arch function and prepends it with a
tracepoint, bringing parity with send_call_function_single_ipi().
Signed-off-by: Valentin Schneider
---
kernel/smp.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/kernel/smp.c b/kernel/smp.c
index 7a7a22d69972..387735180aed 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -160,6 +160,12 @@ void __init call_function_init(void)
smpcfd_prepare_cpu(smp_processor_id());
}
+static inline void send_call_function_ipi_mask(const struct cpumask *mask)
+{
+ trace_ipi_send_cpumask(_RET_IP_, mask);
+ arch_send_call_function_ipi_mask(mask);
+}
+
#ifdef CONFIG_CSD_LOCK_WAIT_DEBUG
static DEFINE_STATIC_KEY_FALSE(csdlock_debug_enabled);
@@ -970,7 +976,7 @@ static void smp_call_function_many_cond(const struct
cpumask *mask,
if (nr_cpus == 1)
send_call_function_single_ipi(last_cpu);
else if (likely(nr_cpus > 1))
- arch_send_call_function_ipi_mask(cfd->cpumask_ipi);
+ send_call_function_ipi_mask(cfd->cpumask_ipi);
cfd_seq_store(this_cpu_ptr(_seq_local)->pinged, this_cpu,
CFD_SEQ_NOCPU, CFD_SEQ_PINGED);
}
--
2.31.1
[RFC PATCH 2/5] sched, smp: Trace send_call_function_single_ipi()
send_call_function_single_ipi() is the thing that sends IPIs at the bottom
of smp_call_function*() via either generic_exec_single() or
smp_call_function_many_cond(). Give it an IPI-related tracepoint.
Note that this ends up tracing any IPI sent via __smp_call_single_queue(),
which covers __ttwu_queue_wakelist() and irq_work_queue_on() "for free".
Signed-off-by: Valentin Schneider
---
arch/arm/kernel/smp.c | 3 ---
arch/arm64/kernel/smp.c | 1 -
kernel/sched/core.c | 7 +--
kernel/smp.c| 4
4 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 978db2d96b44..3b280d55c1c4 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -48,9 +48,6 @@
#include
#include
-#define CREATE_TRACE_POINTS
-#include
-
/*
* as from 2.5, kernels no longer have an init_tasks structure
* so we need some other way of telling a new secondary core
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index ffc5d76cf695..937d2623e06b 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -51,7 +51,6 @@
#include
#include
-#define CREATE_TRACE_POINTS
#include
DEFINE_PER_CPU_READ_MOSTLY(int, cpu_number);
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 60fdc0faf1c9..14e5e137172f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -81,6 +81,7 @@
#include
#include
#undef CREATE_TRACE_POINTS
+#include
#include "sched.h"
#include "stats.h"
@@ -3753,10 +3754,12 @@ void send_call_function_single_ipi(int cpu)
{
struct rq *rq = cpu_rq(cpu);
- if (!set_nr_if_polling(rq->idle))
+ if (!set_nr_if_polling(rq->idle)) {
+ trace_ipi_send_cpu(_RET_IP_, cpu);
arch_send_call_function_single_ipi(cpu);
- else
+ } else {
trace_sched_wake_idle_without_ipi(cpu);
+ }
}
/*
diff --git a/kernel/smp.c b/kernel/smp.c
index e8cdc025a046..7a7a22d69972 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -26,6 +26,10 @@
#include
#include
+#define CREATE_TRACE_POINTS
+#include
+#undef CREATE_TRACE_POINTS
+
#include "smpboot.h"
#include "sched/smp.h"
--
2.31.1
[RFC PATCH 1/5] trace: Add trace_ipi_send_{cpu, cpumask}
trace_ipi_raise is unsuitable for generically tracing IPI sources; add a
variant of it that takes a callsite and a CPU. Define a macro helper for
handling IPIs sent to multiple CPUs.
Signed-off-by: Valentin Schneider
---
include/trace/events/ipi.h | 27 +++
1 file changed, 27 insertions(+)
diff --git a/include/trace/events/ipi.h b/include/trace/events/ipi.h
index 0be71dad6ec0..fd2f2aeb36fe 100644
--- a/include/trace/events/ipi.h
+++ b/include/trace/events/ipi.h
@@ -35,6 +35,33 @@ TRACE_EVENT(ipi_raise,
TP_printk("target_mask=%s (%s)", __get_bitmask(target_cpus),
__entry->reason)
);
+TRACE_EVENT(ipi_send_cpu,
+
+ TP_PROTO(unsigned long callsite, unsigned int cpu),
+
+ TP_ARGS(callsite, cpu),
+
+ TP_STRUCT__entry(
+ __field(unsigned long, callsite)
+ __field(unsigned int, cpu)
+ ),
+
+ TP_fast_assign(
+ __entry->callsite = callsite;
+ __entry->cpu = cpu;
+ ),
+
+ TP_printk("callsite=%pS target_cpu=%d", (void *)__entry->callsite,
__entry->cpu)
+);
+
+#define trace_ipi_send_cpumask(callsite, mask) do {\
+ if (static_key_false(&__tracepoint_ipi_send_cpu.key)) { \
+ int cpu;\
+ for_each_cpu(cpu, mask) \
+ trace_ipi_send_cpu(callsite, cpu); \
+ } \
+} while (0)
+
DECLARE_EVENT_CLASS(ipi_handler,
TP_PROTO(const char *reason),
--
2.31.1
[RFC PATCH 0/5] Generic IPI sending tracepoint
Background
==
Detecting IPI *reception* is relatively easy, e.g. using
trace_irq_handler_{entry,exit} or even just function-trace
flush_smp_call_function_queue() for SMP calls.
Figuring out their *origin*, is trickier as there is no generic tracepoint tied
to e.g. smp_call_function():
o AFAIA x86 has no tracepoint tied to sending IPIs, only receiving them
(cf. trace_call_function{_single}_entry()).
o arm/arm64 do have trace_ipi_raise(), which gives us the target cpus but also a
mostly useless string (smp_calls will all be "Function call interrupts").
o Other architectures don't seem to have any IPI-sending related tracepoint.
I believe one reason those tracepoints used by arm/arm64 ended up as they were
is because these archs used to handle IPIs differently from regular interrupts
(the IRQ driver would directly invoke an IPI-handling routine), which meant
they
never showed up in trace_irq_handler_{entry, exit}. The trace_ipi_{entry,exit}
tracepoints gave a way to trace IPI reception but those have become redundant as
of:
56afcd3dbd19 ("ARM: Allow IPIs to be handled as normal interrupts")
d3afc7f12987 ("arm64: Allow IPIs to be handled as normal interrupts")
which gave IPIs a "proper" handler function used through
generic_handle_domain_irq(), which makes them show up via
trace_irq_handler_{entry, exit}.
Changing stuff up
=
Per the above, it would make sense to reshuffle trace_ipi_raise() and move it
into generic code. This also came up during Daniel's talk on Osnoise at the CPU
isolation MC of LPC 2022 [1].
Now, to be useful, such a tracepoint needs to export:
o targeted CPU(s)
o calling context
The only way to get the calling context with trace_ipi_raise() is to trigger a
stack dump, e.g. $(trace-cmd -e ipi* -T echo 42).
As for the targeted CPUs, the existing tracepoint does export them, albeit in
cpumask form, which is quite inconvenient from a tooling perspective. For
instance, as far as I'm aware, it's not possible to do event filtering on a
cpumask via trace-cmd.
Because of the above points, this is introducing a new tracepoint.
Patches
===
This results in having trace events for:
o smp_call_function*()
o smp_send_reschedule()
o irq_work_queue*()
This is incomplete, just looking at arm64 there's more IPI types that aren't
covered:
IPI_CPU_STOP,
IPI_CPU_CRASH_STOP,
IPI_TIMER,
IPI_WAKEUP,
... But it feels like a good starting point.
Another thing worth mentioning is that depending on the callsite, the _RET_IP_
fed to the tracepoint is not always useful - generic_exec_single() doesn't tell
you much about the actual callback being sent via IPI, so there might be value
in exploding the single tracepoint into at least one variant for smp_calls.
Links
=
[1]: https://youtu.be/5gT57y4OzBM?t=14234
Valentin Schneider (5):
trace: Add trace_ipi_send_{cpu, cpumask}
sched, smp: Trace send_call_function_single_ipi()
smp: Add a multi-CPU variant to send_call_function_single_ipi()
irq_work: Trace calls to arch_irq_work_raise()
treewide: Rename and trace arch-definitions of smp_send_reschedule()
arch/alpha/kernel/smp.c | 2 +-
arch/arc/kernel/smp.c| 2 +-
arch/arm/kernel/smp.c| 5 +
arch/arm64/kernel/smp.c | 3 +--
arch/csky/kernel/smp.c | 2 +-
arch/hexagon/kernel/smp.c| 2 +-
arch/ia64/kernel/smp.c | 4 ++--
arch/loongarch/include/asm/smp.h | 2 +-
arch/mips/include/asm/smp.h | 2 +-
arch/openrisc/kernel/smp.c | 2 +-
arch/parisc/kernel/smp.c | 4 ++--
arch/powerpc/kernel/smp.c| 4 ++--
arch/riscv/kernel/smp.c | 4 ++--
arch/s390/kernel/smp.c | 2 +-
arch/sh/kernel/smp.c | 2 +-
arch/sparc/kernel/smp_32.c | 2 +-
arch/sparc/kernel/smp_64.c | 2 +-
arch/x86/include/asm/smp.h | 2 +-
arch/xtensa/kernel/smp.c | 2 +-
include/linux/smp.h | 1 +
include/trace/events/ipi.h | 27 +++
kernel/irq_work.c| 12 +++-
kernel/sched/core.c | 7 +--
kernel/smp.c | 18 +-
24 files changed, 84 insertions(+), 31 deletions(-)
--
2.31.1
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Fri, Oct 07, 2022 at 04:57:24AM +, Christophe Leroy wrote:
>
>
> Le 07/10/2022 à 01:36, Jason A. Donenfeld a écrit :
> > On 10/6/22, Christophe Leroy wrote:
> >>
> >>
> >> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
> >>>
> >>>
> >>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
> Hi Christophe,
>
> On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
> wrote:
> > Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> >> The prandom_u32() function has been a deprecated inline wrapper around
> >> get_random_u32() for several releases now, and compiles down to the
> >> exact same code. Replace the deprecated wrapper with a direct call to
> >> the real function. The same also applies to get_random_int(), which is
> >> just a wrapper around get_random_u32().
> >>
> >> Reviewed-by: Kees Cook
> >> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> >> Acked-by: Chuck Lever # for nfsd
> >> Reviewed-by: Jan Kara # for ext4
> >> Signed-off-by: Jason A. Donenfeld
> >> ---
> >
> >> diff --git a/arch/powerpc/kernel/process.c
> >> b/arch/powerpc/kernel/process.c
> >> index 0fbda89cd1bb..9c4c15afbbe8 100644
> >> --- a/arch/powerpc/kernel/process.c
> >> +++ b/arch/powerpc/kernel/process.c
> >> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
> >> unsigned long arch_align_stack(unsigned long sp)
> >> {
> >> if (!(current->personality & ADDR_NO_RANDOMIZE) &&
> >> randomize_va_space)
> >> - sp -= get_random_int() & ~PAGE_MASK;
> >> + sp -= get_random_u32() & ~PAGE_MASK;
> >> return sp & ~0xf;
> >
> > Isn't that a candidate for prandom_u32_max() ?
> >
> > Note that sp is deemed to be 16 bytes aligned at all time.
>
> Yes, probably. It seemed non-trivial to think about, so I didn't. But
> let's see here... maybe it's not too bad:
>
> If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
> (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
> thing? Is that accurate? And holds across platforms (this comes up a
> few places)? If so, I'll do that for a v4.
>
> >>>
> >>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
> >>>
> >>> /*
> >>>* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
> >>>* assign PAGE_MASK to a larger type it gets extended the way we want
> >>>* (i.e. with 1s in the high bits)
> >>>*/
> >>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
> >>>
> >>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
> >>>
> >>>
> >>> So it would work I guess.
> >>
> >> But taking into account that sp must remain 16 bytes aligned, would it
> >> be better to do something like ?
> >>
> >>sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
> >>
> >>return sp;
> >
> > Does this assume that sp is already aligned at the beginning of the
> > function? I'd assume from the function's name that this isn't the
> > case?
>
> Ah you are right, I overlooked it.
So I think to stay on the safe side, I'm going to go with
`prandom_u32_max(PAGE_SIZE)`. Sound good?
Jason
Re: [RFC PATCH 5/5] treewide: Rename and trace arch-definitions of smp_send_reschedule()
On Fri, Oct 7, 2022 at 11:46 PM Valentin Schneider wrote:
>
> To be able to trace invocations of smp_send_reschedule(), rename the
> arch-specific definitions of it to arch_smp_send_reschedule() and wrap it
> into an smp_send_reschedule() that contains a tracepoint.
>
> Signed-off-by: Valentin Schneider
> ---
> arch/alpha/kernel/smp.c | 2 +-
> arch/arc/kernel/smp.c| 2 +-
> arch/arm/kernel/smp.c| 2 +-
> arch/arm64/kernel/smp.c | 2 +-
> arch/csky/kernel/smp.c | 2 +-
> arch/hexagon/kernel/smp.c| 2 +-
> arch/ia64/kernel/smp.c | 4 ++--
> arch/loongarch/include/asm/smp.h | 2 +-
> arch/mips/include/asm/smp.h | 2 +-
> arch/openrisc/kernel/smp.c | 2 +-
> arch/parisc/kernel/smp.c | 4 ++--
> arch/powerpc/kernel/smp.c| 4 ++--
> arch/riscv/kernel/smp.c | 4 ++--
> arch/s390/kernel/smp.c | 2 +-
> arch/sh/kernel/smp.c | 2 +-
> arch/sparc/kernel/smp_32.c | 2 +-
> arch/sparc/kernel/smp_64.c | 2 +-
> arch/x86/include/asm/smp.h | 2 +-
> arch/xtensa/kernel/smp.c | 2 +-
> include/linux/smp.h | 1 +
> kernel/smp.c | 6 ++
> 21 files changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
> index f4e20f75438f..38637eb9eebd 100644
> --- a/arch/alpha/kernel/smp.c
> +++ b/arch/alpha/kernel/smp.c
> @@ -562,7 +562,7 @@ handle_ipi(struct pt_regs *regs)
> }
>
> void
> -smp_send_reschedule(int cpu)
> +arch_smp_send_reschedule(int cpu)
> {
> #ifdef DEBUG_IPI_MSG
> if (cpu == hard_smp_processor_id())
> diff --git a/arch/arc/kernel/smp.c b/arch/arc/kernel/smp.c
> index ab9e75e90f72..ae2e6a312361 100644
> --- a/arch/arc/kernel/smp.c
> +++ b/arch/arc/kernel/smp.c
> @@ -292,7 +292,7 @@ static void ipi_send_msg(const struct cpumask *callmap,
> enum ipi_msg_type msg)
> ipi_send_msg_one(cpu, msg);
> }
>
> -void smp_send_reschedule(int cpu)
> +void arch_smp_send_reschedule(int cpu)
> {
> ipi_send_msg_one(cpu, IPI_RESCHEDULE);
> }
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 3b280d55c1c4..f216ac890b6f 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -745,7 +745,7 @@ void __init set_smp_ipi_range(int ipi_base, int n)
> ipi_setup(smp_processor_id());
> }
>
> -void smp_send_reschedule(int cpu)
> +void arch_smp_send_reschedule(int cpu)
> {
> smp_cross_call(cpumask_of(cpu), IPI_RESCHEDULE);
> }
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 937d2623e06b..8d108edc4a89 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -976,7 +976,7 @@ void __init set_smp_ipi_range(int ipi_base, int n)
> ipi_setup(smp_processor_id());
> }
>
> -void smp_send_reschedule(int cpu)
> +void arch_smp_send_reschedule(int cpu)
> {
> smp_cross_call(cpumask_of(cpu), IPI_RESCHEDULE);
> }
> diff --git a/arch/csky/kernel/smp.c b/arch/csky/kernel/smp.c
> index 4b605aa2e1d6..fd7f81be16dd 100644
> --- a/arch/csky/kernel/smp.c
> +++ b/arch/csky/kernel/smp.c
> @@ -140,7 +140,7 @@ void smp_send_stop(void)
> on_each_cpu(ipi_stop, NULL, 1);
> }
>
> -void smp_send_reschedule(int cpu)
> +void arch_smp_send_reschedule(int cpu)
For csky part, Acked-by: Guo Ren
> {
> send_ipi_message(cpumask_of(cpu), IPI_RESCHEDULE);
> }
> diff --git a/arch/hexagon/kernel/smp.c b/arch/hexagon/kernel/smp.c
> index 4ba93e59370c..4e8bee25b8c6 100644
> --- a/arch/hexagon/kernel/smp.c
> +++ b/arch/hexagon/kernel/smp.c
> @@ -217,7 +217,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> }
> }
>
> -void smp_send_reschedule(int cpu)
> +void arch_smp_send_reschedule(int cpu)
> {
> send_ipi(cpumask_of(cpu), IPI_RESCHEDULE);
> }
> diff --git a/arch/ia64/kernel/smp.c b/arch/ia64/kernel/smp.c
> index e2cc59db86bc..ea4f009a232b 100644
> --- a/arch/ia64/kernel/smp.c
> +++ b/arch/ia64/kernel/smp.c
> @@ -220,11 +220,11 @@ kdump_smp_send_init(void)
> * Called with preemption disabled.
> */
> void
> -smp_send_reschedule (int cpu)
> +arch_smp_send_reschedule (int cpu)
> {
> ia64_send_ipi(cpu, IA64_IPI_RESCHEDULE, IA64_IPI_DM_INT, 0);
> }
> -EXPORT_SYMBOL_GPL(smp_send_reschedule);
> +EXPORT_SYMBOL_GPL(arch_smp_send_reschedule);
>
> /*
> * Called with preemption disabled.
> diff --git a/arch/loongarch/include/asm/smp.h
> b/arch/loongarch/include/asm/smp.h
> index 71189b28bfb2..3fcca134dfb1 100644
> --- a/arch/loongarch/include/asm/smp.h
> +++ b/arch/loongarch/include/asm/smp.h
> @@ -83,7 +83,7 @@ extern void show_ipi_list(struct seq_file *p, int prec);
> * it goes straight through and wastes no time serializing
> * anything. Worst case is that we lose a reschedule ...
> */
> -static inline void smp_send_reschedule(int cpu)
> +static inline void arch_smp_send_reschedule(int cpu)
> {
> loongson3_send_ipi_single(cpu,
RE: [PATCH v4 2/3] powerpc/pseries: PLPKS SED Opal keystore support
> -Original Message-
> From: gjo...@linux.vnet.ibm.com
> Sent: Friday, August 19, 2022 5:32 PM
> To: linux-bl...@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org; jonathan.derr...@linux.dev;
> brk...@linux.vnet.ibm.com; msucha...@suse.de; m...@ellerman.id.au;
> na...@linux.ibm.com; ax...@kernel.dk; a...@linux-foundation.org;
> gjo...@linux.vnet.ibm.com; linux-...@vger.kernel.org;
> keyri...@vger.kernel.org; dhowe...@redhat.com; jar...@kernel.org
> Subject: [PATCH v4 2/3] powerpc/pseries: PLPKS SED Opal keystore support
>
> +++ b/arch/powerpc/platforms/pseries/plpks_sed_ops.c
...
> +struct plpks_sed_object_data {
> + u_char version;
> + u_char pad1[7];
> + u_long authority;
> + u_long range;
> + u_int key_len;
> + u_char key[32];
> +};
...
> +/*
> + * Read the SED Opal key from PLPKS given the label
> + */
> +int sed_read_key(char *keyname, char *key, u_int *keylen)
> +{
...
> + *keylen = be32_to_cpu(data->key_len);
> +
> + if (var.data) {
> + memcpy(key, var.data + offset, var.datalen - offset);
> + key[*keylen] = '\0';
Is there a guarantee that key_len is always < sizeof key, or
does that need to be checked in more places?
Re: [RFC PATCH 0/5] Generic IPI sending tracepoint
Hi Valentin,
On Fri, Oct 07, 2022 at 04:41:40PM +0100, Valentin Schneider wrote:
> Background
> ==
>
> Detecting IPI *reception* is relatively easy, e.g. using
> trace_irq_handler_{entry,exit} or even just function-trace
> flush_smp_call_function_queue() for SMP calls.
>
> Figuring out their *origin*, is trickier as there is no generic tracepoint
> tied
> to e.g. smp_call_function():
>
> o AFAIA x86 has no tracepoint tied to sending IPIs, only receiving them
> (cf. trace_call_function{_single}_entry()).
> o arm/arm64 do have trace_ipi_raise(), which gives us the target cpus but
> also a
> mostly useless string (smp_calls will all be "Function call interrupts").
> o Other architectures don't seem to have any IPI-sending related tracepoint.
>
> I believe one reason those tracepoints used by arm/arm64 ended up as they were
> is because these archs used to handle IPIs differently from regular interrupts
> (the IRQ driver would directly invoke an IPI-handling routine), which meant
> they
> never showed up in trace_irq_handler_{entry, exit}. The trace_ipi_{entry,exit}
> tracepoints gave a way to trace IPI reception but those have become redundant
> as
> of:
>
> 56afcd3dbd19 ("ARM: Allow IPIs to be handled as normal interrupts")
> d3afc7f12987 ("arm64: Allow IPIs to be handled as normal interrupts")
>
> which gave IPIs a "proper" handler function used through
> generic_handle_domain_irq(), which makes them show up via
> trace_irq_handler_{entry, exit}.
>
> Changing stuff up
> =
>
> Per the above, it would make sense to reshuffle trace_ipi_raise() and move it
> into generic code. This also came up during Daniel's talk on Osnoise at the
> CPU
> isolation MC of LPC 2022 [1].
>
> Now, to be useful, such a tracepoint needs to export:
> o targeted CPU(s)
> o calling context
>
> The only way to get the calling context with trace_ipi_raise() is to trigger a
> stack dump, e.g. $(trace-cmd -e ipi* -T echo 42).
>
> As for the targeted CPUs, the existing tracepoint does export them, albeit in
> cpumask form, which is quite inconvenient from a tooling perspective. For
> instance, as far as I'm aware, it's not possible to do event filtering on a
> cpumask via trace-cmd.
https://man7.org/linux/man-pages/man1/trace-cmd-set.1.html
-f filter
Specify a filter for the previous event. This must come after
a -e. This will filter what events get recorded based on the
content of the event. Filtering is passed to the kernel
directly so what filtering is allowed may depend on what
version of the kernel you have. Basically, it will let you
use C notation to check if an event should be processed or
not.
==, >=, <=, >, <, &, |, && and ||
The above are usually safe to use to compare fields.
This looks overkill to me (consider large number of bits set in mask).
+#define trace_ipi_send_cpumask(callsite, mask) do {\
+ if (static_key_false(&__tracepoint_ipi_send_cpu.key)) { \
+ int cpu;\
+ for_each_cpu(cpu, mask) \
+ trace_ipi_send_cpu(callsite, cpu); \
+ } \
+} while (0)
>
> Because of the above points, this is introducing a new tracepoint.
>
> Patches
> ===
>
> This results in having trace events for:
>
> o smp_call_function*()
> o smp_send_reschedule()
> o irq_work_queue*()
>
> This is incomplete, just looking at arm64 there's more IPI types that aren't
> covered:
>
> IPI_CPU_STOP,
> IPI_CPU_CRASH_STOP,
> IPI_TIMER,
> IPI_WAKEUP,
>
> ... But it feels like a good starting point.
Can't you have a single tracepoint (or variant with cpumask) that would
cover such cases as well?
Maybe (as parameters for tracepoint):
* type (reschedule, smp_call_function, timer, wakeup, ...).
* function address: valid for smp_call_function, irq_work_queue
types.
> Another thing worth mentioning is that depending on the callsite, the _RET_IP_
> fed to the tracepoint is not always useful - generic_exec_single() doesn't
> tell
> you much about the actual callback being sent via IPI, so there might be value
> in exploding the single tracepoint into at least one variant for smp_calls.
Not sure i grasp what you mean by "exploding the single tracepoint...",
but yes knowing the function or irq work function is very useful.
>
> Links
> =
>
> [1]: https://youtu.be/5gT57y4OzBM?t=14234
>
> Valentin Schneider (5):
> trace: Add trace_ipi_send_{cpu, cpumask}
> sched, smp: Trace send_call_function_single_ipi()
> smp: Add a multi-CPU variant to send_call_function_single_ipi()
> irq_work: Trace calls to arch_irq_work_raise()
> treewide: Rename and trace arch-definitions of smp_send_reschedule()
>
> arch/alpha/kernel/smp.c | 2 +-
Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option
On Fri, Oct 07, 2022 at 10:03:38AM +1000, Nicholas Piggin wrote:
> On Fri Oct 7, 2022 at 9:23 AM AEST, Segher Boessenkool wrote:
> > > For pcrel addressing? Bootstrapping the C environment is one, the
> > > module dynamic linker is another.
> >
> > I don't know what either of those mean.
>
> arch/powerpc/kernel/head_64.S and arch/powerpc/kernel/module_64.c
>
> Can discuss in the pcrel patch series thread if you would like to know
> more.
So "bootstrapping the C environment" is meant to mean "initialising it",
like *rt*.o ("C runtime") does normally?
And "module dynamic linker" is "module loader" here?
Yes, those things probably need some attention for pcrel, but
"bootstrapping" and "dynamic" had me scratch my head: there is nothing
that pulls itself up by its bootstraps (like, the initialisation itself
would be done in C code), and module loading is much closer to static
loading than to dynamic loading :-)
> > Just say in a comment where you disable stuff that it is to prevent
> > possible problems, this is a WIP, that kind of thing? Otherwise other
> > people (like me :-) ) will read it and think there must be some deeper
> > reason. Like, changing code to work with pcrel is hard or a lot of
> > work -- it isn't :-) As you say in 0/7 yourself btw!
>
> I will describe limitations and issues a bit more in changelog of patches
> to enable prefix and pcrel when I submit as non-RFC candidate. It would
> probably not be too hard to get things to a workable state that could be
> merged.
Looking forward to it!
> > VMX and VSX are disabled here because the compiler *will* use those
> > registers if it feels like it (that is, if it thinks that will be
> > faster). MMA is a very different beast: the compiler can never know if
> > it will be faster, to start with.
>
> True, but now I don't have to find the exact clause and have my lawyer
> confirm that it definitely probably won't change in future and break
> things.
Your lawyer won't be able to tell you, but I can. And I did already.
The reason I care about these things is that very often people look at
what the kernel does as a "best practices" example. And then copy this
stuff as some cargo cult incantations :-/
Segher
Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option
On Fri, Oct 07, 2022 at 04:31:28PM +1100, Michael Ellerman wrote: > "Nicholas Piggin" writes: > > On Fri Oct 7, 2022 at 9:23 AM AEST, Segher Boessenkool wrote: > >> On Fri, Oct 07, 2022 at 07:56:09AM +1000, Nicholas Piggin wrote: > >> > On Fri Oct 7, 2022 at 5:54 AM AEST, Segher Boessenkool wrote: > >> > > On Fri, Sep 23, 2022 at 01:30:04PM +1000, Nicholas Piggin wrote: > ... > >> > > > +# No AltiVec or VSX or MMA instructions when building kernel > >> > > > KBUILD_CFLAGS += $(call cc-option,-mno-altivec) > >> > > > KBUILD_CFLAGS += $(call cc-option,-mno-vsx) > >> > > > +KBUILD_CFLAGS += $(call cc-option,-mno-mma) > >> > > > >> > > MMA code is never generated unless the code asks for it explicitly. > >> > > This is fundamental, not just an implementations side effect. > >> > > >> > Well, now it double won't be generated :) > >> > >> Yeah, but there are many other things you can unnecessarily disable as > >> well! :-) > >> > >> VMX and VSX are disabled here because the compiler *will* use those > >> registers if it feels like it (that is, if it thinks that will be > >> faster). MMA is a very different beast: the compiler can never know if > >> it will be faster, to start with. > > > > True, but now I don't have to find the exact clause and have my lawyer > > confirm that it definitely probably won't change in future and break > > things. > > Right. If someone asks "does the kernel ever use MMA instructions?" we > can just point at that line and we have a definite answer. No need to > audit the behaviour of all GCC and Clang versions ever released. As I said, no sane compiler can use MMA ever (unless asked for it directly of course). But yeah, who knows what clang does! Segher
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 10:53:44AM -0600, Jason A. Donenfeld wrote: > drivers/thunderbolt/xdomain.c | 2 +- Acked-by: Mika Westerberg
RE: [PATCH v3 3/5] treewide: use get_random_u32() when possible
From: Christophe Leroy > Sent: 06 October 2022 18:43 ... > But taking into account that sp must remain 16 bytes aligned, would it > be better to do something like ? > > sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4; That makes me think... If prandom_u32_max() is passed a (constant) power of 2 it doesn't need to do the multiply, it can just do a shift right. Doesn't it also always get a 32bit random value? So actually get_random_u32() & PAGE_MASK & ~0xf is faster! When PAGE_SIZE is 4k, PAGE_SIZE >> 4 is 256 so it could use: get_ramdom_u8() << 4 You also seem to have removed prandom_u32() in favour of get_random_u32() but have added more prandom_() functions. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH v3 0/5] treewide cleanup of random integer usage
On Thu, 6 Oct 2022 at 18:54, Jason A. Donenfeld wrote: > > Changes v2->v3: > - Handle get_random_int() conversions too, which were overlooked before, > in the same way as the rest. > > Hi folks, > > This is a five part treewide cleanup of random integer handling. The > rules for random integers are: > > - If you want a secure or an insecure random u64, use get_random_u64(). > - If you want a secure or an insecure random u32, use get_random_u32(). > * The old function prandom_u32() has been deprecated for a while now > and is just a wrapper around get_random_u32(). Same for > get_random_int(). > - If you want a secure or an insecure random u16, use get_random_u16(). > - If you want a secure or an insecure random u8, use get_random_u8(). > - If you want secure or insecure random bytes, use get_random_bytes(). > * The old function prandom_bytes() has been deprecated for a while now > and has long been a wrapper around get_random_bytes(). > - If you want a non-uniform random u32, u16, or u8 bounded by a certain > open interval maximum, use prandom_u32_max(). > * I say "non-uniform", because it doesn't do any rejection sampling or > divisions. Hence, it stays within the prandom_* namespace. > > These rules ought to be applied uniformly, so that we can clean up the > deprecated functions, and earn the benefits of using the modern > functions. In particular, in addition to the boring substitutions, this > patchset accomplishes a few nice effects: > > - By using prandom_u32_max() with an upper-bound that the compiler can > prove at compile-time is ≤65536 or ≤256, internally get_random_u16() > or get_random_u8() is used, which wastes fewer batched random bytes, > and hence has higher throughput. > > - By using prandom_u32_max() instead of %, when the upper-bound is not a > constant, division is still avoided, because prandom_u32_max() uses > a faster multiplication-based trick instead. > > - By using get_random_u16() or get_random_u8() in cases where the return > value is intended to indeed be a u16 or a u8, we waste fewer batched > random bytes, and hence have higher throughput. > > So, based on those rules and benefits from following them, this patchset > breaks down into the following five steps, which were done mostly > manually: > > 1) Replace `prandom_u32() % max` and variants thereof with >prandom_u32_max(max). > > 2) Replace `(type)get_random_u32()` and variants thereof with >get_random_u16() or get_random_u8(). I took the pains to actually >look and see what every lvalue type was across the entire tree. > > 3) Replace remaining deprecated uses of prandom_u32() and >get_random_int() with get_random_u32(). > > 4) Replace remaining deprecated uses of prandom_bytes() with >get_random_bytes(). > > 5) Remove the deprecated and now-unused prandom_u32() and >prandom_bytes() inline wrapper functions. > > I was thinking of taking this through my random.git tree (on which this > series is currently based) and submitting it near the end of the merge > window, or waiting for the very end of the 6.1 cycle when there will be > the fewest new patches brewing. If somebody with some treewide-cleanup > experience might share some wisdom about what the best timing usually > winds up being, I'm all ears. > > Please take a look! At "379 insertions(+), 422 deletions(-)", this > should be a somewhat small patchset to review, despite it having the > scary "treewide" moniker on it. > > Thanks, > Jason > > Cc: Andreas Noever > Cc: Andrew Morton > Cc: Andy Shevchenko > Cc: Borislav Petkov > Cc: Catalin Marinas > Cc: Christoph Böhmwalder > Cc: Christoph Hellwig > Cc: Christophe Leroy > Cc: Daniel Borkmann > Cc: Dave Airlie > Cc: Dave Hansen > Cc: David S. Miller > Cc: Eric Dumazet > Cc: Florian Westphal > Cc: Greg Kroah-Hartman , > Cc: H. Peter Anvin > Cc: Heiko Carstens > Cc: Helge Deller > Cc: Herbert Xu > Cc: Huacai Chen > Cc: Hugh Dickins > Cc: Jakub Kicinski > Cc: James E.J. Bottomley > Cc: Jan Kara > Cc: Jason Gunthorpe > Cc: Jens Axboe > Cc: Johannes Berg > Cc: Jonathan Corbet > Cc: Jozsef Kadlecsik > Cc: KP Singh > Cc: Kees Cook > Cc: Marco Elver > Cc: Mauro Carvalho Chehab > Cc: Michael Ellerman > Cc: Pablo Neira Ayuso > Cc: Paolo Abeni > Cc: Peter Zijlstra > Cc: Richard Weinberger > Cc: Russell King > Cc: Theodore Ts'o > Cc: Thomas Bogendoerfer > Cc: Thomas Gleixner > Cc: Thomas Graf > Cc: Ulf Hansson > Cc: Vignesh Raghavendra > Cc: WANG Xuerui > Cc: Will Deacon > Cc: Yury Norov > Cc: dri-de...@lists.freedesktop.org > Cc: kasan-...@googlegroups.com > Cc: kernel-janit...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-bl...@vger.kernel.org > Cc: linux-cry...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: linux-fsde...@vger.kernel.org > Cc: linux-me...@vger.kernel.org > Cc: linux-m...@vger.kernel.org > Cc: linux...@kvack.org > Cc: linux-...@vger.kernel.org > Cc:
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
Le 07/10/2022 à 01:36, Jason A. Donenfeld a écrit :
> On 10/6/22, Christophe Leroy wrote:
>>
>>
>> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
>>>
>>>
>>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
Hi Christophe,
On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
wrote:
> Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
>> The prandom_u32() function has been a deprecated inline wrapper around
>> get_random_u32() for several releases now, and compiles down to the
>> exact same code. Replace the deprecated wrapper with a direct call to
>> the real function. The same also applies to get_random_int(), which is
>> just a wrapper around get_random_u32().
>>
>> Reviewed-by: Kees Cook
>> Acked-by: Toke Høiland-Jørgensen # for sch_cake
>> Acked-by: Chuck Lever # for nfsd
>> Reviewed-by: Jan Kara # for ext4
>> Signed-off-by: Jason A. Donenfeld
>> ---
>
>> diff --git a/arch/powerpc/kernel/process.c
>> b/arch/powerpc/kernel/process.c
>> index 0fbda89cd1bb..9c4c15afbbe8 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
>> unsigned long arch_align_stack(unsigned long sp)
>> {
>> if (!(current->personality & ADDR_NO_RANDOMIZE) &&
>> randomize_va_space)
>> - sp -= get_random_int() & ~PAGE_MASK;
>> + sp -= get_random_u32() & ~PAGE_MASK;
>> return sp & ~0xf;
>
> Isn't that a candidate for prandom_u32_max() ?
>
> Note that sp is deemed to be 16 bytes aligned at all time.
Yes, probably. It seemed non-trivial to think about, so I didn't. But
let's see here... maybe it's not too bad:
If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
(PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
thing? Is that accurate? And holds across platforms (this comes up a
few places)? If so, I'll do that for a v4.
>>>
>>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
>>>
>>> /*
>>>* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
>>>* assign PAGE_MASK to a larger type it gets extended the way we want
>>>* (i.e. with 1s in the high bits)
>>>*/
>>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
>>>
>>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
>>>
>>>
>>> So it would work I guess.
>>
>> But taking into account that sp must remain 16 bytes aligned, would it
>> be better to do something like ?
>>
>> sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
>>
>> return sp;
>
> Does this assume that sp is already aligned at the beginning of the
> function? I'd assume from the function's name that this isn't the
> case?
Ah you are right, I overlooked it.
Looking in more details, I see that all architectures that implement it
implement it almost the same way.
By the way, the comment in arch/um/kernel/process.c is overdated.
Most architectures AND the random value with ~PAGE_MASK, x86 and um use
%8192. Seems like at the time 2.6.12 was introduced into git, only i386
x86_64 and um had that function.
Maybe it is time for a cleanup and a refactoring ? Architectures would
just have to provide STACK_ALIGN just like loongarch does today, and we
could get a generic arch_align_stack() ?
Christophe
Re: [PATCH v3 4/5] treewide: use get_random_bytes when possible
On 10/6/22, Bagas Sanjaya wrote:
> On 10/6/22 23:53, Jason A. Donenfeld wrote:
>> The prandom_bytes() function has been a deprecated inline wrapper around
>> get_random_bytes() for several releases now, and compiles down to the
>> exact same code. Replace the deprecated wrapper with a direct call to
>> the real function.
>>
>> Reviewed-by: Kees Cook
>> Signed-off-by: Jason A. Donenfeld
>> ---
>> arch/powerpc/crypto/crc-vpmsum_test.c | 2 +-
>> block/blk-crypto-fallback.c | 2 +-
>> crypto/async_tx/raid6test.c | 2 +-
>> drivers/dma/dmatest.c | 2 +-
>> drivers/mtd/nand/raw/nandsim.c | 2 +-
>> drivers/mtd/tests/mtd_nandecctest.c | 2 +-
>> drivers/mtd/tests/speedtest.c | 2 +-
>> drivers/mtd/tests/stresstest.c | 2 +-
>> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
>> drivers/net/ethernet/rocker/rocker_main.c | 2 +-
>> drivers/net/wireguard/selftest/allowedips.c | 12 ++--
>> fs/ubifs/debug.c| 2 +-
>> kernel/kcsan/selftest.c | 2 +-
>> lib/random32.c | 2 +-
>> lib/test_objagg.c | 2 +-
>> lib/uuid.c | 2 +-
>> net/ipv4/route.c| 2 +-
>> net/mac80211/rc80211_minstrel_ht.c | 2 +-
>> net/sched/sch_pie.c | 2 +-
>> 19 files changed, 24 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/powerpc/crypto/crc-vpmsum_test.c
>> b/arch/powerpc/crypto/crc-vpmsum_test.c
>> index c1c1ef9457fb..273c527868db 100644
>> --- a/arch/powerpc/crypto/crc-vpmsum_test.c
>> +++ b/arch/powerpc/crypto/crc-vpmsum_test.c
>> @@ -82,7 +82,7 @@ static int __init crc_test_init(void)
>>
>> if (len <= offset)
>> continue;
>> -prandom_bytes(data, len);
>> +get_random_bytes(data, len);
>> len -= offset;
>>
>> crypto_shash_update(crct10dif_shash, data+offset, len);
>> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
>> index 621abd1b0e4d..ad9844c5b40c 100644
>> --- a/block/blk-crypto-fallback.c
>> +++ b/block/blk-crypto-fallback.c
>> @@ -539,7 +539,7 @@ static int blk_crypto_fallback_init(void)
>> if (blk_crypto_fallback_inited)
>> return 0;
>>
>> -prandom_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
>> +get_random_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
>>
>> err = bioset_init(_bio_split, 64, 0, 0);
>> if (err)
>> diff --git a/crypto/async_tx/raid6test.c b/crypto/async_tx/raid6test.c
>> index c9d218e53bcb..f74505f2baf0 100644
>> --- a/crypto/async_tx/raid6test.c
>> +++ b/crypto/async_tx/raid6test.c
>> @@ -37,7 +37,7 @@ static void makedata(int disks)
>> int i;
>>
>> for (i = 0; i < disks; i++) {
>> -prandom_bytes(page_address(data[i]), PAGE_SIZE);
>> +get_random_bytes(page_address(data[i]), PAGE_SIZE);
>> dataptrs[i] = data[i];
>> dataoffs[i] = 0;
>> }
>> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
>> index 9fe2ae794316..ffe621695e47 100644
>> --- a/drivers/dma/dmatest.c
>> +++ b/drivers/dma/dmatest.c
>> @@ -312,7 +312,7 @@ static unsigned long dmatest_random(void)
>> {
>> unsigned long buf;
>>
>> -prandom_bytes(, sizeof(buf));
>> +get_random_bytes(, sizeof(buf));
>> return buf;
>> }
>>
>> diff --git a/drivers/mtd/nand/raw/nandsim.c
>> b/drivers/mtd/nand/raw/nandsim.c
>> index 4bdaf4aa7007..c941a5a41ea6 100644
>> --- a/drivers/mtd/nand/raw/nandsim.c
>> +++ b/drivers/mtd/nand/raw/nandsim.c
>> @@ -1393,7 +1393,7 @@ static int ns_do_read_error(struct nandsim *ns, int
>> num)
>> unsigned int page_no = ns->regs.row;
>>
>> if (ns_read_error(page_no)) {
>> -prandom_bytes(ns->buf.byte, num);
>> +get_random_bytes(ns->buf.byte, num);
>> NS_WARN("simulating read error in page %u\n", page_no);
>> return 1;
>> }
>> diff --git a/drivers/mtd/tests/mtd_nandecctest.c
>> b/drivers/mtd/tests/mtd_nandecctest.c
>> index 1c7201b0f372..440988562cfd 100644
>> --- a/drivers/mtd/tests/mtd_nandecctest.c
>> +++ b/drivers/mtd/tests/mtd_nandecctest.c
>> @@ -266,7 +266,7 @@ static int nand_ecc_test_run(const size_t size)
>> goto error;
>> }
>>
>> -prandom_bytes(correct_data, size);
>> +get_random_bytes(correct_data, size);
>> ecc_sw_hamming_calculate(correct_data, size, correct_ecc, sm_order);
>> for (i = 0; i < ARRAY_SIZE(nand_ecc_test); i++) {
>> nand_ecc_test[i].prepare(error_data, error_ecc,
>> diff --git a/drivers/mtd/tests/speedtest.c
>> b/drivers/mtd/tests/speedtest.c
>> index c9ec7086bfa1..075bce32caa5 100644
>> --- a/drivers/mtd/tests/speedtest.c
>> +++ b/drivers/mtd/tests/speedtest.c
>> @@ -223,7 +223,7 @@ static int
Re: [PATCH v3 4/5] treewide: use get_random_bytes when possible
On 10/6/22 23:53, Jason A. Donenfeld wrote:
> The prandom_bytes() function has been a deprecated inline wrapper around
> get_random_bytes() for several releases now, and compiles down to the
> exact same code. Replace the deprecated wrapper with a direct call to
> the real function.
>
> Reviewed-by: Kees Cook
> Signed-off-by: Jason A. Donenfeld
> ---
> arch/powerpc/crypto/crc-vpmsum_test.c | 2 +-
> block/blk-crypto-fallback.c | 2 +-
> crypto/async_tx/raid6test.c | 2 +-
> drivers/dma/dmatest.c | 2 +-
> drivers/mtd/nand/raw/nandsim.c | 2 +-
> drivers/mtd/tests/mtd_nandecctest.c | 2 +-
> drivers/mtd/tests/speedtest.c | 2 +-
> drivers/mtd/tests/stresstest.c | 2 +-
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
> drivers/net/ethernet/rocker/rocker_main.c | 2 +-
> drivers/net/wireguard/selftest/allowedips.c | 12 ++--
> fs/ubifs/debug.c| 2 +-
> kernel/kcsan/selftest.c | 2 +-
> lib/random32.c | 2 +-
> lib/test_objagg.c | 2 +-
> lib/uuid.c | 2 +-
> net/ipv4/route.c| 2 +-
> net/mac80211/rc80211_minstrel_ht.c | 2 +-
> net/sched/sch_pie.c | 2 +-
> 19 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/arch/powerpc/crypto/crc-vpmsum_test.c
> b/arch/powerpc/crypto/crc-vpmsum_test.c
> index c1c1ef9457fb..273c527868db 100644
> --- a/arch/powerpc/crypto/crc-vpmsum_test.c
> +++ b/arch/powerpc/crypto/crc-vpmsum_test.c
> @@ -82,7 +82,7 @@ static int __init crc_test_init(void)
>
> if (len <= offset)
> continue;
> - prandom_bytes(data, len);
> + get_random_bytes(data, len);
> len -= offset;
>
> crypto_shash_update(crct10dif_shash, data+offset, len);
> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
> index 621abd1b0e4d..ad9844c5b40c 100644
> --- a/block/blk-crypto-fallback.c
> +++ b/block/blk-crypto-fallback.c
> @@ -539,7 +539,7 @@ static int blk_crypto_fallback_init(void)
> if (blk_crypto_fallback_inited)
> return 0;
>
> - prandom_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
> + get_random_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
>
> err = bioset_init(_bio_split, 64, 0, 0);
> if (err)
> diff --git a/crypto/async_tx/raid6test.c b/crypto/async_tx/raid6test.c
> index c9d218e53bcb..f74505f2baf0 100644
> --- a/crypto/async_tx/raid6test.c
> +++ b/crypto/async_tx/raid6test.c
> @@ -37,7 +37,7 @@ static void makedata(int disks)
> int i;
>
> for (i = 0; i < disks; i++) {
> - prandom_bytes(page_address(data[i]), PAGE_SIZE);
> + get_random_bytes(page_address(data[i]), PAGE_SIZE);
> dataptrs[i] = data[i];
> dataoffs[i] = 0;
> }
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index 9fe2ae794316..ffe621695e47 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -312,7 +312,7 @@ static unsigned long dmatest_random(void)
> {
> unsigned long buf;
>
> - prandom_bytes(, sizeof(buf));
> + get_random_bytes(, sizeof(buf));
> return buf;
> }
>
> diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c
> index 4bdaf4aa7007..c941a5a41ea6 100644
> --- a/drivers/mtd/nand/raw/nandsim.c
> +++ b/drivers/mtd/nand/raw/nandsim.c
> @@ -1393,7 +1393,7 @@ static int ns_do_read_error(struct nandsim *ns, int num)
> unsigned int page_no = ns->regs.row;
>
> if (ns_read_error(page_no)) {
> - prandom_bytes(ns->buf.byte, num);
> + get_random_bytes(ns->buf.byte, num);
> NS_WARN("simulating read error in page %u\n", page_no);
> return 1;
> }
> diff --git a/drivers/mtd/tests/mtd_nandecctest.c
> b/drivers/mtd/tests/mtd_nandecctest.c
> index 1c7201b0f372..440988562cfd 100644
> --- a/drivers/mtd/tests/mtd_nandecctest.c
> +++ b/drivers/mtd/tests/mtd_nandecctest.c
> @@ -266,7 +266,7 @@ static int nand_ecc_test_run(const size_t size)
> goto error;
> }
>
> - prandom_bytes(correct_data, size);
> + get_random_bytes(correct_data, size);
> ecc_sw_hamming_calculate(correct_data, size, correct_ecc, sm_order);
> for (i = 0; i < ARRAY_SIZE(nand_ecc_test); i++) {
> nand_ecc_test[i].prepare(error_data, error_ecc,
> diff --git a/drivers/mtd/tests/speedtest.c b/drivers/mtd/tests/speedtest.c
> index c9ec7086bfa1..075bce32caa5 100644
> --- a/drivers/mtd/tests/speedtest.c
> +++ b/drivers/mtd/tests/speedtest.c
> @@ -223,7 +223,7 @@ static int __init mtd_speedtest_init(void)
> if (!iobuf)
> goto out;
>
> -
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On 10/6/22, Christophe Leroy wrote:
>
>
> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
>>
>>
>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
>>> Hi Christophe,
>>>
>>> On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
>>> wrote:
Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> The prandom_u32() function has been a deprecated inline wrapper around
> get_random_u32() for several releases now, and compiles down to the
> exact same code. Replace the deprecated wrapper with a direct call to
> the real function. The same also applies to get_random_int(), which is
> just a wrapper around get_random_u32().
>
> Reviewed-by: Kees Cook
> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> Acked-by: Chuck Lever # for nfsd
> Reviewed-by: Jan Kara # for ext4
> Signed-off-by: Jason A. Donenfeld
> ---
> diff --git a/arch/powerpc/kernel/process.c
> b/arch/powerpc/kernel/process.c
> index 0fbda89cd1bb..9c4c15afbbe8 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
>unsigned long arch_align_stack(unsigned long sp)
>{
>if (!(current->personality & ADDR_NO_RANDOMIZE) &&
> randomize_va_space)
> - sp -= get_random_int() & ~PAGE_MASK;
> + sp -= get_random_u32() & ~PAGE_MASK;
>return sp & ~0xf;
Isn't that a candidate for prandom_u32_max() ?
Note that sp is deemed to be 16 bytes aligned at all time.
>>>
>>> Yes, probably. It seemed non-trivial to think about, so I didn't. But
>>> let's see here... maybe it's not too bad:
>>>
>>> If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
>>> (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
>>> thing? Is that accurate? And holds across platforms (this comes up a
>>> few places)? If so, I'll do that for a v4.
>>>
>>
>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
>>
>> /*
>> * Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
>> * assign PAGE_MASK to a larger type it gets extended the way we want
>> * (i.e. with 1s in the high bits)
>> */
>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
>>
>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
>>
>>
>> So it would work I guess.
>
> But taking into account that sp must remain 16 bytes aligned, would it
> be better to do something like ?
>
> sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
>
> return sp;
Does this assume that sp is already aligned at the beginning of the
function? I'd assume from the function's name that this isn't the
case?
Jason
Re: [PATCH] powerpc/kasan/book3s_64: warn when running with hash MMU
Christophe Leroy writes:
> + KASAN list
>
> Le 06/10/2022 à 06:10, Michael Ellerman a écrit :
>> Nathan Lynch writes:
>>> kasan is known to crash at boot on book3s_64 with non-radix MMU. As
>>> noted in commit 41b7a347bf14 ("powerpc: Book3S 64-bit outline-only
>>> KASAN support"):
>>>
>>>A kernel with CONFIG_KASAN=y will crash during boot on a machine
>>>using HPT translation because not all the entry points to the
>>>generic KASAN code are protected with a call to kasan_arch_is_ready().
>>
>> I guess I thought there was some plan to fix that.
>
> I was thinking the same.
>
> Do we have a list of the said entry points to the generic code that are
> lacking a call to kasan_arch_is_ready() ?
>
> Typically, the BUG dump below shows that kasan_byte_accessible() is
> lacking the check. It should be straight forward to add
> kasan_arch_is_ready() check to kasan_byte_accessible(), shouldn't it ?
Yes :)
And one other spot, but the patch below boots OK for me. I'll leave it
running for a while just in case there's a path I've missed.
cheers
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 69f583855c8b..5def0118f2cd 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -377,6 +377,9 @@ bool __kasan_slab_free(struct kmem_cache *cache, void
*object,
static inline bool kasan_kfree_large(void *ptr, unsigned long ip)
{
+ if (!kasan_arch_is_ready())
+ return false;
+
if (ptr != page_address(virt_to_head_page(ptr))) {
kasan_report_invalid_free(ptr, ip, KASAN_REPORT_INVALID_FREE);
return true;
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 437fcc7e77cf..017d3c69e3b3 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -191,7 +191,12 @@ bool kasan_check_range(unsigned long addr, size_t size,
bool write,
bool kasan_byte_accessible(const void *addr)
{
- s8 shadow_byte = READ_ONCE(*(s8 *)kasan_mem_to_shadow(addr));
+ s8 shadow_byte;
+
+ if (!kasan_arch_is_ready())
+ return true;
+
+ shadow_byte = READ_ONCE(*(s8 *)kasan_mem_to_shadow(addr));
return shadow_byte >= 0 && shadow_byte < KASAN_GRANULE_SIZE;
}
Re: [PATCH v2 06/19] powerpc/cputable: Split cpu_specs[] out of cputable.h
Christophe Leroy writes: > Le 20/09/2022 à 10:56, Nicholas Piggin a écrit : >> On Tue Sep 20, 2022 at 3:01 AM AEST, Christophe Leroy wrote: >> >> This series is a nice cleanup. No comments yet but kernel/ is getting >> pretty crowded. Should we make some subdirectories for subarch things >> like mm has? >> >> Can do that after your series. Probably requires another merge window >> to do it. >> > > By the way, I'm wondering how we decide whether some code goes in > arch/powerpc/kernel/ or in arch/powerpc/lib/ On a case-by-case basis? :) I think our lib is *mostly* routines that are library like, eg. string copy, memcpy, memcmp etc. Though there's also single step code, code patching etc. I guess one thing they have in common is that they're (somewhat) self contained routines that do a specific thing, and that they are called from various other parts of the kernel. On the other hand code in kernel *tends* to be more larger things, like early boot sequence, interrupt/syscall entry, module loading etc. But really kernel is where everything lives that doesn't have anywhere else to go, so it's a bit of a dumping ground. Talking specifically about all these CPU files, I think we could create an arch/powerpc/cpu for them. On x86 they have arch/x86/kernel/cpu, but the kernel seems redundant. With all the CPU specs, CPU setups, and dt_cpu_ftrs.c we'd have ~4500 lines that could go in arch/powerpc/cpu. Which seems like enough to justify a directory. cheers
