Subject: Re: [PATCH net-next 1/2] ebpf: add prandom helper for packet sampling

From: Daniel Borkmann <[email-masked]>
Date: Fri, 13 Mar 2015 23:44:38 +0100

On 03/13/2015 06:03 PM, Daniel Borkmann wrote:
> On 03/13/2015 05:58 PM, Alexei Starovoitov wrote:
...
>>> +static u64 bpf_random(u64 dw, u64 r2, u64 r3, u64 r4, u64 r5)
>>> +{
>>> +    return prandom_u32() | (dw ? ((u64)prandom_u32() << 32) : 0);
>>
>> can you make first argument to be a flag instead of bool ?
>> 0 to return u32, 1 to return u64 and >=2 to be reserved and
>> return -EINVAL?
...
>> This way we can extend it later. For example 2 might return
>> 8 truly random bytes from get_random_bytes_arch()

Hmm, just got back to this thinking about it a bit more. I don't
like that we would return with a negative error here. An eBPF
application would then need to check for errors as well to make
sure, and the signature returns max u64. So _theoretically_,
-EINVAL could also come as a result from prandom() albeit unlikely.
That would make the interface a bit ugly, imho.

I think the interface should be as simple as possible for
obtaining a random number. I think that if we would want to
support something like get_random_bytes_arch(), it would be
better to do so in a new interface, where the name also more
clearly indicates where the random data comes from.

That said, I'd rather prefer to leave the code as is, but go
with the rename, maybe even BPF_FUNC_get_prandom_val so it
indicates it's pseudo-random as opposed to, say, something like
BPF_FUNC_get_srandom_val if we need that one day.

Cheers,
Daniel