On 21/06/2024 11:54, Harald Freudenberger wrote:
> When there are rng sources registering at the hwrng core via
> hwrng_register() a struct hwrng is delivered. There is a quality
> field in there which is used to decide which of the registered
> hw rng sources will be used by the hwrng core.
>
> With commit 16bdbae39428 ("hwrng: core - treat default_quality as
> a maximum and default to 1024") there came in a new default of
> 1024 in case this field is empty and all the known hw rng sources
> at that time had been reworked to not fill this field and thus
> use the default of 1024.
>
> The code choosing the 'better' hw rng source during registration
> of a new hw rng source has never been adapted to this and thus
> used 0 if the hw rng implementation does not fill the quality field.
> So when two rng sources register, one with 0 (meaning 1024) and
> the other one with 999, the 999 hw rng will be chosen.
>
> This patch simple takes into account that a quality field value
> of 0 is to be treated as 1024 and then the decision about which
> hw rng to use works as expected.
>
> Tested on s390 with two hardware rng sources: crypto cards and
> trng true random generator device driver.
>
> Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>
>
> Reported-by: Christian Rund <Christian.Rund@xxxxxxxxxx>
> Fixes: 16bdbae39428 ("hwrng: core - treat default_quality as a maximum and default to 1024")
> ---
> drivers/char/hw_random/core.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 4084df65c9fa..993b8a1f1d19 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -525,6 +525,7 @@ static int hwrng_fillfn(void *unused)
>
> int hwrng_register(struct hwrng *rng)
> {
> + unsigned short rng_quality, cur_quality;
In my opinion, we no not need these variables.
> int err = -EINVAL;
> struct hwrng *tmp;
>
> @@ -545,8 +546,14 @@ int hwrng_register(struct hwrng *rng)
> complete(&rng->cleanup_done);
> init_completion(&rng->dying);
>
> + /* Quality field not set in struct hwrng means 1024 */
> + rng_quality = rng->quality ? rng->quality : 1024;
Please use the shortcut "(a) ?: (b)" for "(a) ? (a) : (b)", also remove non-necessary parenthesis.
rng_quality = rng->quality ?: 1024;
Because this variable is only used once, you can also change it directly in the if statement below.
> + cur_quality = current_rng ?
> + (current_rng->quality ? current_rng->quality : 1024) :
> + 0;
> +
This one is not necessary. The quality field of current_rng is has been updated already by the hwrng_init() function.
> if (!current_rng ||
> - (!cur_rng_set_by_user && rng->quality > current_rng->quality)) {
> + (!cur_rng_set_by_user && rng_quality > cur_quality)) {
Unfortunately, the quality field of rng is read here, before the quality field is updated by hwrng_init().
Maybe we can use the following:
if (!current_rng ||
(!cur_rng_set_by_user && (rng->quality ?: 1024) > current_rng->quality)) {
> /*
> * Set new rng as current as the new rng source
> * provides better entropy quality and was not
--
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@xxxxxxxxxxxxx
