From patchwork Tue Mar 19 16:52:49 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 31908 Received: (qmail 60793 invoked by alias); 19 Mar 2019 16:52:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 60613 invoked by uid 89); 19 Mar 2019 16:52:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=resp, accuracy X-HELO: mail-vs1-f68.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:cc:references:from:openpgp:autocrypt:subject:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=9hLym3uubMexgVexZ4tsNQeoTo58hzmtT798Y0ippqE=; b=zMyJL5tDGv7j5znjbgxcOllG1OOhqrvR3i+mWAgLzh4GEFfCM5cpHjBO7gjRH6p3ul Zx0uyqxdZKaUD8X1uGHsZrdVXx92i7YixgMSyv2TShr9rYM5/UyCt0kjE0V/KCmGToWk CEcTsSgi/sbrLWquz1CGLDpkuN23ka/WC1nfMthXDkts0ClQQU9381tHfCF7auMntQ7u gkQtLm/7eXTYqMo5YONRPjhvsd86iXCBg4NR3uN55rBn1C8SeTElKdBi0IPT3RoT7Vph j6IcY1BeA5AK6jW3eye80iNxCYslW081UuMvSWo0e8V0xZ8c/MkHGpCd7anwfSBcCIed SGBA== Return-Path: To: Wilco Dijkstra Cc: nd , "libc-alpha@sourceware.org" References: From: Adhemerval Zanella Openpgp: preference=signencrypt Subject: Re: [PATCH v2 4/6] Do not use HP_TIMING_NOW for random bits Message-ID: <6ea276af-aa62-2bfe-ee6c-83c8eeaf283a@linaro.org> Date: Tue, 19 Mar 2019 13:52:49 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: 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 > + . */ > + > +#ifndef _RANDOM_BITS_H > +# define _RANDOM_BITS_H > + > +#include > +#include > + > +/* 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 > #include > #include > #include > @@ -92,12 +93,7 @@ > #include > #include > #include > - > -#include > -#include > -#if HP_TIMING_AVAIL > -# define RANDOM_BITS(Var) { uint64_t v64; HP_TIMING_NOW (v64); Var = v64; } > -#endif > +#include > > 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 > #include > #include > -#include > +#include > > #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 > -# 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 > +# 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. diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c index 5217cb38e1..d062e4b82f 100644 --- a/sysdeps/posix/tempname.c +++ b/sysdeps/posix/tempname.c @@ -73,6 +73,13 @@ #ifdef _LIBC # include # 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) {