Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 05:21:17PM +0100, Stanislaw Gruszka wrote: > On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > > wrote: > > > > > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > > Please CC me on future revisions. > > > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > > There's no need to rename anything. So nack on this patch 1/5. > > > > > > It is not obvious (for casual developers like me) that p in prandom > > > stands for predictable. Some renaming would be useful IMHO. I disagree. pseudo-random has *always* menat "predictable". And the 'p' in prandom was originally "pseudo-random". In userspace, random(3) is also pseudo-random, and is ***utterly*** predictable. So the original use of prandom() was a bit more of an explicit nod to the fact that prandom is something which is inherently predictable. So I don't think it's needed to rename it, whether it's to "predictable_rng_prandom_u32", or "no_you_idiot_dont_you_dare_use_it_for_cryptographi_purposes_prandom_u32". I think we need to assume a certain base level of competence, especially for someone who is messing with security psensitive kernel code. If a developer doesn't know that a prng is predictable, that's probably the *least* of the sort of mistakes that they might make. - Ted
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > wrote: > > > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > Please CC me on future revisions. > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > There's no need to rename anything. So nack on this patch 1/5. > > > > It is not obvious (for casual developers like me) that p in prandom > > stands for predictable. Some renaming would be useful IMHO. > > Renaming makes backports more complicated, because stable teams will > have to 'undo' name changes. > Stable teams are already overwhelmed by the amount of backports, and > silly merge conflicts. Since when backporting problems is valid argument for stop making changes? That's new for me. > linux kernel is not for casual readers. Sure. Regards Stanislaw
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 05:53:52PM +0200, Andy Shevchenko wrote: > On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > > wrote: > > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > > Please CC me on future revisions. > > > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > > There's no need to rename anything. So nack on this patch 1/5. > > > > > > It is not obvious (for casual developers like me) that p in prandom > > > stands for predictable. Some renaming would be useful IMHO. > > > > Renaming makes backports more complicated, because stable teams will > > have to 'undo' name changes. > > Stable teams are already overwhelmed by the amount of backports, and > > silly merge conflicts. > > > > Take another example : > > > > u64 timecounter_read(struct timecounter *tc) > > > > You would think this function would read the timecounter, right ? > > > > Well, it _updates_ many fields from @tc, so a 'better name' would also > > be useful. > > Right, at some point we become into the world of > > #define true 0 > > because... (read below) > > > linux kernel is not for casual readers. > > P.S. I believe you applied a common sense and in some cases > the renames are necessary. And before you become to a wrong conclusion by reading between the lines, no, I'm not taking either side (to rename or not to rename) in this case. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > wrote: > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > Please CC me on future revisions. > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > There's no need to rename anything. So nack on this patch 1/5. > > > > It is not obvious (for casual developers like me) that p in prandom > > stands for predictable. Some renaming would be useful IMHO. > > Renaming makes backports more complicated, because stable teams will > have to 'undo' name changes. > Stable teams are already overwhelmed by the amount of backports, and > silly merge conflicts. > > Take another example : > > u64 timecounter_read(struct timecounter *tc) > > You would think this function would read the timecounter, right ? > > Well, it _updates_ many fields from @tc, so a 'better name' would also > be useful. Right, at some point we become into the world of #define true 0 because... (read below) > linux kernel is not for casual readers. P.S. I believe you applied a common sense and in some cases the renames are necessary. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka wrote: > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > Please CC me on future revisions. > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > There's no need to rename anything. So nack on this patch 1/5. > > It is not obvious (for casual developers like me) that p in prandom > stands for predictable. Some renaming would be useful IMHO. Renaming makes backports more complicated, because stable teams will have to 'undo' name changes. Stable teams are already overwhelmed by the amount of backports, and silly merge conflicts. Take another example : u64 timecounter_read(struct timecounter *tc) You would think this function would read the timecounter, right ? Well, it _updates_ many fields from @tc, so a 'better name' would also be useful. linux kernel is not for casual readers.
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > Please CC me on future revisions. > > As of 6.2, the prandom namespace is *only* for predictable randomness. > There's no need to rename anything. So nack on this patch 1/5. It is not obvious (for casual developers like me) that p in prandom stands for predictable. Some renaming would be useful IMHO. Regards Stanislaw
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
Please CC me on future revisions. As of 6.2, the prandom namespace is *only* for predictable randomness. There's no need to rename anything. So nack on this patch 1/5. With regards to the remaining patches in this series, if you want to move prandom_u32_state callers over to get_random_bytes() and get_random_u32(), that's fine from my perspective, but last I looked, there was much usage in places where being repeatable was actually the goal - test suites and such, where you want to be able to redo your tests with the same seed. So you'll have to look at each instance case by case and convince whoever maintains that code that they don't need predictability. However, if you do that, the right functions to use are get_random_bytes() and get_random_u32(). Jason
[PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
From: David
Since the two functions
prandom_byte_state and prandom_u32_state
use the weak prng prandom_u32,
we added the prefix predictable_rng,
to their signatures so it is clear they are weak.
Signed-off-by: David
---
arch/x86/mm/kaslr.c | 2 +-
.../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +-
.../i915/gem/selftests/i915_gem_client_blt.c | 2 +-
.../i915/gem/selftests/i915_gem_coherency.c | 2 +-
.../drm/i915/gem/selftests/i915_gem_context.c | 2 +-
drivers/gpu/drm/i915/gt/selftest_lrc.c| 2 +-
drivers/gpu/drm/i915/gt/selftest_migrate.c| 2 +-
drivers/gpu/drm/i915/gt/selftest_timeline.c | 4 +-
drivers/gpu/drm/i915/selftests/i915_random.c | 4 +-
drivers/gpu/drm/i915/selftests/i915_random.h | 4 +-
drivers/gpu/drm/i915/selftests/i915_syncmap.c | 4 +-
.../drm/i915/selftests/intel_memory_region.c | 10 ++---
drivers/gpu/drm/i915/selftests/scatterlist.c | 4 +-
drivers/gpu/drm/lib/drm_random.c | 2 +-
drivers/mtd/tests/oobtest.c | 10 ++---
drivers/mtd/tests/pagetest.c | 12 +++---
drivers/mtd/tests/subpagetest.c | 12 +++---
drivers/scsi/fcoe/fcoe_ctlr.c | 2 +-
include/linux/prandom.h | 6 +--
kernel/bpf/core.c | 2 +-
lib/interval_tree_test.c | 6 +--
lib/random32.c| 42 +--
lib/rbtree_test.c | 4 +-
lib/test_bpf.c| 2 +-
lib/test_parman.c | 2 +-
lib/test_scanf.c | 8 ++--
mm/slab.c | 2 +-
mm/slab_common.c | 2 +-
28 files changed, 79 insertions(+), 79 deletions(-)
diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
index 557f0fe25..66c17b449 100644
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -123,7 +123,7 @@ void __init kernel_randomize_memory(void)
* available.
*/
entropy = remain_entropy / (ARRAY_SIZE(kaslr_regions) - i);
- prandom_bytes_state(_state, , sizeof(rand));
+ predictable_rng_prandom_bytes_state(_state, ,
sizeof(rand));
entropy = (rand % (entropy + 1)) & PUD_MASK;
vaddr += entropy;
*kaslr_regions[i].base = vaddr;
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index c570cf780..f698d58e4 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -1294,7 +1294,7 @@ static u32 igt_random_size(struct rnd_state *prng,
GEM_BUG_ON(min_page_size > max_page_size);
mask = ((max_page_size << 1ULL) - 1) & PAGE_MASK;
- size = prandom_u32_state(prng) & mask;
+ size = predictable_rng_prandom_u32_state(prng) & mask;
if (size < min_page_size)
size |= min_page_size;
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
index 9a6a6b5b7..039f17b6b 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
@@ -630,7 +630,7 @@ static int tiled_blits_prepare(struct tiled_blits *t,
/* Use scratch to fill objects */
for (i = 0; i < ARRAY_SIZE(t->buffers); i++) {
- fill_scratch(t, map, prandom_u32_state(prng));
+ fill_scratch(t, map, predictable_rng_prandom_u32_state(prng));
GEM_BUG_ON(verify_buffer(t, >scratch, prng));
err = tiled_blit(t,
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
index a666d7e61..24cc7e6d4 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
@@ -371,7 +371,7 @@ static int igt_gem_coherency(void *arg)
i915_random_reorder(offsets,
ncachelines, );
for (n = 0; n < count; n++)
- values[n] =
prandom_u32_state();
+ values[n] =
predictable_rng_prandom_u32_state();
for (n = 0; n < count; n++) {
err = over->set(,
offsets[n], ~values[n]);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index c6ad67b90..6e437a1d6 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -1407,7 +1407,7 @@ static int igt_ctx_readonly(void *arg)
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Mon, Dec 12, 2022 at 12:16:04AM +0200, david.keisars...@mail.huji.ac.il
wrote:
> From: David
>
> Since the two functions
> prandom_byte_state and prandom_u32_state
> use the weak prng prandom_u32,
> we added the prefix predictable_rng,
> to their signatures so it is clear they are weak.
It's fancy indentation.
...
> /* Fisher-Yates shuffle */
> for (i = count - 1; i > 0; i--) {
> - rand = prandom_u32_state(_state);
> + rand =
> predictable_rng_prandom_u32_state(_state);
Isn't it too many "random":s encoded in the name?
I would leave either "rng" or "[p]random".
> rand %= (i + 1);
> swap_free_obj(slab, i, rand);
> }
--
With Best Regards,
Andy Shevchenko
