Message-ID: <87mtcjr4cw.ffs@tglx>
Date:   Fri, 05 Aug 2022 01:11:43 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        X86 ML <x86@...nel.org>, Nadia Heninger <nadiah@...ucsd.edu>,
        Thomas Ristenpart <ristenpart@...nell.edu>,
        Theodore Ts'o <tytso@....edu>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Adhemerval Zanella Netto <adhemerval.zanella@...aro.org>,
        Florian Weimer <fweimer@...hat.com>
Subject: Re: [PATCH RFC v1] random: implement getrandom() in vDSO

Jason!

On Thu, Aug 04 2022 at 17:23, Jason A. Donenfeld wrote:
> On Wed, Aug 03, 2022 at 12:27:43AM +0200, Thomas Gleixner wrote:
>> Not at all. The concept of 'basically same semantics' is a delusion. It
>> does not exist. Either it's the same or it's not.
>> 
>> I really want to see your reaction on a claim that some RNG
>> implementation is basically the same as the existing one. I'm sure you
>> buy that without complaints.
>
> I mean there are some undeniable similarities here.

Similarity yes. Semantically in the sense of an API definitely not.

> We might be arguing over semantics of semantics at this point,
> but... Barring the additional `void *state` argument, they take the
> same inputs and produce the same outputs. Then, most importantly, the
> way they each produce randomness is the same, using the same
> algorithms and same lifetime rules. For all measures that are
> meaningful to me, they're the "same". Yes yes technically that extra
> `void *state` means something is slightly different.  But does that
> matter? I guess your point is that somehow it does matter, and maybe
> that's where we disagree.

The point is that the API is different and you can't provide a API
compatible variant in the VDSO ever.

This matters because it requires auxilliary code outside of the VDSO or
even application changes.

>> >> So you have to go through the whole process of a new ABI whether you
>> >> like it or not.
>> >
>> > Ahh, in that sense. Yea, I'd rather not have to do that too, with the
>> > additional opaque handle passed as the first argument. It'd be nice if
>> > there were some private place where I could store the necessary state,
>> > but I'm not really sure where that might be at the moment. If you have
>> > any ideas, please let me know.
>> 
>> That's exactly the problem. VDSO is a stateless syscall wrapper which
>> has to be self contained for obvious reasons.
>
> Unless each call can import and export any potential state. That is,
> unless there's a `void *state` argument like what I added. Then the code
> itself doesn't refer to any global state, but can still behave in a
> stateful manner.

Which makes it an incompatible API with an untyped pointer which is
sloppy at best.

>> The unwillingness part is just the question:
>> 
>>     Is there a sensible usecase?
>
> In the use case department, by the way, apparently there really is.
> arc4random() is too slow for chronyd:
> https://sourceware.org/bugzilla/show_bug.cgi?id=29437
> And that's on a kernel even with the "newer faster getrandom()".

For a problem which does not require cryptographically secure random
numbers. Truly convincing.

>> Assumed that there is a sensible usecase, there is a way out and that's
>> exactly the library part. You can make that VDSO interface versioned and
>> provide a library in tools/random/ which goes in lockstep with the VDSO
>> changes.
>
> That sounds absolutely dreadful; no way jose. Then we wind up having to
> maintain the data in the vDSO as a particular version, and keep that
> working into the future. That's not going to fly. As I wrote before, I
> don't want to commit the RNG to preserving certain internal semantics
> over time. That's not something I feel comfortable committing to. And
> even if we can "add" new ones with a new version in your hypothetical
> scheme, we'd still have to keep the old ones working, and that could
> prove prohibitive of improvements. So that's not going to work.

That's a complete nonsensical argumentation. If you install a new
kernel, then the new library which exposes the application interface is
installed too. So where is the problem?

That needs obviously some infrastructure so that the kernel install
handles such libraries similar to modules, but that would be a
worthwhile exercise if we want to expose more information through VDSO.

Such a library exposes the API and the interface between the library and
the VDSO is kernel dependent. They are shipped in lockstep.

That way you avoid to implement a ton of syscall fallbacks for this muck
and just treat it as any other regular library. You even can hide the
whole allocation completely and make it a drop in replacement:

  getrandom(....)
        if (!tls->random_state)
           alloc_tls_random_state()
        ....

A real library can do that, VDSO not.

The only version information in that data interface would be:

struct vdso_random_data {
       unsigned int	version;
       ....
};

All other fields are version dependent and you can change them as you
want.

If some tinkerer uses the data interface directly and ignores the
version check, then he can keep the pieces. That's not any different
than the timekeeping data interface of the VDSO.

I've seen code which accesses the timekeeping VDSO data directly just
because some performance expert decided that rdtsc_ordered() is too slow
and a trivial rdtsc() is good enough. That exploded in their face when
we overhauled the VDSO.

Can you prevent that? No.

Does it matter? No.

The data interface is documented to be for the kernel supplied library
only and the version field is only for paranoia reasons to validate that
kernel and library agree.

That means if some tinkerer uses the data interface directly then he
either does a version check and bails out when the version is different
from the expected version or he falls flat on his face.

That's a very simple contract. Versioned interfaces are not required to
be backwards compatible. That's a matter of definition and documentation.

> And again, I don't see how this is any different than gettimeofday() and
> such. Why didn't you just make versioned accessor functions for each one
> of the various struct fields, and then stick some library code into
> tools/timekeeping/ for glibc to copy and paste once and never update
> again? Why isn't this a nightmare you chose to have each time the moon
> is full? Clearly because doing so would be a maintenance disaster and
> would impede future meaningful progress, not to mention proliferating
> wrong means of reading the time.

The time accessors have been proven to be performance critical and the
implementation is a drop in replacement for the syscall so that an extra
library is pointless and less performant.

But you are neither providing a convincing use case nor a drop in
replacement simply because it's technically impossible in the VDSO to
provide that with the features you want to add.

> Just as gettimeofday benefits from being an actual function in the vDSO,
> so too does getrandom().

time*() == vsdo_time*() but getrandom() != vdso_magic_getrandom()

Can you spot the difference?

>> Vs. the storage problem. That yells TLS, but that makes your process
>> wide sharing moot, which might not be the worst of all things IMO.
>
> Yea, TLS is what we want here. The `void *state` argument thing is meant
> for this. You allocate an array of states using that alloc function, and
> then you divvy them up per-thread.

How does the first caller know how many threads are going to use this
and how is the pointer handed over to another thread?. TLS == Thread
Local Storage which unsurprisingly means that such a pointer is thread
private.

Which part of the code is going to do that pointer sharing? Are you
going to hide that in the VDSO library too? You can't.

If you want to use TLS which is the only sensible solution then you have
to do the allocation per thread which avoids all the sharding issues and
screams even more for a lib_getrandom_$kernel_version.so solution.

But before we go there, please provide a sensible use case.

Thanks,

        tglx

Powered by blists - more mailing lists