[PATCH] hwrng: do not warn when there are no devices

[PATCH] hwrng: do not warn when there are no devices

[Mike Frysinger]

From: Mike Frysinger <vapier@chromium.org>

If you build in hwrng & tpm-rng, but boot on a system that doesn't
have a tpm (like via KVM), hwrng will spam the logs every 10 seconds
with the line:
	hwrng: no data available

This isn't terribly useful, so squelch the error in the ENODEV case.
For all other errors, we still warn, and include the actual error.

Signed-off-by: Mike Frysinger <vapier@chromium.org>
---
 drivers/char/hw_random/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 503a41dfa193..da24bd5a9902 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -392,7 +392,8 @@ static int hwrng_fillfn(void *unused)
 		mutex_unlock(&reading_mutex);
 		put_rng(rng);
 		if (rc <= 0) {
-			pr_warn("hwrng: no data available\n");
+			if (rc != -ENODEV)
+				pr_warn("hwrng: no data available: %li\n", rc);
 			msleep_interruptible(10000);
 			continue;
 		}
-- 
2.12.0

Re: [PATCH] hwrng: do not warn when there are no devices

[PrasannaKumar Muralidharan]

On 12 May 2017 at 09:47, Mike Frysinger <vapier@gentoo.org> wrote:
> From: Mike Frysinger <vapier@chromium.org>
>
> If you build in hwrng & tpm-rng, but boot on a system that doesn't
> have a tpm (like via KVM), hwrng will spam the logs every 10 seconds
> with the line:
>         hwrng: no data available
>
> This isn't terribly useful, so squelch the error in the ENODEV case.
> For all other errors, we still warn, and include the actual error.

This patch removes the logging but does not fix the real problem.
Better method would be to start the hwrng_fillfn thread when first rng
provider registers and stop it when the last rng provider unregisters.

> Signed-off-by: Mike Frysinger <vapier@chromium.org>
> ---
>  drivers/char/hw_random/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 503a41dfa193..da24bd5a9902 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -392,7 +392,8 @@ static int hwrng_fillfn(void *unused)
>                 mutex_unlock(&reading_mutex);
>                 put_rng(rng);
>                 if (rc <= 0) {
> -                       pr_warn("hwrng: no data available\n");
> +                       if (rc != -ENODEV)
> +                               pr_warn("hwrng: no data available: %li\n", rc);
>                         msleep_interruptible(10000);
>                         continue;
>                 }
> --
> 2.12.0
>

Re: [PATCH] hwrng: do not warn when there are no devices

[Mike Frysinger]

On Fri, May 12, 2017 at 2:15 AM, PrasannaKumar Muralidharan wrote:
> On 12 May 2017 at 09:47, Mike Frysinger <vapier@gentoo.org> wrote:
> > From: Mike Frysinger <vapier@chromium.org>
> >
> > If you build in hwrng & tpm-rng, but boot on a system that doesn't
> > have a tpm (like via KVM), hwrng will spam the logs every 10 seconds
> > with the line:
> >         hwrng: no data available
> >
> > This isn't terribly useful, so squelch the error in the ENODEV case.
> > For all other errors, we still warn, and include the actual error.
>
> This patch removes the logging but does not fix the real problem.
> Better method would be to start the hwrng_fillfn thread when first rng
> provider registers and stop it when the last rng provider unregisters.

what you describe is already implemented in the hw random code.  the
kthread only starts up when a registration happens, and will stop it
when the last rng unregisters itself.

the issue is that tpm-rng has registered itself here, but there aren't
any tpm devices available.  so it returns ENODEV.
-mike

Re: [PATCH] hwrng: do not warn when there are no devices

[PrasannaKumar Muralidharan]

On 12 May 2017 at 12:11, Mike Frysinger <vapier@chromium.org> wrote:
> On Fri, May 12, 2017 at 2:15 AM, PrasannaKumar Muralidharan wrote:
>> On 12 May 2017 at 09:47, Mike Frysinger <vapier@gentoo.org> wrote:
>> > From: Mike Frysinger <vapier@chromium.org>
>> >
>> > If you build in hwrng & tpm-rng, but boot on a system that doesn't
>> > have a tpm (like via KVM), hwrng will spam the logs every 10 seconds
>> > with the line:
>> >         hwrng: no data available
>> >
>> > This isn't terribly useful, so squelch the error in the ENODEV case.
>> > For all other errors, we still warn, and include the actual error.

If the boot system does not have a tpm I think registering tpm-rng is
not useful. On tpm-rng load instead of registering with hwrng a check
can be made whether the system supports tpm. Is this possible?

