[PATCH v2] seedrng: import SeedRNG utility for kernel RNG seed files

[Bernhard Reutner-Fischer]

On Sun, 10 Apr 2022 15:56:17 +0200
"Jason A. Donenfeld" <Jason at zx2c4.com> wrote:

> Hi Bernhard,
> 
> On Sun, Apr 10, 2022 at 3:40 PM Bernhard Reutner-Fischer
> <rep.dot.nop at gmail.com> wrote:

> > Also seed_dir et al are only used in main so i'd move them there and
> > not have them static. Doesn't make much difference though.  
> 
> seed_dir isn't only used from main. It doesn't impact binary size.

I'd argue that it should only be used from main.
There's no benefit to have additional code dealing with the dir if what
you really want and operate on is the respective seed file, no?

What is that fsync(dfd) supposed to achieve? I'd remove all this, i
don't really see why it's needed.

> > > > Why do you open(), flock() instead of using O_EXCL?  
> > >
> > > So that if the system crashes, the next boot can still move forward,  
> >
> > The next boot will not find any previous data i assume? I thought they
> > live in /run which usually is not in battery backed up RAM.  
> 
> Sometimes it's tmpfs, sometimes it isn't. And sometimes programs get
> oom'd mid-execution and can't cleanup files. O_EXCL is not the right
> way of doing this.

Yea, so if this is supposed to be run even after the initial invocation
at boot, i.e. anytime via timers, then yes.
The lockfile is conceptually not much different than a pidfile i
suppose. But there is probably no benefit in creating a pidfile and
locking that as opposed to what you manually do now. Hmm.

I'm aware that this whole getrandom() is a linux-only thing, otherwise
i would recommend against using flock() ang go the standard fcntl SETLK
route. Just as a sidenote.

> > > or if the process crashes, the next run can still move forward.
> > > flock() is a runtime thing, where as O_EXCL is a "must be in
> > > filesystem" thing, which is much weaker.  
> >
> > I admit i did not really look. Don't you ever only want to run one
> > instance at once? I had thought so, no?  
> 
> Yes, which is why the lock is there. Running more than one at once can
> be catastrophic. That invariant must be enforced with a lock. And
> given that people might wind up running this on timers -- timers that
> can race with other events like shutdown -- that lock must be there.
> I'm not going to remove the lock, sorry.

Yes sure, if a program is required to run exclusively then that's of
course fine. I'm just thinking loud if there is a different way to
achieve the same with less non-shared code, or by using existing
helpers..

Btw, you seem to be touching errno a lot and you do that in addition to
ret.
Maybe this can be simplified a little bit?
For example, maybe you can just bb_simple_perror_msg_and_die in most of
the error paths? If you really, really have to communicate some
non-standard exit code back then you would set xfunc_error_retval to
your non-standard value. But maybe we're lucky and you wouldn't need
anything but 0 or 1 exit at all?
thanks,