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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55036A7D.7050402@plumgrid.com>
Date:	Fri, 13 Mar 2015 15:53:49 -0700
From:	Alexei Starovoitov <ast@...mgrid.com>
To:	Daniel Borkmann <daniel@...earbox.net>, davem@...emloft.net
CC:	netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] ebpf: add prandom helper for packet sampling

On 3/13/15 3:44 PM, Daniel Borkmann wrote:
> 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.

fair point.

I was also thinking about it more and I believe that 'bool' flag
is redundant.
If program wants to have 64-bit value it should just call this
helper twice. It actually will be faster, since such 64-bit
value will be without branches whereas 'dw' flag introduces
if condition in critical path that will be executed for every packet.
So my vote is for BPF_FUNC_get_prandom_u32 that returns
prandom_u32() and nothing else.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html