>> This patch removes the logging but does not fix the real problem.
>> Better method would be to start the hwrng_fillfn thread when first rng
>> provider registers and stop it when the last rng provider unregisters.
>
> what you describe is already implemented in the hw random code.  the
> kthread only starts up when a registration happens, and will stop it
> when the last rng unregisters itself.
>
> the issue is that tpm-rng has registered itself here, but there aren't
> any tpm devices available.  so it returns ENODEV.

Missed it. Please see if the above comment can be addressed.

Thanks,
PrasannaKumar

Re: [PATCH] hwrng: do not warn when there are no devices

[PrasannaKumar Muralidharan]

On 12 May 2017 at 12:22, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> wrote:
> On 12 May 2017 at 12:11, Mike Frysinger <vapier@chromium.org> wrote:
>> On Fri, May 12, 2017 at 2:15 AM, PrasannaKumar Muralidharan wrote:
>>> On 12 May 2017 at 09:47, Mike Frysinger <vapier@gentoo.org> wrote:
>>> > From: Mike Frysinger <vapier@chromium.org>
>>> >
>>> > If you build in hwrng & tpm-rng, but boot on a system that doesn't
>>> > have a tpm (like via KVM), hwrng will spam the logs every 10 seconds
>>> > with the line:
>>> >         hwrng: no data available
>>> >
>>> > This isn't terribly useful, so squelch the error in the ENODEV case.
>>> > For all other errors, we still warn, and include the actual error.
>
> If the boot system does not have a tpm I think registering tpm-rng is
> not useful. On tpm-rng load instead of registering with hwrng a check
> can be made whether the system supports tpm. Is this possible?

Completely untested patch below. Will something like this work?

diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
index d6d4482..f78f8ca 100644
--- a/drivers/char/hw_random/tpm-rng.c
+++ b/drivers/char/hw_random/tpm-rng.c
@@ -35,7 +35,13 @@ static int tpm_rng_read(struct hwrng *rng, void
*data, size_t max, bool wait)

 static int __init rng_init(void)
 {
-       return hwrng_register(&tpm_rng);
+       struct tpm_chip *tpm_rng_chip = tpm_chip_find_get(TPM_ANY_NUM);
+       if (tpm_chip) {
+               tpm_put_ops(tpm_rng_chip);
+               return hwrng_register(&tpm_rng);
+       }
+
+       return -ENODEV;
 }
 module_init(rng_init);

Thanks,
PrasannaKumar

Re: [PATCH] hwrng: do not warn when there are no devices

[Mike Frysinger]

On Fri, May 12, 2017 at 3:06 AM, PrasannaKumar Muralidharan wrote:
> On 12 May 2017 at 12:22, PrasannaKumar Muralidharan wrote:
> > On 12 May 2017 at 12:11, Mike Frysinger wrote:
> >> On Fri, May 12, 2017 at 2:15 AM, PrasannaKumar Muralidharan wrote:
> >>> On 12 May 2017 at 09:47, Mike Frysinger wrote:
> >>> > From: Mike Frysinger <vapier@chromium.org>
> >>> >
> >>> > If you build in hwrng & tpm-rng, but boot on a system that doesn't
> >>> > have a tpm (like via KVM), hwrng will spam the logs every 10 seconds
> >>> > with the line:
> >>> >         hwrng: no data available
> >>> >
> >>> > This isn't terribly useful, so squelch the error in the ENODEV case.
> >>> > For all other errors, we still warn, and include the actual error.
> >
> > If the boot system does not have a tpm I think registering tpm-rng is
> > not useful. On tpm-rng load instead of registering with hwrng a check
> > can be made whether the system supports tpm. Is this possible?
>
> Completely untested patch below. Will something like this work?
>
> --- a/drivers/char/hw_random/tpm-rng.c
> +++ b/drivers/char/hw_random/tpm-rng.c
> @@ -35,7 +35,13 @@ static int tpm_rng_read(struct hwrng *rng, void
> *data, size_t max, bool wait)
>
>  static int __init rng_init(void)
>  {
> -       return hwrng_register(&tpm_rng);
> +       struct tpm_chip *tpm_rng_chip = tpm_chip_find_get(TPM_ANY_NUM);
> +       if (tpm_chip) {
> +               tpm_put_ops(tpm_rng_chip);
> +               return hwrng_register(&tpm_rng);
> +       }
> +
> +       return -ENODEV;
>  }
>  module_init(rng_init);

