Am Sonntag, 17. November 2019, 00:36:25 CET schrieb Eric W. Biederman:
Hi Eric,
> Stephan Müller <smueller@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>
> Eric
>
> > +
> > +extern struct ctl_table random_table[];
> > +struct ctl_table random_table[] = {
> > + {
Ciao
Stephan
