netdev - Re: [RFC 1/3] bpf/wireless: add wifimon program type

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1492509349.2472.15.camel@sipsolutions.net>
Date:   Tue, 18 Apr 2017 11:55:49 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     David Miller <davem@...emloft.net>, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, daniel@...earbox.net, ast@...nel.org
Subject: Re: [RFC 1/3] bpf/wireless: add wifimon program type

On Fri, 2017-04-14 at 11:51 -0700, Alexei Starovoitov wrote:
> 
> so today only 'len' field is needed,

Correct, the other __sk_buff fields don't apply.

It's more of an XDP model (with data/data_end), but as the SKB might be
non-linear at this point I prefer to have the SKB so that skb data
access (skb_copy_bits equivalent) works.

> but the plan to add wifi specific
> stuff to bpf context?

Maybe, maybe not.

> If so, adding these very wifi specific fields to __sk_buff will
> confuse other program types,

I don't think this is true - the verifier still checks that you can't
actually use them. It might confuse authors though, if not documented
well.

> so new program type (like you did) and new 'struct bpf_wifi_md' are
> probably cleaner.

The problem is that I still need struct __wifi_sk_buff or so, and need
to internally let it operate like an SKB, since I need
bpf_skb_load_bytes().

Now, bpf_helpers declares bpf_skb_load_bytes() to take an argument of
type "struct __sk_buff *", and thus I either need to provide an alias
to it, or cast every time I use this function.

> Physically the R1 register to bpf program will still be 'struct
> sk_buff', but from bpf program side it will be:
> struct bpf_wifi_md {
>   __u32 len;
>   __u32 wifi_things;
> };

Right, that would work immediately if it's only about the extra fields.
But it's more about being able to call bpf_skb_load_bytes() easily.

I don't even know if I want to add *any* wifi_things here at all. I
don't actually have much data available directly at this point, or at
least not in a format that would be useful, so I think it'd be better
to have a BPF helper function to obtain wifi_things outside of the
struct itself, passing the struct bpf_wifi_md * (and thus getting
struct sk_buff * in the kernel code) to it.

> At the same time if most of the __sk_buff fields can be useful to
> wifimon, then just use it as-is (without restricting to 'len' only)
> and add wifi specific fields to it with a comment.

No, I don't think they can ever be useful.

johannes