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

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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

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

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

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