netdev - Re: [PATCH] filter: added BPF random opcode

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: <CA+ZOOTPs8cpjqttv2DxU14GGZF1zETnN74_0t0KVjTZV2uY3nQ@mail.gmail.com>
Date:	Wed, 16 Apr 2014 17:19:57 -0700
From:	Chema Gonzalez <chema@...gle.com>
To:	Alexei Starovoitov <ast@...mgrid.com>
Cc:	David Miller <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Daniel Borkmann <dborkman@...hat.com>,
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] filter: added BPF random opcode

On Tue, Apr 15, 2014 at 11:24 PM, Alexei Starovoitov <ast@...mgrid.com> wrote:
> On Tue, Apr 15, 2014 at 11:16 AM, Chema Gonzalez <chema@...gle.com> wrote:
>> This should allow random packet sampling.
>
> I think commit log should have more than one line,
> especially for the new feature that affects uapi
I will beef it up in the next version (this time I'll wait for the
net-next window to open).

> In particular I don't find the reason of moving random
> packet sampling into kernel to be that great.
> pcap cannot receive them quickly enough anyway,
> so there are drops already.
> Just randomly pick packets that reached the user space.
> Why add this to the kernel? I don't see how it can improve accuracy.
The main use I see for this is random packet sampling. If you're
getting packet drops in pcap, you definitely want the kernel to do the
sampling. Sampling the packets that survived all the way through
userspace instead of the packets that arrived to the nic is biased.

The new message will be:

"""
filter: added BPF random opcode

Added a new ancillary load (bpf call in eBPF parlance) that produces
a 32-bit random number. We are implementing it as an ancillary load
(instead of an ISA opcode) because (a) it is simpler, (b) allows easy
JITing, and (c) seems more in line with generic ISAs that do not have
"get a random number" as a instruction, but as an OS call.

The main use for this ancillary load is to perform random packet sampling.
"""

> At the same time I think the extension technic itself is nice and clean :)
> The call approach is definitely how we envisioned it to be used.
> Right now we have only x86-64 jit that is waiting for net-next to be opened,
> but this extension will be automatically jit-ed due to 'call approach'. Nice.
>
> When you post patch revision please add vN tag.
I will.

>> +/* note that this only generates 32-bit random numbers */
>> +static u64 __skb_get_random(u64 ctx, u64 A, u64 X, u64 r4, u64 r5)
>
> function name is misleading. It has nothing to do with 'skb'. Please
> drop that prefix.
I renamed the function "__get_random_u32".

>> +{
>> +       return (u64)prandom_u32();
>
> we have 64-bit registers now, so would be nice to generalize it to 64-bit random
> since 8/16/32 can be made with bpf_and operation.
> but I don't see how to cleanly do it yet, so I guess the current form is fine.
>
> I like the approach, but would be nice to hear better justification
> for extending uapi.

-Chema
--
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