Re: [PATCH v1 1/5] treewide: use prandom_u32_max() when possible
On Thu, Oct 06, 2022 at 09:55:19AM -0300, Jason Gunthorpe wrote: > On Thu, Oct 06, 2022 at 06:45:25AM -0600, Jason A. Donenfeld wrote: > > On Wed, Oct 05, 2022 at 09:16:50PM -0700, Kees Cook wrote: > > > On Wed, Oct 05, 2022 at 11:48:40PM +0200, Jason A. Donenfeld wrote: > > > > Rather than incurring a division or requesting too many random bytes for > > > > the given range, use the prandom_u32_max() function, which only takes > > > > the minimum required bytes from the RNG and avoids divisions. > > > > > > Yes please! > > > > > > Since this is a treewide patch, it's helpful for (me at least) doing > > > reviews to detail the mechanism of the transformation. > > > > This is hand done. There were also various wrong seds done. And then I'd > > edit the .diff manually, and then reapply it, as an iterative process. > > No internet on the airplane, and oddly no spatch already on my laptop (I > > think I had some Gentoo ocaml issues at some point and removed it?). > > > > > e.g. I imagine this could be done with something like Coccinelle and > > > > Feel free to check the work here by using Coccinelle if you're into > > that. > > Generally these series are a lot easier to review if it is structured > as a patches doing all the unusual stuff that had to be by hand > followed by an unmodified Coccinelle/sed/etc handling the simple > stuff. > > Especially stuff that is reworking the logic beyond simple > substitution should be one patch per subsystem not rolled into a giant > one patch conversion. > > This makes the whole workflow better because the hand-done stuff can > have a chance to flow through subsystem trees. +1 to all arguments for the splitting. I looked a bit into the code I have the interest to, but I won't spam people with not-so-important questions / comments / tags, etc. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/5] treewide: use prandom_u32_max() when possible
On Thu, Oct 06, 2022 at 06:45:25AM -0600, Jason A. Donenfeld wrote: > Hi Kees, > > On Wed, Oct 05, 2022 at 09:16:50PM -0700, Kees Cook wrote: > > On Wed, Oct 05, 2022 at 11:48:40PM +0200, Jason A. Donenfeld wrote: > > > Rather than incurring a division or requesting too many random bytes for > > > the given range, use the prandom_u32_max() function, which only takes > > > the minimum required bytes from the RNG and avoids divisions. > > > > Yes please! > > > > Since this is a treewide patch, it's helpful for (me at least) doing > > reviews to detail the mechanism of the transformation. > > This is hand done. There were also various wrong seds done. And then I'd > edit the .diff manually, and then reapply it, as an iterative process. > No internet on the airplane, and oddly no spatch already on my laptop (I > think I had some Gentoo ocaml issues at some point and removed it?). > > > e.g. I imagine this could be done with something like Coccinelle and > > Feel free to check the work here by using Coccinelle if you're into > that. Generally these series are a lot easier to review if it is structured as a patches doing all the unusual stuff that had to be by hand followed by an unmodified Coccinelle/sed/etc handling the simple stuff. Especially stuff that is reworking the logic beyond simple substitution should be one patch per subsystem not rolled into a giant one patch conversion. This makes the whole workflow better because the hand-done stuff can have a chance to flow through subsystem trees. Thanks, Jason
Re: [PATCH v1 1/5] treewide: use prandom_u32_max() when possible
Hi Kees,
On Wed, Oct 05, 2022 at 09:16:50PM -0700, Kees Cook wrote:
> On Wed, Oct 05, 2022 at 11:48:40PM +0200, Jason A. Donenfeld wrote:
> > Rather than incurring a division or requesting too many random bytes for
> > the given range, use the prandom_u32_max() function, which only takes
> > the minimum required bytes from the RNG and avoids divisions.
>
> Yes please!
>
> Since this is a treewide patch, it's helpful for (me at least) doing
> reviews to detail the mechanism of the transformation.
This is hand done. There were also various wrong seds done. And then I'd
edit the .diff manually, and then reapply it, as an iterative process.
No internet on the airplane, and oddly no spatch already on my laptop (I
think I had some Gentoo ocaml issues at some point and removed it?).
> e.g. I imagine this could be done with something like Coccinelle and
Feel free to check the work here by using Coccinelle if you're into
that.
> > static inline int ubi_dbg_is_bitflip(const struct ubi_device *ubi)
> > {
> > if (ubi->dbg.emulate_bitflips)
> > - return !(prandom_u32() % 200);
> > + return !(prandom_u32_max(200));
> > return 0;
> > }
> >
>
> Because some looks automated (why the parens?)
I saw this before going out and thought I'd fixed it but I guess I sent
the wrong one.
Jason
Re: [PATCH v1 1/5] treewide: use prandom_u32_max() when possible
On Wed, Oct 5, 2022 at 9:16 PM Kees Cook wrote:
>
> On Wed, Oct 05, 2022 at 11:48:40PM +0200, Jason A. Donenfeld wrote:
> > Rather than incurring a division or requesting too many random bytes for
> > the given range, use the prandom_u32_max() function, which only takes
> > the minimum required bytes from the RNG and avoids divisions.
>
> Yes please!
>
> Since this is a treewide patch, it's helpful for (me at least) doing
> reviews to detail the mechanism of the transformation.
>
> e.g. I imagine this could be done with something like Coccinelle and
>
> @no_modulo@
> expression E;
> @@
>
> - (prandom_u32() % (E))
> + prandom_u32_max(E)
>
> > diff --git a/drivers/mtd/ubi/debug.h b/drivers/mtd/ubi/debug.h
> > index 118248a5d7d4..4236c799a47c 100644
> > --- a/drivers/mtd/ubi/debug.h
> > +++ b/drivers/mtd/ubi/debug.h
> > @@ -73,7 +73,7 @@ static inline int ubi_dbg_is_bgt_disabled(const struct
> > ubi_device *ubi)
> > static inline int ubi_dbg_is_bitflip(const struct ubi_device *ubi)
> > {
> > if (ubi->dbg.emulate_bitflips)
> > - return !(prandom_u32() % 200);
> > + return !(prandom_u32_max(200));
> > return 0;
> > }
> >
>
> Because some looks automated (why the parens?)
>
> > @@ -393,14 +387,11 @@ static struct test_driver {
> >
> > static void shuffle_array(int *arr, int n)
> > {
> > - unsigned int rnd;
> > int i, j;
> >
> > for (i = n - 1; i > 0; i--) {
> > - rnd = prandom_u32();
> > -
> > /* Cut the range. */
> > - j = rnd % i;
> > + j = prandom_u32_max(i);
> >
> > /* Swap indexes. */
> > swap(arr[i], arr[j]);
>
> And some by hand. :)
>
> Reviewed-by: Kees Cook
Thanks!
Reviewed-by: KP Singh
>
> --
> Kees Cook
Re: [PATCH v1 1/5] treewide: use prandom_u32_max() when possible
On Wed, Oct 05, 2022 at 11:48:40PM +0200, Jason A. Donenfeld wrote:
> Rather than incurring a division or requesting too many random bytes for
> the given range, use the prandom_u32_max() function, which only takes
> the minimum required bytes from the RNG and avoids divisions.
Yes please!
Since this is a treewide patch, it's helpful for (me at least) doing
reviews to detail the mechanism of the transformation.
e.g. I imagine this could be done with something like Coccinelle and
@no_modulo@
expression E;
@@
- (prandom_u32() % (E))
+ prandom_u32_max(E)
> diff --git a/drivers/mtd/ubi/debug.h b/drivers/mtd/ubi/debug.h
> index 118248a5d7d4..4236c799a47c 100644
> --- a/drivers/mtd/ubi/debug.h
> +++ b/drivers/mtd/ubi/debug.h
> @@ -73,7 +73,7 @@ static inline int ubi_dbg_is_bgt_disabled(const struct
> ubi_device *ubi)
> static inline int ubi_dbg_is_bitflip(const struct ubi_device *ubi)
> {
> if (ubi->dbg.emulate_bitflips)
> - return !(prandom_u32() % 200);
> + return !(prandom_u32_max(200));
> return 0;
> }
>
Because some looks automated (why the parens?)
> @@ -393,14 +387,11 @@ static struct test_driver {
>
> static void shuffle_array(int *arr, int n)
> {
> - unsigned int rnd;
> int i, j;
>
> for (i = n - 1; i > 0; i--) {
> - rnd = prandom_u32();
> -
> /* Cut the range. */
> - j = rnd % i;
> + j = prandom_u32_max(i);
>
> /* Swap indexes. */
> swap(arr[i], arr[j]);
And some by hand. :)
Reviewed-by: Kees Cook
--
Kees Cook
[PATCH v1 1/5] treewide: use prandom_u32_max() when possible
Rather than incurring a division or requesting too many random bytes for
the given range, use the prandom_u32_max() function, which only takes
the minimum required bytes from the RNG and avoids divisions.
Signed-off-by: Jason A. Donenfeld
---
arch/x86/mm/pat/cpa-test.c| 4 +-
crypto/testmgr.c | 86 +--
drivers/block/drbd/drbd_receiver.c| 4 +-
drivers/infiniband/core/cma.c | 2 +-
drivers/infiniband/hw/cxgb4/id_table.c| 4 +-
drivers/infiniband/hw/hns/hns_roce_ah.c | 5 +-
drivers/infiniband/ulp/rtrs/rtrs-clt.c| 3 +-
drivers/mmc/core/core.c | 4 +-
drivers/mmc/host/dw_mmc.c | 2 +-
drivers/mtd/nand/raw/nandsim.c| 4 +-
drivers/mtd/tests/mtd_nandecctest.c | 10 +--
drivers/mtd/tests/stresstest.c| 17 +---
drivers/mtd/ubi/debug.c | 2 +-
drivers/mtd/ubi/debug.h | 6 +-
drivers/net/ethernet/broadcom/cnic.c | 3 +-
.../chelsio/inline_crypto/chtls/chtls_io.c| 4 +-
drivers/net/hamradio/baycom_epp.c | 2 +-
drivers/net/hamradio/hdlcdrv.c| 2 +-
drivers/net/hamradio/yam.c| 2 +-
drivers/net/phy/at803x.c | 2 +-
.../broadcom/brcm80211/brcmfmac/p2p.c | 2 +-
.../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c | 2 +-
drivers/scsi/fcoe/fcoe_ctlr.c | 4 +-
drivers/scsi/qedi/qedi_main.c | 2 +-
fs/ceph/inode.c | 2 +-
fs/ceph/mdsmap.c | 2 +-
fs/ext4/super.c | 7 +-
fs/f2fs/gc.c | 2 +-
fs/f2fs/segment.c | 8 +-
fs/ubifs/debug.c | 8 +-
fs/ubifs/lpt_commit.c | 14 +--
fs/ubifs/tnc_commit.c | 2 +-
fs/xfs/libxfs/xfs_alloc.c | 2 +-
fs/xfs/libxfs/xfs_ialloc.c| 2 +-
fs/xfs/xfs_error.c| 2 +-
kernel/time/clocksource.c | 2 +-
lib/fault-inject.c| 2 +-
lib/find_bit_benchmark.c | 4 +-
lib/reed_solomon/test_rslib.c | 6 +-
lib/sbitmap.c | 4 +-
lib/test_list_sort.c | 2 +-
lib/test_vmalloc.c| 17 +---
net/ceph/mon_client.c | 2 +-
net/ceph/osd_client.c | 2 +-
net/core/neighbour.c | 2 +-
net/core/pktgen.c | 43 +-
net/core/stream.c | 2 +-
net/ipv4/igmp.c | 6 +-
net/ipv4/inet_connection_sock.c | 2 +-
net/ipv4/inet_hashtables.c| 2 +-
net/ipv6/addrconf.c | 8 +-
net/ipv6/mcast.c | 10 +--
net/netfilter/ipvs/ip_vs_twos.c | 4 +-
net/packet/af_packet.c| 2 +-
net/sched/act_gact.c | 2 +-
net/sched/act_sample.c| 2 +-
net/sched/sch_netem.c | 4 +-
net/sctp/socket.c | 2 +-
net/sunrpc/cache.c| 2 +-
net/sunrpc/xprtsock.c | 2 +-
net/tipc/socket.c | 2 +-
net/xfrm/xfrm_state.c | 2 +-
62 files changed, 173 insertions(+), 196 deletions(-)
diff --git a/arch/x86/mm/pat/cpa-test.c b/arch/x86/mm/pat/cpa-test.c
index 0612a73638a8..423b21e80929 100644
--- a/arch/x86/mm/pat/cpa-test.c
+++ b/arch/x86/mm/pat/cpa-test.c
@@ -136,10 +136,10 @@ static int pageattr_test(void)
failed += print_split(&sa);
for (i = 0; i < NTEST; i++) {
- unsigned long pfn = prandom_u32() % max_pfn_mapped;
+ unsigned long pfn = prandom_u32_max(max_pfn_mapped);
addr[i] = (unsigned long)__va(pfn << PAGE_SHIFT);
- len[i] = prandom_u32() % NPAGES;
+ len[i] = prandom_u32_max(NPAGES);
len[i] = min_t(unsigned long, len[i], max_pfn_mapped - pfn - 1);
if (len[i] == 0)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 5349ffee6bbd..be45217acde4 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -855,9 +855,9 @@ static int prepare_keybuf(const u8 *key, unsigned int ksize,
/* Generate a random length in range [0, max_len], but prefer smaller values */
static unsigned int generate_random_length(unsigned int max_len)
{
- unsigned int len = prandom_u32() % (max_len + 1);
+ unsigned int len = prandom_u32_max(max_len + 1);
- switch (prandom_u32() % 4) {
+ swit