keep in mind that TPMs are often on slow buses like I2C, so i suspect
rng_init runs before those have been initialized.  so this patch would
break them.

it would also break if the tpm drivers are modules that don't get
loaded until later, but tpm-rng is built in.  or tpm-rng is loaded
first.
-mike

Re: [PATCH] hwrng: do not warn when there are no devices

[PrasannaKumar Muralidharan]

On 12 May 2017 at 13:17, Mike Frysinger <vapier@chromium.org> wrote:
>> Completely untested patch below. Will something like this work?
>>
>> --- a/drivers/char/hw_random/tpm-rng.c
>> +++ b/drivers/char/hw_random/tpm-rng.c
>> @@ -35,7 +35,13 @@ static int tpm_rng_read(struct hwrng *rng, void
>> *data, size_t max, bool wait)
>>
>>  static int __init rng_init(void)
>>  {
>> -       return hwrng_register(&tpm_rng);
>> +       struct tpm_chip *tpm_rng_chip = tpm_chip_find_get(TPM_ANY_NUM);
>> +       if (tpm_chip) {
>> +               tpm_put_ops(tpm_rng_chip);
>> +               return hwrng_register(&tpm_rng);
>> +       }
>> +
>> +       return -ENODEV;
>>  }
>>  module_init(rng_init);
>
> keep in mind that TPMs are often on slow buses like I2C, so i suspect
> rng_init runs before those have been initialized.  so this patch would
> break them.
>
> it would also break if the tpm drivers are modules that don't get
> loaded until later, but tpm-rng is built in.  or tpm-rng is loaded
> first.

Hmm. I am not aware of the tpm hardware or driver behavior. Based on
your explanation I see that this patch is not useful. It looks like it
is possible to detect the presence of tpm device and call
hwrng_register once the corresponding driver is loaded.

I leave it to Herbert to decide whether to accept this patch in
current form or not.

Regardless of whether this patch gets accepted or not I can work on a
better fix if you can provide instructions on how to setup and use
tpm. But that will be only after a couple of months.

Regards,
PrasannaKumar

Re: [PATCH] hwrng: do not warn when there are no devices

[Herbert Xu]

On Fri, May 12, 2017 at 01:49:52PM +0530, PrasannaKumar Muralidharan wrote:
>
> I leave it to Herbert to decide whether to accept this patch in
> current form or not.

I think the correct fix would be for the TPM subsystem to signal that
it is ready and then register the tpm-rng device.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] hwrng: do not warn when there are no devices

[Mike Frysinger]

On Sun, Jun 18, 2017 at 9:12 PM, Herbert Xu wrote:
> On Fri, May 12, 2017 at 01:49:52PM +0530, PrasannaKumar Muralidharan wrote:
> > I leave it to Herbert to decide whether to accept this patch in
> > current form or not.
>
> I think the correct fix would be for the TPM subsystem to signal that
> it is ready and then register the tpm-rng device.

the TPM subsystem is ready.  it's like saying "the USB subsystem
should signal when it's ready".  the TPM subsystem provides a bus (of
sorts) and clients (like tpm-rng) can use whatever backend happens to
be available.

in order to make tpm-rng react in the way you're implying, the TPM
subsystem would need to add a notification chain for transitions from
none<->some devices, then tpm-rng could subscribe to that, and during
those transition points, it would call hwrng_register/hwrng_unregister
to make itself visible accordingly to the hwrng subsystem.  maybe
someone on the TPM side would be interested in writing all that logic,
but it sounds excessive for this minor usage.  the current tpm-rng
driver is *extremely* simple -- it's 3 funcs, each of which are 1
line.
-mike

Re: [PATCH] hwrng: do not warn when there are no devices

[Herbert Xu]

On Sun, Jun 18, 2017 at 10:00:17PM -0700, Mike Frysinger wrote:
> 
> in order to make tpm-rng react in the way you're implying, the TPM
> subsystem would need to add a notification chain for transitions from
> none<->some devices, then tpm-rng could subscribe to that, and during
> those transition points, it would call hwrng_register/hwrng_unregister
> to make itself visible accordingly to the hwrng subsystem.  maybe
> someone on the TPM side would be interested in writing all that logic,
> but it sounds excessive for this minor usage.  the current tpm-rng
> driver is *extremely* simple -- it's 3 funcs, each of which are 1
> line.

It's simple and it's broken, as far as the way it hooks into the
hwrng is concerned.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] hwrng: do not warn when there are no devices

[PrasannaKumar Muralidharan]

