Thread (21 messages) 21 messages, 5 authors, 2022-10-09

Re: [PATCH v4 4/6] treewide: use get_random_u32() when possible

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: 2022-10-08 01:40:35
Also in: dri-devel, kernel-janitors, linux-block, linux-crypto, linux-doc, linux-fsdevel, linux-media, linux-mips, linux-mm, linux-mmc, linux-nvme, linux-patches, linux-rdma, linux-s390, linux-um, linux-usb, linux-wireless, linuxppc-dev, lkml, loongarch, sparclinux

On Fri, Oct 07, 2022 at 10:34:47PM +0200, Rolf Eike Beer wrote:
quoted
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 : 0x3ffffUL)

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.]
quoted
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, &addr);
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.
quoted
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)
 				   &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.
quoted
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help