From: François Michel <francois.michel@uclouvain.be>

Add the netem_get_random_u32 function to sch_netem.c
and use it to generate the random loss events of netem.

If no seed was specified by the application through
setting TCA_NETEM_PRNG_SEED, the default behaviour
is the same as before, i.e. it relies on the unseeded
get_random_u32() function. If a seed was provided,
it relies on the seeded prng attribute of
struct netem_sched_data.

Signed-off-by: François Michel <francois.michel@uclouvain.be>
---
 net/sched/sch_netem.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index d73596d0e912..1190782ef79d 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -154,6 +154,19 @@ struct netem_sched_data {
 	struct disttable *slot_dist;
 };
 
+/* netem_get_random_u32 - polls a new random 32-bits integer from
+ * the prng.
+ * Uses a deterministic seeded prng if p->deterministic_rng is true.
+ * Uses get_random_u32() underneath if p is NULL or if p->deterministic_rng
+ * is false.
+ */
+static u32 netem_get_random_u32(struct prng *p)
+{
+	if (p && p->deterministic_rng)
+		return prandom_u32_state(&p->prng_state);
+	return get_random_u32();
+}
+
 /* Time stamp put into socket buffer control block
  * Only valid when skbs are in our internal t(ime)fifo queue.
  *
@@ -207,7 +220,7 @@ static u32 get_crandom(struct crndstate *state)
 static bool loss_4state(struct netem_sched_data *q)
 {
 	struct clgstate *clg = &q->clg;
-	u32 rnd = get_random_u32();
+	u32 rnd = netem_get_random_u32(&q->prng);
 
 	/*
 	 * Makes a comparison between rnd and the transition
@@ -272,18 +285,19 @@ static bool loss_4state(struct netem_sched_data *q)
 static bool loss_gilb_ell(struct netem_sched_data *q)
 {
 	struct clgstate *clg = &q->clg;
+	struct prng *p = &q->prng;
 
 	switch (clg->state) {
 	case GOOD_STATE:
-		if (get_random_u32() < clg->a1)
+		if (netem_get_random_u32(p) < clg->a1)
 			clg->state = BAD_STATE;
-		if (get_random_u32() < clg->a4)
+		if (netem_get_random_u32(p) < clg->a4)
 			return true;
 		break;
 	case BAD_STATE:
-		if (get_random_u32() < clg->a2)
+		if (netem_get_random_u32(p) < clg->a2)
 			clg->state = GOOD_STATE;
-		if (get_random_u32() > clg->a3)
+		if (netem_get_random_u32(p) > clg->a3)
 			return true;
 	}
 
-- 
2.41.0

On Mon, 14 Aug 2023 04:31:39 +0200
Francois Michel <francois.michel@uclouvain.be> wrote:

> +/* netem_get_random_u32 - polls a new random 32-bits integer from
> + * the prng.
> + * Uses a deterministic seeded prng if p->deterministic_rng is true.
> + * Uses get_random_u32() underneath if p is NULL or if p->deterministic_rng
> + * is false.
> + */
> +static u32 netem_get_random_u32(struct prng *p)

Overall I am fine with this patch, but the function name is getting excessively
long. It is a local function, so no need for netem_ prefix.

Checking for p == NULL is redundant, all callers are passing a valid pointer.

For logical consistency, put the new wrapper before init_crandom() and after netem_skb_cb().

Since this is not security related, the change could also be simplified to just
always prandom_u32_state() and initialize the state on first use with either 
get_random or provided seed.  This would also simplify the code around storing
original seed and boolean.

Reminds me of the quote attributed to Mark Twain:
“I apologize for such a long letter - I didn't have time to write a short one.”

Hi,

Le 14/08/23 à 17:49, Stephen Hemminger a écrit :
> On Mon, 14 Aug 2023 04:31:39 +0200
> Francois Michel <francois.michel@uclouvain.be> wrote:
> 
>> +/* netem_get_random_u32 - polls a new random 32-bits integer from
>> + * the prng.
>> + * Uses a deterministic seeded prng if p->deterministic_rng is true.
>> + * Uses get_random_u32() underneath if p is NULL or if p->deterministic_rng
>> + * is false.
>> + */
>> +static u32 netem_get_random_u32(struct prng *p)
> 
> Overall I am fine with this patch, but the function name is getting excessively
> long. It is a local function, so no need for netem_ prefix.
> 
> Checking for p == NULL is redundant, all callers are passing a valid pointer.
> 
> For logical consistency, put the new wrapper before init_crandom() and after netem_skb_cb().
> 
> Since this is not security related, the change could also be simplified to just
> always prandom_u32_state() and initialize the state on first use with either
> get_random or provided seed.  This would also simplify the code around storing
> original seed and boolean.

Thank you very much for your comment.

I do not use prandom_u32_state() directly in order to ensure
that the original netem behaviour is preserved when no seed is specified.

But I agree that it would be cleaner to directly use prandom_u32_state() 
instead of get_random_u32(), if we are sure that we won't have problems 
(e.g. short prng cycles) with the randomly generated seeds when no seed 
is explicitly provided. If it is okay, then
I don't see a reason to not use prandom_u32_state() directly.

I'll make an update of the patch taking these comments into account and 
simplifying the patch.

Thank you !

François

> 
> Reminds me of the quote attributed to Mark Twain:
> “I apologize for such a long letter - I didn't have time to write a short one.”

On Mon, 14 Aug 2023 22:14:53 +0200
François Michel <francois.michel@uclouvain.be> wrote:

> Thank you very much for your comment.
> 
> I do not use prandom_u32_state() directly in order to ensure
> that the original netem behaviour is preserved when no seed is specified.
> 
> But I agree that it would be cleaner to directly use prandom_u32_state() 
> instead of get_random_u32(), if we are sure that we won't have problems 
> (e.g. short prng cycles) with the randomly generated seeds when no seed 
> is explicitly provided. If it is okay, then
> I don't see a reason to not use prandom_u32_state() directly.
> 
> I'll make an update of the patch taking these comments into account and 
> simplifying the patch.
> 
> Thank you !
> 
> François

Older versions of netem had prandom_u32_state() equivalent built inside.
The code was split out later to be usable in other places.
Over time, get_random_u32() was added because it was more random and
there were calls to unify random number usage.

The prandom was based on Tausworthe to have good long simulation cycles
and reasonable performance.

Going back to prandom always is good idea, since get_random_u32()
has addition locking and batched entropy which netem really doesn't need/want.