Date:   Sun, 17 Nov 2019 12:37:42 +0100
From:   Stephan Müller <smueller@...onox.de>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-crypto@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        linux-api@...r.kernel.org,
        "Alexander E. Patrakov" <patrakov@...il.com>,
        "Ahmed S. Darwish" <darwish.07@...il.com>,
        "Theodore Y. Ts'o" <tytso@....edu>, Willy Tarreau <w@....eu>,
        Matthew Garrett <mjg59@...f.ucam.org>,
        Vito Caputo <vcaputo@...garu.com>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Jan Kara <jack@...e.cz>, Ray Strode <rstrode@...hat.com>,
        William Jon McCann <mccann@....edu>,
        zhangjs <zachary@...shancloud.com>,
        Andy Lutomirski <luto@...nel.org>,
        Florian Weimer <fweimer@...hat.com>,
        Lennart Poettering <mzxreary@...inter.de>,
        Nicolai Stange <nstange@...e.de>,
        "Peter, Matthias" <matthias.peter@....bund.de>,
        Marcelo Henrique Cerri <marcelo.cerri@...onical.com>,
        Roman Drahtmueller <draht@...altsekun.de>,
        Neil Horman <nhorman@...hat.com>
Subject: Re: [PATCH v25 03/12] LRNG - /proc interface

Am Sonntag, 17. November 2019, 00:36:25 CET schrieb Eric W. Biederman:

Hi Eric,

> Stephan Müller <smueller@...onox.de> writes:
> > The LRNG /proc interface provides the same files as the legacy
> > /dev/random. These files behave identically. Yet, all files are
> > documented at [1].
> 
> For someone who works in this area a lot this description is confusing.
> 
> You are talking about sysctls not ordinary proc files.

Agreed and I will change the description accordingly.
> 
> You don't have a call register_sysctl.  If you want your own
> implementation of these sysctls that would seem to be the way to get
> them.  Teach each implementation to register their own set of sysctls
> if they are enabled.

Agreed, I will do that.
> 
> The entire structure of the code you are adding I have quite confusing,
> and a bit messing.
> 
> Why add a declaration of random_table in patch 1 and then delete that
> declaration in patch 3?  Nothing uses that declaration until this point.

This is only to ensure that patch 1 compiles. Without it, there would be a 
dangling reference that is required by static struct ctl_table kern_table[].

As I was always under the impression that each patch should compile by itself 
to support bisect, I added that empty declaration. Yet, patch 1 is never 
intended to live without patch 3. I only split patch 3 up is to aid code 
review by having as many individual patches as possible while still allowing 
them to compile.
> 
> What is the point of adding an extern declaration just before you
> declare the table itself?  As I understand the C language that achieves
> nothing.  I understand that is what the existing code in
> drivers/char/random.c does but that is equally buggy there.

I totally agree and I was wondering here as well. But I simply was taking it 
as is.

I am happy to clean this one up.
> 
> I also don't understand why you don't modify the existing random
> generator code into the form you want?  What is the point of a
> side-by-side replacement?  Especially since only one of them can
> be compiled into the kernel at the same time?

I was developing small patches for random.c since 2013, mostly cleanup 
patches. Unfortunately most of them were silently ignored. Some others were 
silently taken but appeared two or three kernel releases later.

Getting more architecture-invasive patches into the existing random.c code is 
considered to be quite a problem considering this experience.

Besides, the LRNG has quite a different architecture compared to the random.c. 
As the RNG is an important aspect of the kernel, I did not feel bold enough to 
simply replace the existing code which implies that there is no fallback. By 
allowing a side-by-side code base which is even deactivated by default, it 
allows other researchers to analyze the mathematical aspects beyond the pure 
code while still having an implementation that provides a known and analyzed 
entropy source.

Also, considering other kernel components like memory allocators, I/O 
schedulers or even file systems, providing different architectures covering 
similar problems (memory allocation, accessing a disk) with an entirely 
different architecture and thus implementation seems to be an appropriate 
solution.

Finally, I tried to keep code that has a similar functionality to the existing 
random.c similar to allow a merge at a later stage. For example, the sysctls 
are identical, but internally use different variables.
> 
> This build a replacement and then switch over seems like a recipe for
> loosing the lessons of history because you are not making incremental
> changes that can be clearly understood, reviewed and bisected.
> 
> As I read your patchset until this change your code will fail to compile
> in an ordinary configuration with proc enabled.  Have you even tested
> compiling your patchset one patch at a time?

Yes, it does compile with proc enabled with the warning that random_table is 
considered to contain one element.
> 
> For me a great reorganization to impelment a better structure that fails
> to have a good structure on the usual merits makes me dubious about the
> entire thing.  As it can be a sign the author was pushing so hard to
> make things work he stopped looking at problematic details.
> 
> Dubious-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> 
> Eric
> 
> > +
> > +extern struct ctl_table random_table[];
> > +struct ctl_table random_table[] = {
> > +	{


Ciao
Stephan

Powered by blists - more mailing lists