Re: [PATCH] hwrng: do not warn when there are no devices
On 20 June 2017 at 00:33, Mike Frysinger 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 >> >> #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
Re: [PATCH] hwrng: do not warn when there are no devices
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 > > #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
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 #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
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 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
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
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 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
On 12 May 2017 at 13:17, Mike Frysinger 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
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 > >>> > > >>> > 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
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 >>> > >>> > 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
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 >> > >> > 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
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 > > > > 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
On 12 May 2017 at 09:47, Mike Frysinger wrote: > From: Mike Frysinger > > 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 > --- > 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(1); > continue; > } > -- > 2.12.0 >