[PATCH v6 5/7] treewide: use get_random_u32() when possible

Rolf Eike Beer eike-kernel at sf-tec.de
Thu Oct 13 02:25:11 PDT 2022


Am Dienstag, 11. Oktober 2022, 01:06:11 CEST schrieb Jason A. Donenfeld:
> 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(). This was done as a basic find
> and replace.
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> Reviewed-by: Kees Cook <keescook at chromium.org>
> Reviewed-by: Yury Norov <yury.norov at gmail.com>
> Acked-by: Toke Høiland-Jørgensen <toke at toke.dk> # for sch_cake
> Acked-by: Chuck Lever <chuck.lever at oracle.com> # for nfsd
> Reviewed-by: Jan Kara <jack at suse.cz> # for ext4
> Acked-by: Mika Westerberg <mika.westerberg at linux.intel.com> # for
> thunderbolt Acked-by: Darrick J. Wong <djwong at kernel.org> # for xfs
> Signed-off-by: Jason A. Donenfeld <Jason at zx2c4.com>
> ---
>  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/tests/drm_buddy_test.c         |  2 +-
>  drivers/gpu/drm/tests/drm_mm_test.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/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/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/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 +-
>  72 files changed, 101 insertions(+), 101 deletions(-)
> 

> diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
> index 5a1dd4736b56..b358a74ed7ed 100644
> --- a/lib/test_rhashtable.c
> +++ b/lib/test_rhashtable.c
> @@ -291,7 +291,7 @@ static int __init test_rhltable(unsigned int entries)
>  	if (WARN_ON(err))
>  		goto out_free;
> 
> -	k = prandom_u32();
> +	k = get_random_u32();
>  	ret = 0;
>  	for (i = 0; i < entries; i++) {
>  		rhl_test_objects[i].value.id = k;

This one looks ok.

> @@ -369,12 +369,12 @@ static int __init test_rhltable(unsigned int entries)
>  	pr_info("test %d random rhlist add/delete operations\n", entries);
>  	for (j = 0; j < entries; j++) {
>  		u32 i = prandom_u32_max(entries);
> -		u32 prand = prandom_u32();
> +		u32 prand = get_random_u32();
> 
>  		cond_resched();
> 
>  		if (prand == 0)
> -			prand = prandom_u32();
> +			prand = get_random_u32();
> 
>  		if (prand & 1) {
>  			prand >>= 1;

But this doesn't make any sense to me. It needs a bit more context:

>			continue;
>		}

Why would one change prand wen it will be overwritten in the next loop anyway?

>		err = rhltable_remove(&rhlt, &rhl_test_objects[i].list_node, test_rht_params);
>		if (test_bit(i, obj_in_table)) {
>			clear_bit(i, obj_in_table);
>			if (WARN(err, "cannot remove element at slot %d", i))
>				continue;
>		} else {
>			if (WARN(err != -ENOENT, "removed non-existent element %d, error %d not %d",
>			     i, err, -ENOENT))
>				continue;
>		}
>
>		if (prand & 1) {
>			prand >>= 1;
>			continue;
>		}

The same code again, and in this case it is impossible to reach, as the check 
already returned false before.

Should these have been something like this in the first place:

	if (prand & 1)
		prand >>=1;
	else
		continue;

At least as the code looks now this only ever needs a single bit of randomness,
and the later checks and the shift can go away, but I suspect that something 
else was meant with that code.

Florian, can you comment and maybe fix it? When possible use prandom_u8() as 
it seems to me that you only need 3 bytes of randomness here anyway.

Or you wanted to move the variable before the loop and keep the random state
between the loops and only reseed when all '1' bits have been consumed. But 
even in this case the later checks seem wrong as the value has not changed in 
between.

Eike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20221013/e984442a/attachment-0001.sig>


More information about the Linux-nvme mailing list