[v2,4/6] Do not use HP_TIMING_NOW for random bits
Commit Message
On 07/03/2019 14:09, Wilco Dijkstra wrote:
> Hi Adhemerval,
>
> LGTM with a few minor comments below.
>
> Wilco
>
> include/random-bits.h | 41 ++++++++++++++++++++++++++++++++++++++++
> resolv/res_mkquery.c | 19 +++----------------
> resolv/res_send.c | 12 ++----------
> sysdeps/posix/tempname.c | 19 +++----------------
> 4 files changed, 49 insertions(+), 42 deletions(-)
> create mode 100644 include/random-bits.h
>
> diff --git a/include/random-bits.h b/include/random-bits.h
> new file mode 100644
> index 0000000000..5ab53450af
> --- /dev/null
> +++ b/include/random-bits.h
> @@ -0,0 +1,41 @@
> +/* Fast pseudo-random bits based on clock_gettime.
> + Copyright (C) 2019 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#ifndef _RANDOM_BITS_H
> +# define _RANDOM_BITS_H
> +
> +#include <time.h>
> +#include <stdint.h>
> +
> +/* Provides fast pseudo-random bits through clock_gettime. It has unspecified
> + starting time, nano-second accuracy, its randomness is significantly better
> + than gettimeofday, and for mostly architectures it is implemented through
> + vDSO instead of a syscall. Since the source is a system clock, the upper
> + bits will have less entropy. */
> +static inline uint32_t
> +random_bits (void)
> +{
> + struct timespec tv;
> + __clock_gettime (CLOCK_MONOTONIC, &tv);
> + /* Shuffle the lower bits to minimize the clock bias. */
> + uint32_t ret = tv.tv_nsec ^ tv.tv_sec;
> + ret ^= (ret << 24) | (ret >> 8);
> + return ret;
> +}
> +
> +#endif
>
> OK
>
> diff --git a/resolv/res_mkquery.c b/resolv/res_mkquery.c
> index 19b8b402c4..dd43d347af 100644
> --- a/resolv/res_mkquery.c
> +++ b/resolv/res_mkquery.c
> @@ -82,6 +82,7 @@
> * SOFTWARE.
> */
>
> +#include <stdint.h>
> #include <sys/types.h>
> #include <sys/param.h>
> #include <netinet/in.h>
> @@ -92,12 +93,7 @@
> #include <string.h>
> #include <sys/time.h>
> #include <shlib-compat.h>
> -
> -#include <hp-timing.h>
> -#include <stdint.h>
> -#if HP_TIMING_AVAIL
> -# define RANDOM_BITS(Var) { uint64_t v64; HP_TIMING_NOW (v64); Var = v64; }
> -#endif
> +#include <random-bits.h>
>
> int
> __res_context_mkquery (struct resolv_context *ctx, int op, const char *dname,
> @@ -120,16 +116,7 @@ __res_context_mkquery (struct resolv_context *ctx, int op, const char *dname,
> /* We randomize the IDs every time. The old code just incremented
> by one after the initial randomization which still predictable if
> the application does multiple requests. */
> - int randombits;
> -#ifdef RANDOM_BITS
> - RANDOM_BITS (randombits);
> -#else
> - struct timeval tv;
> - __gettimeofday (&tv, NULL);
> - randombits = (tv.tv_sec << 8) ^ tv.tv_usec;
> -#endif
> -
> - hp->id = randombits;
> + hp->id = random_bits ();
> hp->opcode = op;
> hp->rd = (ctx->resp->options & RES_RECURSE) != 0;
> hp->rcode = NOERROR;
>
> OK
>
> diff --git a/resolv/res_send.c b/resolv/res_send.c
> index fa040c1198..1b59b6080c 100644
> --- a/resolv/res_send.c
> +++ b/resolv/res_send.c
> @@ -109,7 +109,7 @@
> #include <unistd.h>
> #include <kernel-features.h>
> #include <libc-diag.h>
> -#include <hp-timing.h>
> +#include <random-bits.h>
>
> #if PACKETSZ > 65536
> #define MAXPACKET PACKETSZ
> @@ -309,15 +309,7 @@ nameserver_offset (struct __res_state *statp)
> if ((offset & 1) == 0)
> {
> /* Initialization is required. */
> -#if HP_TIMING_AVAIL
> - uint64_t ticks;
> - HP_TIMING_NOW (ticks);
> - offset = ticks;
> -#else
> - struct timeval tv;
> - __gettimeofday (&tv, NULL);
> - offset = ((tv.tv_sec << 8) ^ tv.tv_usec);
> -#endif
> + offset = random_bits ();
> /* The lowest bit is the most random. Preserve it. */
> offset <<= 1;
>
> OK
>
> diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
> index 2ed39d1a42..5217cb38e1 100644
> --- a/sysdeps/posix/tempname.c
> +++ b/sysdeps/posix/tempname.c
> @@ -71,22 +71,8 @@
> #endif
>
> #ifdef _LIBC
> -# include <hp-timing.h>
> -# if HP_TIMING_AVAIL
> -# define RANDOM_BITS(Var) \
> - if (__glibc_unlikely (value == UINT64_C (0))) \
> - { \
> - /* If this is the first time this function is used initialize \
> - the variable we accumulate the value in to some somewhat \
> - random value. If we'd not do this programs at startup time \
> - might have a reduced set of possible names, at least on slow \
> - machines. */ \
> - struct timeval tv; \
> - __gettimeofday (&tv, NULL); \
> - value = ((uint64_t) tv.tv_usec << 16) ^ tv.tv_sec; \
> - } \
> - HP_TIMING_NOW (Var)
> -# endif
> +# include <random-bits.h>
> +# define RANDOM_BITS(Var) ((Var) = random_bits ())
>
> This define is not used (removed above).
I think we need to still define it if we eventually decide to sync it back
to gnulib.
>
> #endif
>
> /* Use the widest available unsigned type if uint64_t is not
> @@ -237,6 +223,7 @@ __gen_tempname (char *tmpl, int suffixlen, int flags, int kind)
> }
> #endif
> value += random_time_bits ^ __getpid ();
> + value += random_bits () ^ __getpid ();
>
> One of these should be shifted so that it actually increases the number of
> random bits.
>
> Note value is static, which looks like a concurrency bug. Making it a local
> should work equally well now we've got a better random number.
>
> for (count = 0; count < attempts; value += 7777, ++count)
> {
>
I fact the new line should not be added, since random_time_bits should already
get the random_bits() value. In any case I think we can remove random_time_bits
altogether and just call RANDOM_BITS on value instead.
And it seems 'value' is static by design, but I do agree there is no impeding
reason to continue to do so.
I applied the following changes locally based on your review.
---
@@ -73,6 +73,13 @@
#ifdef _LIBC
# include <random-bits.h>
# define RANDOM_BITS(Var) ((Var) = random_bits ())
+# else
+# define RANDOM_BITS(Var) \
+ { \
+ struct timeval tv; \
+ __gettimeofday (&tv, NULL); \
+ (Var) = ((uint64_t) tv.tv_usec << 16) ^ tv.tv_sec; \
+ }
#endif
/* Use the widest available unsigned type if uint64_t is not
@@ -179,8 +186,7 @@ __gen_tempname (char *tmpl, int suffixlen, int flags, int kind)
{
int len;
char *XXXXXX;
- static uint64_t value;
- uint64_t random_time_bits;
+ uint64_t value;
unsigned int count;
int fd = -1;
int save_errno = errno;
@@ -213,17 +219,10 @@ __gen_tempname (char *tmpl, int suffixlen, int flags, int kind)
XXXXXX = &tmpl[len - 6 - suffixlen];
/* Get some more or less random data. */
-#ifdef RANDOM_BITS
- RANDOM_BITS (random_time_bits);
-#else
- {
- struct timeval tv;
- __gettimeofday (&tv, NULL);
- random_time_bits = ((uint64_t) tv.tv_usec << 16) ^ tv.tv_sec;
- }
-#endif
- value += random_time_bits ^ __getpid ();
- value += random_bits () ^ __getpid ();
+ RANDOM_BITS (value);
+ value ^= __getpid ();
+ /* Shuffle the lower bits to minimize the pid bias due low maximum value. */
+ value = (value << 24) | (value >> 8);
for (count = 0; count < attempts; value += 7777, ++count)
{