[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH v9] seedrng: import SeedRNG utility for kernel RNG seed files
From:       "Jason A. Donenfeld" <Jason () zx2c4 ! com>
Date:       2022-04-30 13:39:18
Message-ID: CAHmME9oO4BWWMf3Zsbgd=jrVu1_nQ7pP0+-+CRv7OWC4VNjAcQ () mail ! gmail ! com
[Download RAW message or body]

Hi Denys,

On Sat, Apr 30, 2022 at 4:09 AM Denys Vlasenko <vda.linux@googlemail.com> wrote:
>
> On Fri, Apr 29, 2022 at 6:57 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > On Fri, Apr 29, 2022 at 6:04 PM Denys Vlasenko <vda.linux@googlemail.com> wrote:
> > > On Wed, Apr 27, 2022 at 6:55 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > > On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote:
> > > > >         if ((unlink(filename) < 0 || fsync(dfd) < 0) && seed_len) {
> > > > >                 bb_perror_msg("can't%s seed", " remove");
> > > > >                 return -1;
> > > > >         }
> > > > >
> > > > > Why can't the entire above if() be replaced with xunlink(filename) ?
> > > >
> > > > It cannot be replaced with that. The fsync() must happen before other
> > > > operations move forward,
> > >
> > > Why? It should be explained in a comment, and explained well enough
> > > so that future developers do not repeatedly try to remove it
> > > because "I don't see why it's necessary".
> >
> > I'll add a comment and submit v2 of my recent patch.
>
> It'll be faster if we just discuss it in this thread.

Surely not, since I submitted v2 a day prior to your reply. It looks
like you responded over there, so I'll follow up in that thread.

> > > > and exiting the program isn't necessarily the
> > > > correct course of action.
>
> Of course. I'm not saying that "immediately exit with error message"
> _always_ is the correct approach.
>
> What I saw a lot over the years is that error paths
> tend to be not well-tested, and nevertheless, even when they
> are not buggy per se, the error conditions they are reacting to
> are often a part of a bigger problem (usually beyond the ability
> of the code in question to detect, much less sensibly correct).
>
> For example, if you get a ENOSPC on write, you might be
> tempted to return a special exit code signifying this specific error.
>
> However.
> I'd bet you real money that NOT ONE of callers of your tool
> will ever bother checking specifically for this exit code.
> At best, the callers will react uniformly for any non-zero exit code as
> "something went wrong". Quite often, check for !0 exit code will be
> entirely forgotten.

I'm not going to bet you real money on anything, as this is volunteer
work, unpaid, etc. Please don't offer that again.

Anyway, your point is rubbish. The promise of SeedRNG is that it tries
to always do the maximal amount of useful things, and safely handles
the various error cases that can happen, because the kernel's
interface is complicated and a modicum of errors can transpire. The
user can then decide what to do with those errors -- halt the system
boot under certain circumstances, try to pull from some expensive last
ditch effort, send an alert email to an administrator, etc. It's both
important that the user is notified about this via the error code AND
that SeedRNG continues on doing the maximum amount of initialization
that it can do safely (i.e. without introducing a vulnerability).

Please stop attempting to make SeedRNG as worthless as all the
previous other attempts to do RNG initialization. Your direction
toward fragmentation, non-robustness, shortsighted thinking, and so
forth is only going to perpetuate a problem that has long plagued the
Linux ecosystem. Stop doing that.

Jason
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]