Re: [PATCH v4 4/6] treewide: use get_random_u32() when possible
On Sat, Oct 8, 2022 at 4:37 PM Jason A. Donenfeld wrote:
>
> On Sat, Oct 08, 2022 at 10:18:45PM +, David Laight wrote:
> > From: Jason A. Donenfeld
> > > Sent: 07 October 2022 19:01
> > >
> > > 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().
> > >
> > ...
> > > diff --git a/net/802/garp.c b/net/802/garp.c
> > > index f6012f8e59f0..c1bb67e25430 100644
> > > --- a/net/802/garp.c
> > > +++ b/net/802/garp.c
> > > @@ -407,7 +407,7 @@ static void garp_join_timer_arm(struct garp_applicant
> > > *app)
> > > {
> > > unsigned long delay;
> > >
> > > - delay = (u64)msecs_to_jiffies(garp_join_time) * prandom_u32() >> 32;
> > > + delay = (u64)msecs_to_jiffies(garp_join_time) * get_random_u32() >>
> > > 32;
> > > mod_timer(>join_timer, jiffies + delay);
> > > }
> > >
> > > diff --git a/net/802/mrp.c b/net/802/mrp.c
> > > index 35e04cc5390c..3e9fe9f5d9bf 100644
> > > --- a/net/802/mrp.c
> > > +++ b/net/802/mrp.c
> > > @@ -592,7 +592,7 @@ static void mrp_join_timer_arm(struct mrp_applicant
> > > *app)
> > > {
> > > unsigned long delay;
> > >
> > > - delay = (u64)msecs_to_jiffies(mrp_join_time) * prandom_u32() >> 32;
> > > + delay = (u64)msecs_to_jiffies(mrp_join_time) * get_random_u32() >> 32;
> > > mod_timer(>join_timer, jiffies + delay);
> > > }
> > >
> >
> > Aren't those:
> > delay = prandom_u32_max(msecs_to_jiffies(xxx_join_time));
>
> Probably, but too involved and peculiar for this cleanup.
>
> Feel free to send a particular patch to that maintainer.
I guess the cocci patch looks like this, so maybe I'll put that in 1/7
if I respin this.
@@
expression E;
identifier get_random_u32 =~ "get_random_int|prandom_u32|get_random_u32";
typedef u64;
@@
- ((u64)(E) * get_random_u32() >> 32)
+ prandom_u32_max(E)
Re: [PATCH v4 4/6] treewide: use get_random_u32() when possible
On Sat, Oct 08, 2022 at 10:18:45PM +, David Laight wrote:
> From: Jason A. Donenfeld
> > Sent: 07 October 2022 19:01
> >
> > 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().
> >
> ...
> > diff --git a/net/802/garp.c b/net/802/garp.c
> > index f6012f8e59f0..c1bb67e25430 100644
> > --- a/net/802/garp.c
> > +++ b/net/802/garp.c
> > @@ -407,7 +407,7 @@ static void garp_join_timer_arm(struct garp_applicant
> > *app)
> > {
> > unsigned long delay;
> >
> > - delay = (u64)msecs_to_jiffies(garp_join_time) * prandom_u32() >> 32;
> > + delay = (u64)msecs_to_jiffies(garp_join_time) * get_random_u32() >> 32;
> > mod_timer(>join_timer, jiffies + delay);
> > }
> >
> > diff --git a/net/802/mrp.c b/net/802/mrp.c
> > index 35e04cc5390c..3e9fe9f5d9bf 100644
> > --- a/net/802/mrp.c
> > +++ b/net/802/mrp.c
> > @@ -592,7 +592,7 @@ static void mrp_join_timer_arm(struct mrp_applicant
> > *app)
> > {
> > unsigned long delay;
> >
> > - delay = (u64)msecs_to_jiffies(mrp_join_time) * prandom_u32() >> 32;
> > + delay = (u64)msecs_to_jiffies(mrp_join_time) * get_random_u32() >> 32;
> > mod_timer(>join_timer, jiffies + delay);
> > }
> >
>
> Aren't those:
> delay = prandom_u32_max(msecs_to_jiffies(xxx_join_time));
Probably, but too involved and peculiar for this cleanup.
Feel free to send a particular patch to that maintainer.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1
> 1PT, UK
> Registration No: 1397386 (Wales)
RE: [PATCH v4 4/6] treewide: use get_random_u32() when possible
From: Jason A. Donenfeld
> Sent: 07 October 2022 19:01
>
> 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().
>
...
> diff --git a/net/802/garp.c b/net/802/garp.c
> index f6012f8e59f0..c1bb67e25430 100644
> --- a/net/802/garp.c
> +++ b/net/802/garp.c
> @@ -407,7 +407,7 @@ static void garp_join_timer_arm(struct garp_applicant
> *app)
> {
> unsigned long delay;
>
> - delay = (u64)msecs_to_jiffies(garp_join_time) * prandom_u32() >> 32;
> + delay = (u64)msecs_to_jiffies(garp_join_time) * get_random_u32() >> 32;
> mod_timer(>join_timer, jiffies + delay);
> }
>
> diff --git a/net/802/mrp.c b/net/802/mrp.c
> index 35e04cc5390c..3e9fe9f5d9bf 100644
> --- a/net/802/mrp.c
> +++ b/net/802/mrp.c
> @@ -592,7 +592,7 @@ static void mrp_join_timer_arm(struct mrp_applicant *app)
> {
> unsigned long delay;
>
> - delay = (u64)msecs_to_jiffies(mrp_join_time) * prandom_u32() >> 32;
> + delay = (u64)msecs_to_jiffies(mrp_join_time) * get_random_u32() >> 32;
> mod_timer(>join_timer, jiffies + delay);
> }
>
Aren't those:
delay = prandom_u32_max(msecs_to_jiffies(xxx_join_time));
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
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 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 4/6] treewide: use get_random_u32() when possible
> 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.
> 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();
> 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;
Same code comes later again.
> diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c
> index 50bcf745e816..4bdaf4aa7007 100644
> --- a/drivers/mtd/nand/raw/nandsim.c
> +++ b/drivers/mtd/nand/raw/nandsim.c
> @@ -1402,7 +1402,7 @@ static int ns_do_read_error(struct nandsim *ns, int
> num)
>
> 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?
> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c index
> ac452a0111a9..b71ce6c5b512 100644
> --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> @@ -1063,7 +1063,7 @@ static void chtls_pass_accept_rpl(struct sk_buff *skb,
> opt2 |= WND_SCALE_EN_V(WSCALE_OK(tp));
> rpl5->opt0 = cpu_to_be64(opt0);
> rpl5->opt2 = cpu_to_be32(opt2);
> - rpl5->iss = cpu_to_be32((prandom_u32() & ~7UL) - 1);
> + rpl5->iss = cpu_to_be32((get_random_u32() & ~7UL) - 1);
> set_wr_txq(skb, CPL_PRIORITY_SETUP, csk->port_id);
> t4_set_arp_err_handler(skb, sk, chtls_accept_rpl_arp_failure);
> cxgb4_l2t_send(csk->egress_dev, skb, csk->l2t_entry);
> diff --git a/drivers/net/ethernet/rocker/rocker_main.c
> b/drivers/net/ethernet/rocker/rocker_main.c index
> fc83ec23bd1d..8c3bbafabb07 100644
> --- a/drivers/net/ethernet/rocker/rocker_main.c
> +++ b/drivers/net/ethernet/rocker/rocker_main.c
> @@ -139,9 +139,9 @@ static int rocker_reg_test(const struct rocker *rocker)
> return -EIO;
> }
>
> - rnd = prandom_u32();
> + rnd = get_random_u32();
> rnd <<= 31;
> - rnd |= prandom_u32();
> + rnd |= get_random_u32();
> rocker_write64(rocker, TEST_REG64, rnd);
> test_reg = rocker_read64(rocker, TEST_REG64);
> if (test_reg != rnd * 2) {
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c index
> fabfbb0b40b0..374e1cc07a63 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c
> @@ -177,7 +177,7 @@ static int
