Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed, 2022-10-12 at 21:29 +, David Laight wrote: > From: Joe Perches > > Sent: 12 October 2022 20:17 > > > > On Wed, 2022-10-05 at 23:48 +0200, 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. > > [] > > > diff --git 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; > > > > trivia: > > > > There are somewhat odd size mismatches here. > > > > I had to think a tiny bit if random() returned a value from 0 to 7 > > and was promoted to a 64 bit value then truncated to 32 bit. > > > > Perhaps these would be clearer as ~7U and not ~7UL > > That makes no difference - the compiler will generate the same code. True, more or less. It's more a question for the reader. > The real question is WTF is the code doing? True. > The '& ~7u' clears the bottom 3 bits. > The '- 1' then sets the bottom 3 bits and decrements the > (random) high bits. Right. > So is the same as get_random_u32() | 7. True, it's effectively the same as the upper 29 bits are random anyway and the bottom 3 bits are always set. > But I bet the coder had something else in mind. Likely. And it was also likely copy/pasted a few times.
RE: [PATCH v1 3/5] treewide: use get_random_u32() when possible
From: Joe Perches > Sent: 12 October 2022 20:17 > > On Wed, 2022-10-05 at 23:48 +0200, 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. > [] > > diff --git 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; > > trivia: > > There are somewhat odd size mismatches here. > > I had to think a tiny bit if random() returned a value from 0 to 7 > and was promoted to a 64 bit value then truncated to 32 bit. > > Perhaps these would be clearer as ~7U and not ~7UL That makes no difference - the compiler will generate the same code. The real question is WTF is the code doing? The '& ~7u' clears the bottom 3 bits. The '- 1' then sets the bottom 3 bits and decrements the (random) high bits. So is the same as get_random_u32() | 7. But I bet the coder had something else in mind. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed, 2022-10-05 at 23:48 +0200, 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.
[]
> diff --git 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;
trivia:
There are somewhat odd size mismatches here.
I had to think a tiny bit if random() returned a value from 0 to 7
and was promoted to a 64 bit value then truncated to 32 bit.
Perhaps these would be clearer as ~7U and not ~7UL
> 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;
etc...
drivers/infiniband/hw/cxgb4/cm.c: u32 isn = (prandom_u32() & ~7UL) - 1;
drivers/infiniband/hw/cxgb4/cm.c: u32 isn = (prandom_u32() &
~7UL) - 1;
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c:rpl5->iss =
cpu_to_be32((prandom_u32() & ~7UL) - 1);
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() &
~7UL) - 1;
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() &
~7UL) - 1;
drivers/target/iscsi/cxgbit/cxgbit_cm.c:rpl5->iss =
cpu_to_be32((prandom_u32() & ~7UL) - 1);
Re: [f2fs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 10:43:31AM +0200, Jan Kara wrote:
> The code here is effectively doing the
>
> parent_group = prandom_u32_max(ngroups);
>
> Similarly here we can use prandom_u32_max(ngroups) like:
>
> if (qstr) {
> ...
> parent_group = hinfo.hash % ngroups;
> } else
> parent_group = prandom_u32_max(ngroups);
Nice catch. I'll move these to patch #1.
> > diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> > index 9af68a7ecdcf..588cb09c5291 100644
> > --- a/fs/ext4/mmp.c
> > +++ b/fs/ext4/mmp.c
> > @@ -265,7 +265,7 @@ static unsigned int mmp_new_seq(void)
> > u32 new_seq;
> >
> > do {
> > - new_seq = prandom_u32();
> > + new_seq = get_random_u32();
> > } while (new_seq > EXT4_MMP_SEQ_MAX);
>
> OK, here we again effectively implement prandom_u32_max(EXT4_MMP_SEQ_MAX + 1).
> Just presumably we didn't want to use modulo here because EXT4_MMP_SEQ_MAX
> is rather big and so the resulting 'new_seq' would be seriously
> non-uniform.
I'm not handling this during this patch set, but if in the course of
review we find enough places that want actually uniformly bounded
integers, I'll implement efficient rejection sampling to clean up these
cases, with something faster and general, and add a new function for it.
So far this is the first case to come up, but we'll probably eventually
find others. So I'll make note of this.
Jason
Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed, Oct 05, 2022 at 11:48:42PM +0200, Jason A. Donenfeld wrote: > 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; Maybe this wants to be written as (prandom_max(U32_MAX >> 7) << 7) | 7 ? > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > index fd9d7f2c4d64..a605cf66b83e 100644 > --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > @@ -465,7 +465,7 @@ static int ipoib_cm_req_handler(struct ib_cm_id *cm_id, > goto err_qp; > } > > - psn = prandom_u32() & 0xff; > + psn = get_random_u32() & 0xff; prandom_max(0xff + 1) ? Jason
Re: [f2fs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 6, 2022 at 7:01 AM Andy Shevchenko
wrote:
>
> On Thu, Oct 06, 2022 at 06:33:15AM -0600, Jason A. Donenfeld wrote:
> > On Thu, Oct 06, 2022 at 10:43:31AM +0200, Jan Kara wrote:
>
> ...
>
> > > The code here is effectively doing the
> > >
> > > parent_group = prandom_u32_max(ngroups);
> > >
> > > Similarly here we can use prandom_u32_max(ngroups) like:
> > >
> > > if (qstr) {
> > > ...
> > > parent_group = hinfo.hash % ngroups;
> > > } else
> > > parent_group = prandom_u32_max(ngroups);
> >
> > Nice catch. I'll move these to patch #1.
>
> I believe coccinelle is able to handle this kind of code as well
I'd be extremely surprised. The details were kind of non obvious. I
just spent a decent amount of time manually checking those blocks, to
make sure we didn't wind up with different behavior, given the
variable reuse.
Jason
Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 6, 2022 at 6:47 AM Jason Gunthorpe wrote: > > On Wed, Oct 05, 2022 at 11:48:42PM +0200, Jason A. Donenfeld wrote: > > > 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; > > Maybe this wants to be written as > > (prandom_max(U32_MAX >> 7) << 7) | 7 > > ? Holy smokes. Yea I guess maybe? It doesn't exactly gain anything or make the code clearer though, and is a little bit more magical than I'd like on a first pass. > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > index fd9d7f2c4d64..a605cf66b83e 100644 > > --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > @@ -465,7 +465,7 @@ static int ipoib_cm_req_handler(struct ib_cm_id *cm_id, > > goto err_qp; > > } > > > > - psn = prandom_u32() & 0xff; > > + psn = get_random_u32() & 0xff; > > prandom_max(0xff + 1) That'd work, but again it's not more clear. Authors here are going for a 24-bit number, and masking seems like a clear way to express that. Jason
Re: [f2fs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 06:33:15AM -0600, Jason A. Donenfeld wrote:
> On Thu, Oct 06, 2022 at 10:43:31AM +0200, Jan Kara wrote:
...
> > The code here is effectively doing the
> >
> > parent_group = prandom_u32_max(ngroups);
> >
> > Similarly here we can use prandom_u32_max(ngroups) like:
> >
> > if (qstr) {
> > ...
> > parent_group = hinfo.hash % ngroups;
> > } else
> > parent_group = prandom_u32_max(ngroups);
>
> Nice catch. I'll move these to patch #1.
I believe coccinelle is able to handle this kind of code as well, so Kees'
proposal to use it seems more plausible since it's less error prone and more
flexible / powerful.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 07:05:48AM -0600, Jason A. Donenfeld wrote: > On Thu, Oct 6, 2022 at 6:47 AM Jason Gunthorpe wrote: > > On Wed, Oct 05, 2022 at 11:48:42PM +0200, Jason A. Donenfeld wrote: ... > > > - u32 isn = (prandom_u32() & ~7UL) - 1; > > > + u32 isn = (get_random_u32() & ~7UL) - 1; > > > > Maybe this wants to be written as > > > > (prandom_max(U32_MAX >> 7) << 7) | 7 > > ? > > Holy smokes. Yea I guess maybe? It doesn't exactly gain anything or > make the code clearer though, and is a little bit more magical than > I'd like on a first pass. Shouldn't the two first 7s to be 3s? ... > > > - psn = prandom_u32() & 0xff; > > > + psn = get_random_u32() & 0xff; > > > > prandom_max(0xff + 1) > > That'd work, but again it's not more clear. Authors here are going for > a 24-bit number, and masking seems like a clear way to express that. We have some 24-bit APIs (and 48-bit) already in kernel, why not to have get_random_u24() ? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 07:05:48AM -0600, Jason A. Donenfeld wrote: > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > > b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > > index fd9d7f2c4d64..a605cf66b83e 100644 > > > --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c > > > @@ -465,7 +465,7 @@ static int ipoib_cm_req_handler(struct ib_cm_id > > > *cm_id, > > > goto err_qp; > > > } > > > > > > - psn = prandom_u32() & 0xff; > > > + psn = get_random_u32() & 0xff; > > > > prandom_max(0xff + 1) > > That'd work, but again it's not more clear. Authors here are going for > a 24-bit number, and masking seems like a clear way to express that. vs just asking directly for a 24 bit number? Jason
Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed 05-10-22 23:48:42, 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.
>
> Signed-off-by: Jason A. Donenfeld
...
> diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
> index 998dd2ac8008..e439a872c398 100644
> --- a/fs/ext2/ialloc.c
> +++ b/fs/ext2/ialloc.c
> @@ -277,7 +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();
> + group = get_random_u32();
> parent_group = (unsigned)group % ngroups;
> for (i = 0; i < ngroups; i++) {
> group = (parent_group + i) % ngroups;
The code here is effectively doing the
parent_group = prandom_u32_max(ngroups);
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index f73e5eb43eae..954ec9736a8d 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -465,7 +465,7 @@ static int find_group_orlov(struct super_block *sb,
> struct inode *parent,
> ext4fs_dirhash(parent, qstr->name, qstr->len, );
> grp = hinfo.hash;
> } else
> - grp = prandom_u32();
> + grp = get_random_u32();
Similarly here we can use prandom_u32_max(ngroups) like:
if (qstr) {
...
parent_group = hinfo.hash % ngroups;
} else
parent_group = prandom_u32_max(ngroups);
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 9af68a7ecdcf..588cb09c5291 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -265,7 +265,7 @@ static unsigned int mmp_new_seq(void)
> u32 new_seq;
>
> do {
> - new_seq = prandom_u32();
> + new_seq = get_random_u32();
> } while (new_seq > EXT4_MMP_SEQ_MAX);
OK, here we again effectively implement prandom_u32_max(EXT4_MMP_SEQ_MAX + 1).
Just presumably we didn't want to use modulo here because EXT4_MMP_SEQ_MAX
is rather big and so the resulting 'new_seq' would be seriously
non-uniform.
Honza
--
Jan Kara
SUSE Labs, CR
[PATCH v1 3/5] 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.
Signed-off-by: Jason A. Donenfeld
---
Documentation/networking/filter.rst| 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 +-
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 +++---
.../net/wireless/marvell/mwifiex/cfg80211.c| 4 ++--
.../net/wireless/microchip/wilc1000/cfg80211.c | 2 +-
.../net/wireless/quantenna/qtnfmac/cfg80211.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/ext2/ialloc.c | 2 +-
fs/ext4/ialloc.c | 4 ++--
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/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/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_rhashtable.c | 6 +++---
mm/shmem.c | 2 +-
net/802/garp.c | 2 +-
net/802/mrp.c | 2 +-
net/core/pktgen.c | 4 ++--
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 +-
57 files changed, 79 insertions(+), 79 deletions(-)
diff --git a/Documentation/networking/filter.rst
b/Documentation/networking/filter.rst
index 43cdc4d34745..f69da5074860 100644
--- a/Documentation/networking/filter.rst
+++ b/Documentation/networking/filter.rst
@@ -305,7 +305,7 @@ Possible BPF extensions are shown in the following table:
vlan_tci skb_vlan_tag_get(skb)
vlan_availskb_vlan_tag_present(skb)
vlan_tpid skb->vlan_proto
- rand prandom_u32()
+ rand get_random_u32()
===
=
These extensions can also be prefixed with '#'.
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)) {
-