On 19 June 2017 at 11:51, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Sun, Jun 18, 2017 at 10:00:17PM -0700, Mike Frysinger wrote:
>>
>> in order to make tpm-rng react in the way you're implying, the TPM
>> subsystem would need to add a notification chain for transitions from
>> none<->some devices, then tpm-rng could subscribe to that, and during
>> those transition points, it would call hwrng_register/hwrng_unregister
>> to make itself visible accordingly to the hwrng subsystem.  maybe
>> someone on the TPM side would be interested in writing all that logic,
>> but it sounds excessive for this minor usage.  the current tpm-rng
>> driver is *extremely* simple -- it's 3 funcs, each of which are 1
>> line.
>
> It's simple and it's broken, as far as the way it hooks into the
> hwrng is concerned.

*********************************************************************************************
diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
index d6d4482..4861b35 100644
--- a/drivers/char/hw_random/tpm-rng.c
+++ b/drivers/char/hw_random/tpm-rng.c
@@ -22,6 +22,10 @@
 #include <linux/tpm.h>

 #define MODULE_NAME "tpm-rng"
+#define MAX_RETRIES 30
+
+static struct delayed_work check_tpm_work;
+static int retry_count;

 static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
@@ -33,9 +37,27 @@ static struct hwrng tpm_rng = {
        .read = tpm_rng_read,
 };

