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