Received-SPF: pass (google.com: domain of linux-kernel+bounces-44651-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 30 Jan 2024 13:51:50 +0100
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
	Theodore Ts'o <tytso@mit.edu>,
	Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>,
	Elena Reshetova <elena.reshetova@intel.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	"Kalra, Ashish" <ashish.kalra@amd.com>,
	Sean Christopherson <seanjc@google.com>, linux-coco@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] x86/random: Retry on RDSEED failure
Message-ID: <Zbjw5hRHr_E6k18r@zx2c4.com>
References: <20240130083007.1876787-1-kirill.shutemov@linux.intel.com>
 <CAHmME9pOt=uEmuBzBpgUHw9DqAD2FZTZ3v53AOZbQ3Cd2p97xQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHmME9pOt=uEmuBzBpgUHw9DqAD2FZTZ3v53AOZbQ3Cd2p97xQ@mail.gmail.com>

On Tue, Jan 30, 2024 at 01:29:10PM +0100, Jason A. Donenfeld wrote:
> Hi Kirill,
> 
> I've been following the other discussion closely thinking about the
> matter, but I suppose I'll jump in here directly on this patch, if
> this is the approach the discussion is congealing around.
> 
> A comment below:
> 
> On Tue, Jan 30, 2024 at 9:30 AM Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> >  static inline bool __must_check rdseed_long(unsigned long *v)
> >  {
> > +       unsigned int retry = RDRAND_RETRY_LOOPS;
> >         bool ok;
> > -       asm volatile("rdseed %[out]"
> > -                    CC_SET(c)
> > -                    : CC_OUT(c) (ok), [out] "=r" (*v));
> > -       return ok;
> > +
> > +       do {
> > +               asm volatile("rdseed %[out]"
> > +                            CC_SET(c)
> > +                            : CC_OUT(c) (ok), [out] "=r" (*v));
> > +
> > +               if (ok)
> > +                       return true;
> > +       } while (--retry);
> > +
> > +       return false;
> >  }
> 
> So, my understanding of RDRAND vs RDSEED -- deliberately leaving out
> any cryptographic discussion here -- is roughly that RDRAND will
> expand the seed material for longer, while RDSEED will mostly always
> try to sample more bits from the environment. AES is fast, while
> sampling is slow, so RDRAND gives better performance and is less
> likely to fail, whereas RDSEED always has to wait on the hardware to
> collect some bits, so is more likely to fail.
> 
> For that reason, most of the usage of RDRAND and RDSEED inside of
> random.c is something to the tune of `if (!rdseed(out)) rdrand(out);`,
> first trying RDSEED but falling back to RDRAND if it's busy. That
> still seems to me like a reasonable approach, which this patch would
> partly undermine (in concert with the next patch, which I'll comment
> on in a follow up email there).
> 
> So maybe this patch #1 (of 2) can be dropped?

Unless there's a difference between ring 0 and ring 3, this simple test
is telling:

  #include <stdio.h>
  #include <immintrin.h>
  
  int main(int argc, char *argv[])
  {
    unsigned long long rand;
    unsigned int i, success_rand = 0, success_seed = 0;
    enum { TOTAL = 1000000 };
  
    for (i = 0; i < TOTAL; ++i)
    	success_rand += !!_rdrand64_step(&rand);
    for (i = 0; i < TOTAL; ++i)
    	success_seed += !!_rdseed64_step(&rand);
    
    printf("RDRAND: %.2f%%, RDSEED: %.2f%%\n", success_rand * 100.0 / TOTAL, success_seed * 100.0 / TOTAL);
    return 0;
  }

Result on my i7-11850H:

  RDRAND: 100.00%, RDSEED: 29.26%

And this doesn't even test multicore stuff.

Jason