+static void check_tpm_presence(struct work_struct *work)
+{
+       u8 data = 0;
+       if (tpm_get_random(TPM_ANY_NUM, &data, 1) > 0) {
+               hwrng_register(&tpm_rng);
+       } else {
+               if (retry_count < MAX_RETRIES) {
+                       retry_count++;
+                       schedule_delayed_work(&check_tpm_work, HZ * 10);
+               } else {
+                       pr_err("Could not find any TPM chip, not
registering rng");
+               }
+       }
+}
+
 static int __init rng_init(void)
 {
-       return hwrng_register(&tpm_rng);
+       INIT_DELAYED_WORK(&check_tpm_work, check_tpm_presence);
+       check_tpm_presence(NULL);
+
+       return 0;
 }
 module_init(rng_init);
*********************************************************************************************

Why not something like this? Patch is completely untested. If this
idea seems useful I can clean the code but would require help in
testing.

Regards,
PrasannaKumar

Re: [PATCH] hwrng: do not warn when there are no devices

[Mike Frysinger]

On Mon, Jun 19, 2017 at 2:43 AM, PrasannaKumar Muralidharan wrote:
> On 19 June 2017 at 11:51, Herbert Xu wrote:
>> On Sun, Jun 18, 2017 at 10:00:17PM -0700, Mike Frysinger wrote:
>>>
>>> in order to make tpm-rng react in the way you're implying, the TPM
>>> subsystem would need to add a notification chain for transitions from
>>> none<->some devices, then tpm-rng could subscribe to that, and during
>>> those transition points, it would call hwrng_register/hwrng_unregister
>>> to make itself visible accordingly to the hwrng subsystem.  maybe
>>> someone on the TPM side would be interested in writing all that logic,
>>> but it sounds excessive for this minor usage.  the current tpm-rng
>>> driver is *extremely* simple -- it's 3 funcs, each of which are 1
>>> line.
>>
>> It's simple and it's broken, as far as the way it hooks into the
>> hwrng is concerned.
>
> *********************************************************************************************
> diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
> index d6d4482..4861b35 100644
> --- a/drivers/char/hw_random/tpm-rng.c
> +++ b/drivers/char/hw_random/tpm-rng.c
> @@ -22,6 +22,10 @@
>  #include <linux/tpm.h>
>
>  #define MODULE_NAME "tpm-rng"
> +#define MAX_RETRIES 30
> +
> +static struct delayed_work check_tpm_work;
> +static int retry_count;
>
>  static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>  {
> @@ -33,9 +37,27 @@ static struct hwrng tpm_rng = {
>         .read = tpm_rng_read,
>  };
>
> +static void check_tpm_presence(struct work_struct *work)
> +{
> +       u8 data = 0;
> +       if (tpm_get_random(TPM_ANY_NUM, &data, 1) > 0) {
> +               hwrng_register(&tpm_rng);
> +       } else {
> +               if (retry_count < MAX_RETRIES) {
> +                       retry_count++;
> +                       schedule_delayed_work(&check_tpm_work, HZ * 10);
> +               } else {
> +                       pr_err("Could not find any TPM chip, not
> registering rng");
> +               }
> +       }
> +}
> +
>  static int __init rng_init(void)
>  {
> -       return hwrng_register(&tpm_rng);
> +       INIT_DELAYED_WORK(&check_tpm_work, check_tpm_presence);
> +       check_tpm_presence(NULL);
> +
> +       return 0;
>  }
>  module_init(rng_init);
> *********************************************************************************************
>
> Why not something like this? Patch is completely untested. If this
> idea seems useful I can clean the code but would require help in
> testing.

first, that's not how deferred device probing works in the kernel.
drivers shouldn't be doing their own sleeping.  but we can ignore that
because no amount of delay/retries will work -- TPMs can come & go at
anytime via hotplugging or module loading/unloading.  so the only way
to pull it off would be to do something like what i described --
extending the tpm framework so that it can signal children to come
up/go down.

imo, standing all of that up is over-engineering and not worth the
effort, so i'm not going to do it.  but maybe you can convince some of
the TPM maintainers it's worthwhile.
-mike

Re: [PATCH] hwrng: do not warn when there are no devices

[PrasannaKumar Muralidharan]

On 20 June 2017 at 00:33, Mike Frysinger <vapier@chromium.org> wrote:
> On Mon, Jun 19, 2017 at 2:43 AM, PrasannaKumar Muralidharan wrote:
>> On 19 June 2017 at 11:51, Herbert Xu wrote:
>>> On Sun, Jun 18, 2017 at 10:00:17PM -0700, Mike Frysinger wrote:
>>>>
>>>> in order to make tpm-rng react in the way you're implying, the TPM
>>>> subsystem would need to add a notification chain for transitions from
>>>> none<->some devices, then tpm-rng could subscribe to that, and during
>>>> those transition points, it would call hwrng_register/hwrng_unregister
>>>> to make itself visible accordingly to the hwrng subsystem.  maybe
>>>> someone on the TPM side would be interested in writing all that logic,
>>>> but it sounds excessive for this minor usage.  the current tpm-rng
>>>> driver is *extremely* simple -- it's 3 funcs, each of which are 1
>>>> line.
>>>
>>> It's simple and it's broken, as far as the way it hooks into the
>>> hwrng is concerned.
>>
>> *********************************************************************************************
>> diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
>> index d6d4482..4861b35 100644
>> --- a/drivers/char/hw_random/tpm-rng.c
>> +++ b/drivers/char/hw_random/tpm-rng.c
>> @@ -22,6 +22,10 @@
>>  #include <linux/tpm.h>
>>
>>  #define MODULE_NAME "tpm-rng"
>> +#define MAX_RETRIES 30
>> +
>> +static struct delayed_work check_tpm_work;
>> +static int retry_count;
>>
>>  static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>>  {
>> @@ -33,9 +37,27 @@ static struct hwrng tpm_rng = {
>>         .read = tpm_rng_read,
>>  };
>>
>> +static void check_tpm_presence(struct work_struct *work)
>> +{
>> +       u8 data = 0;
>> +       if (tpm_get_random(TPM_ANY_NUM, &data, 1) > 0) {
>> +               hwrng_register(&tpm_rng);
>> +       } else {
>> +               if (retry_count < MAX_RETRIES) {
>> +                       retry_count++;
>> +                       schedule_delayed_work(&check_tpm_work, HZ * 10);
>> +               } else {
>> +                       pr_err("Could not find any TPM chip, not
>> registering rng");
>> +               }
>> +       }
>> +}
>> +
>>  static int __init rng_init(void)
>>  {
>> -       return hwrng_register(&tpm_rng);
>> +       INIT_DELAYED_WORK(&check_tpm_work, check_tpm_presence);
>> +       check_tpm_presence(NULL);
>> +
>> +       return 0;
>>  }
>>  module_init(rng_init);
>> *********************************************************************************************
>>
>> Why not something like this? Patch is completely untested. If this
>> idea seems useful I can clean the code but would require help in
>> testing.
>
> first, that's not how deferred device probing works in the kernel.
> drivers shouldn't be doing their own sleeping.  but we can ignore that
> because no amount of delay/retries will work -- TPMs can come & go at
> anytime via hotplugging or module loading/unloading.  so the only way
> to pull it off would be to do something like what i described --
> extending the tpm framework so that it can signal children to come
> up/go down.

If TPM can come and go then notification or callback is the correct
way to handle this case.

> imo, standing all of that up is over-engineering and not worth the
> effort, so i'm not going to do it.  but maybe you can convince some of
> the TPM maintainers it's worthwhile.

Okay.

Thanks,
PrasannaKumar