Currently get_random*() is used to determine the probability of fault
injection, but cryptographically secure random numbers are not required.

There is no big problem in using prandom instead of get_random*() to
determine the probability of fault injection, and it also avoids acquiring
a spinlock, which is unsafe in some contexts.

Link: https://lore.kernel.org/lkml/20241129120939.GG35539@noisy.programming.kicks-ass.net
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
---
 lib/fault-inject.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index 52eb6ba29698..92a54c8a8380 100644
--- a/lib/fault-inject.c
+++ b/lib/fault-inject.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 #include <linux/kernel.h>
 #include <linux/init.h>
-#include <linux/random.h>
+#include <linux/prandom.h>
 #include <linux/debugfs.h>
 #include <linux/sched.h>
 #include <linux/stat.h>
@@ -12,6 +12,24 @@
 #include <linux/stacktrace.h>
 #include <linux/fault-inject.h>
 
+/*
+ * The should_fail() use prandom instead of the normal Linux RNG since they don't
+ * need cryptographically secure random numbers.
+ */
+static DEFINE_PER_CPU(struct rnd_state, fault_rnd_state);
+
+static u32 fault_prandom_u32_below_100(void)
+{
+	struct rnd_state *state;
+	u32 res;
+
+	state = &get_cpu_var(fault_rnd_state);
+	res = prandom_u32_state(state);
+	put_cpu_var(fault_rnd_state);
+
+	return res % 100;
+}
+
 /*
  * setup_fault_attr() is a helper function for various __setup handlers, so it
  * returns 0 on error, because that is what __setup handlers do.
@@ -31,6 +49,8 @@ int setup_fault_attr(struct fault_attr *attr, char *str)
 		return 0;
 	}
 
+	prandom_init_once(&fault_rnd_state);
+
 	attr->probability = probability;
 	attr->interval = interval;
 	atomic_set(&attr->times, times);
@@ -146,7 +166,7 @@ bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags)
 			return false;
 	}
 
-	if (attr->probability <= get_random_u32_below(100))
+	if (attr->probability <= fault_prandom_u32_below_100())
 		return false;
 
 fail:
@@ -219,6 +239,8 @@ struct dentry *fault_create_debugfs_attr(const char *name,
 	if (IS_ERR(dir))
 		return dir;
 
+	prandom_init_once(&fault_rnd_state);
+
 	debugfs_create_ul("probability", mode, dir, &attr->probability);
 	debugfs_create_ul("interval", mode, dir, &attr->interval);
 	debugfs_create_atomic_t("times", mode, dir, &attr->times);
@@ -431,6 +453,8 @@ static const struct config_item_type fault_config_type = {
 
 void fault_config_init(struct fault_config *config, const char *name)
 {
+	prandom_init_once(&fault_rnd_state);
+
 	config_group_init_type_name(&config->group, name, &fault_config_type);
 }
 EXPORT_SYMBOL_GPL(fault_config_init);
-- 
2.34.1

On Sun, Dec 08, 2024 at 11:24:15PM +0900, Akinobu Mita wrote:
> Currently get_random*() is used to determine the probability of fault
> injection, but cryptographically secure random numbers are not required.
> 
> There is no big problem in using prandom instead of get_random*() to
> determine the probability of fault injection, and it also avoids acquiring
> a spinlock, which is unsafe in some contexts.
> 
> Link: https://lore.kernel.org/lkml/20241129120939.GG35539@noisy.programming.kicks-ass.net
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  lib/fault-inject.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/fault-inject.c b/lib/fault-inject.c
> index 52eb6ba29698..92a54c8a8380 100644
> --- a/lib/fault-inject.c
> +++ b/lib/fault-inject.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> -#include <linux/random.h>
> +#include <linux/prandom.h>
>  #include <linux/debugfs.h>
>  #include <linux/sched.h>
>  #include <linux/stat.h>
> @@ -12,6 +12,24 @@
>  #include <linux/stacktrace.h>
>  #include <linux/fault-inject.h>
>  
> +/*
> + * The should_fail() use prandom instead of the normal Linux RNG since they don't
> + * need cryptographically secure random numbers.
> + */
> +static DEFINE_PER_CPU(struct rnd_state, fault_rnd_state);
> +
> +static u32 fault_prandom_u32_below_100(void)
> +{
> +	struct rnd_state *state;
> +	u32 res;
> +
> +	state = &get_cpu_var(fault_rnd_state);
> +	res = prandom_u32_state(state);
> +	put_cpu_var(fault_rnd_state);
> +
> +	return res % 100;

This is a bit unfortunate, the remainder function is not uniform (for
non-powers-of-two). I mean, nobody cares, and certainly here it doesn't
matter, but *groan*.

The problem seems to be that this ->probability thing used throughout
this code seems to be a percentage, while typically the probability is
expressed as a real number between 0 and 1.

<rant> For reasons not understood by me, and probably related to education
systems, some people think percentages are 'special' and not just a
1/100 fraction. </rant>

I feel this code would be improved if instead of a percentage value, the
probability would be represented as a fixed point value 0<=p<=1 with a
power-of-two base.

Eg. attr->probability = 1024*probability/100;

> +}
> +
>  /*
>   * setup_fault_attr() is a helper function for various __setup handlers, so it
>   * returns 0 on error, because that is what __setup handlers do.
> @@ -31,6 +49,8 @@ int setup_fault_attr(struct fault_attr *attr, char *str)
>  		return 0;
>  	}
>  
> +	prandom_init_once(&fault_rnd_state);
> +
>  	attr->probability = probability;
>  	attr->interval = interval;
>  	atomic_set(&attr->times, times);
> @@ -146,7 +166,7 @@ bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags)
>  			return false;
>  	}
>  
> -	if (attr->probability <= get_random_u32_below(100))
> +	if (attr->probability <= fault_prandom_u32_below_100())
>  		return false;
>  
>  fail:
> @@ -219,6 +239,8 @@ struct dentry *fault_create_debugfs_attr(const char *name,
>  	if (IS_ERR(dir))
>  		return dir;
>  
> +	prandom_init_once(&fault_rnd_state);
> +
>  	debugfs_create_ul("probability", mode, dir, &attr->probability);
>  	debugfs_create_ul("interval", mode, dir, &attr->interval);
>  	debugfs_create_atomic_t("times", mode, dir, &attr->times);
> @@ -431,6 +453,8 @@ static const struct config_item_type fault_config_type = {
>  
>  void fault_config_init(struct fault_config *config, const char *name)
>  {
> +	prandom_init_once(&fault_rnd_state);
> +
>  	config_group_init_type_name(&config->group, name, &fault_config_type);
>  }
>  EXPORT_SYMBOL_GPL(fault_config_init);

Anyway, yes, this cures the random problem. Thanks!

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>