| Message ID | 8242305.3NQCWfJAlD@positron.chronox.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
On Tue, Jun 21, 2016 at 11:05 AM, Stephan Mueller <smueller@chronox.de> wrote: > As part of the Y2038 development, __getnstimeofday is not supposed to be > used any more. It is now replaced with ktime_get_raw_ns. Albeit > ktime_get_raw_ns is monotonic compared to __getnstimeofday, this > difference is irrelevant as the Jitter RNG uses the time stamp to > measure the execution time of a given code path and tries to detect > variations in the execution time. Therefore, the only requirement the > Jitter RNG has, is a sufficient high resolution to detect these > variations. > > The change was tested on x86 to show an identical behavior as RDTSC. The > used test code simply measures the execution time of the heart of the > RNG: > > jent_get_nstime(&time); > jent_memaccess(ec, min); > jent_fold_time(NULL, time, &folded, min); > jent_get_nstime(&time2); > return ((time2 - time)); > > Signed-off-by: Stephan Mueller <smueller@chronox.de> > --- > crypto/jitterentropy-kcapi.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/crypto/jitterentropy-kcapi.c b/crypto/jitterentropy-kcapi.c > index 597cedd..69a2988 100644 > --- a/crypto/jitterentropy-kcapi.c > +++ b/crypto/jitterentropy-kcapi.c > @@ -87,24 +87,29 @@ void jent_memcpy(void *dest, const void *src, unsigned int n) > memcpy(dest, src, n); > } > > +/* > + * Obtain a high-resolution time stamp value. The time stamp is used to measure > + * the execution time of a given code path and its variations. Hence, the time > + * stamp must have a sufficiently high resolution. It is valid if the time > + * runs backwards for short period of time as the RNG code is able handle that. > + * > + * Note, if the function returns zero because a given architecture does not > + * implement a high-resolution time stamp, the RNG code's runtime test > + * will detect it and will not produce output. > + */ > void jent_get_nstime(__u64 *out) > { > - struct timespec ts; > __u64 tmp = 0; > > tmp = random_get_entropy(); > > /* > - * If random_get_entropy does not return a value (which is possible on, > - * for example, MIPS), invoke __getnstimeofday > + * If random_get_entropy does not return a value, i.e. it is not > + * implemented for a given architecture, invoke ktime_get_raw_ns > * hoping that there are timers we can work with. > */ > - if ((0 == tmp) && > - (0 == __getnstimeofday(&ts))) { > - tmp = ts.tv_sec; > - tmp = tmp << 32; > - tmp = tmp | ts.tv_nsec; > - } > + if (tmp == 0) > + tmp = ktime_get_raw_ns(); I don't see in the above an explanation of *why* you're using ktime_get_raw_ns() instead of ktime_get_ns(). Also the bit about time running backwards being ok is confusing since you're not using the "fast" accessor where that would be a risk. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Dienstag, 21. Juni 2016, 11:11:42 schrieb John Stultz: Hi John, > I don't see in the above an explanation of *why* you're using > ktime_get_raw_ns() instead of ktime_get_ns(). Could you help me understand what the difference is or point me to some documentation? I understood that we only talked about the _raw variant. > > Also the bit about time running backwards being ok is confusing since > you're not using the "fast" accessor where that would be a risk. Ok, if the running backwards hint is only applicable to the _fast variants, I can certainly drop it. Thanks Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 21, 2016 at 11:49 AM, Stephan Mueller <smueller@chronox.de> wrote: > Am Dienstag, 21. Juni 2016, 11:11:42 schrieb John Stultz: > > Hi John, > >> I don't see in the above an explanation of *why* you're using >> ktime_get_raw_ns() instead of ktime_get_ns(). > > Could you help me understand what the difference is or point me to some > documentation? I understood that we only talked about the _raw variant. Using specialized interfaces with subtle semantics w/o understanding them is sort of my concern here. There are reasons why you might want to use the ktime_get_raw_ns() interface over ktime_get_ns(), but they have not been made clear in the comment. Arnd discussed some potential concerns that the freq adjustment done by ntp might be somewhat predictable/controlled by remote parties, which could have some effect in the calculation. That feels a little overly vague to me, but I'm no crypto expert, so if that is a reasonable concern, then it should be a conscious and documented decision. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, June 21, 2016 12:05:06 PM CEST John Stultz wrote: > On Tue, Jun 21, 2016 at 11:49 AM, Stephan Mueller <smueller@chronox.de> wrote: > > Am Dienstag, 21. Juni 2016, 11:11:42 schrieb John Stultz: > > > > Hi John, > > > >> I don't see in the above an explanation of *why* you're using > >> ktime_get_raw_ns() instead of ktime_get_ns(). > > > > Could you help me understand what the difference is or point me to some > > documentation? I understood that we only talked about the _raw variant. > > Using specialized interfaces with subtle semantics w/o understanding > them is sort of my concern here. > > There are reasons why you might want to use the ktime_get_raw_ns() > interface over ktime_get_ns(), but they have not been made clear in > the comment. Arnd discussed some potential concerns that the freq > adjustment done by ntp might be somewhat predictable/controlled by > remote parties, which could have some effect in the calculation. That > feels a little overly vague to me, but I'm no crypto expert, so if > that is a reasonable concern, then it should be a conscious and > documented decision. My original patch changed __getnstimeofday() to __getnstimeofday64(), which kept the original semantics of not warning in case the clock source is suspended (which is the only different to the normal getnstimeofday{,64}(). I did the patch a while time ago along with a number of other patches that I never sent out until last week, so I don't remember the reasoning for suggesting ktime_get_raw_fast_ns() over ktime_get_raw_ns(), but I sure wanted to keep the non-warning behavior, and ktime_get_ns() warns on timekeeping_suspended() while the other two don't. If we don't care about the non-warning aspect, ktime_get_ns() makes most sense here, and the original code should probably have used getnstimeofday() as well. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 21, 2016 at 12:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday, June 21, 2016 12:05:06 PM CEST John Stultz wrote: >> On Tue, Jun 21, 2016 at 11:49 AM, Stephan Mueller <smueller@chronox.de> wrote: >> > Am Dienstag, 21. Juni 2016, 11:11:42 schrieb John Stultz: >> > >> > Hi John, >> > >> >> I don't see in the above an explanation of *why* you're using >> >> ktime_get_raw_ns() instead of ktime_get_ns(). >> > >> > Could you help me understand what the difference is or point me to some >> > documentation? I understood that we only talked about the _raw variant. >> >> Using specialized interfaces with subtle semantics w/o understanding >> them is sort of my concern here. >> >> There are reasons why you might want to use the ktime_get_raw_ns() >> interface over ktime_get_ns(), but they have not been made clear in >> the comment. Arnd discussed some potential concerns that the freq >> adjustment done by ntp might be somewhat predictable/controlled by >> remote parties, which could have some effect in the calculation. That >> feels a little overly vague to me, but I'm no crypto expert, so if >> that is a reasonable concern, then it should be a conscious and >> documented decision. > > My original patch changed __getnstimeofday() to __getnstimeofday64(), > which kept the original semantics of not warning in case the clock > source is suspended (which is the only different to the normal > getnstimeofday{,64}(). > > I did the patch a while time ago along with a number of other patches > that I never sent out until last week, so I don't remember the > reasoning for suggesting ktime_get_raw_fast_ns() over ktime_get_raw_ns(), > but I sure wanted to keep the non-warning behavior, and ktime_get_ns() > warns on timekeeping_suspended() while the other two don't. Right. But this code isn't called in late suspend or early resume is it? > If we don't care about the non-warning aspect, ktime_get_ns() makes > most sense here, and the original code should probably have used > getnstimeofday() as well. This is what I suspect as well. But again, I don't mind if folks use the specialized interfaces, as long as they document a clear reason for it. Especially for things like crypto where intuition isn't always the best guide. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/crypto/jitterentropy-kcapi.c b/crypto/jitterentropy-kcapi.c index 597cedd..69a2988 100644 --- a/crypto/jitterentropy-kcapi.c +++ b/crypto/jitterentropy-kcapi.c @@ -87,24 +87,29 @@ void jent_memcpy(void *dest, const void *src, unsigned int n) memcpy(dest, src, n); } +/* + * Obtain a high-resolution time stamp value. The time stamp is used to measure + * the execution time of a given code path and its variations. Hence, the time + * stamp must have a sufficiently high resolution. It is valid if the time + * runs backwards for short period of time as the RNG code is able handle that. + * + * Note, if the function returns zero because a given architecture does not + * implement a high-resolution time stamp, the RNG code's runtime test + * will detect it and will not produce output. + */ void jent_get_nstime(__u64 *out) { - struct timespec ts; __u64 tmp = 0; tmp = random_get_entropy(); /* - * If random_get_entropy does not return a value (which is possible on, - * for example, MIPS), invoke __getnstimeofday + * If random_get_entropy does not return a value, i.e. it is not + * implemented for a given architecture, invoke ktime_get_raw_ns * hoping that there are timers we can work with. */ - if ((0 == tmp) && - (0 == __getnstimeofday(&ts))) { - tmp = ts.tv_sec; - tmp = tmp << 32; - tmp = tmp | ts.tv_nsec; - } + if (tmp == 0) + tmp = ktime_get_raw_ns(); *out = tmp; }
As part of the Y2038 development, __getnstimeofday is not supposed to be used any more. It is now replaced with ktime_get_raw_ns. Albeit ktime_get_raw_ns is monotonic compared to __getnstimeofday, this difference is irrelevant as the Jitter RNG uses the time stamp to measure the execution time of a given code path and tries to detect variations in the execution time. Therefore, the only requirement the Jitter RNG has, is a sufficient high resolution to detect these variations. The change was tested on x86 to show an identical behavior as RDTSC. The used test code simply measures the execution time of the heart of the RNG: jent_get_nstime(&time); jent_memaccess(ec, min); jent_fold_time(NULL, time, &folded, min); jent_get_nstime(&time2); return ((time2 - time)); Signed-off-by: Stephan Mueller <smueller@chronox.de> --- crypto/jitterentropy-kcapi.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)