From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
To: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>,
Alexei Starovoitov
<alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>,
linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Subject: Re: [RFC 1/3] bpf/wireless: add wifimon program type
Date: Tue, 18 Apr 2017 13:28:02 +0200 [thread overview]
Message-ID: <1492514882.2472.22.camel@sipsolutions.net> (raw)
In-Reply-To: <58F5F148.1090700-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
On Tue, 2017-04-18 at 12:58 +0200, Daniel Borkmann wrote:
>
> Note that for skbs the data / data_end model (aka direct read /
> write) is also supported. There's also a bpf_skb_pull_data() helper
> that pulls in data from non-linear parts if necessary (f.e. if data /
> data_end test in the program fails). bpf_skb_load_bytes() is fine as
> well, comes with function call overhead, though, but depending on the
> use-case that might be just fine, too.
Yeah. I did see this, I just wasn't convinced I wanted the program to
be able to modify the SKB that way. OTOH, it doesn't actually matter if
it does this since that doesn't fundamentally change the SKB, so I
might as well allow it - and hook up data/data_end. In many cases, the
decision will be taken on the 802.11 header only, and that will be in
the linear portion anyway (with any self-respecting driver :p)
> Yeah, *_is_valid_access() callbacks would need to reject these
> members for most of the program types.
Yes, but that's the default case, so there's no real danger.
> Assuming you would introduce __wifi_sk_buff to the uapi, then it
> would be that the kernel internally still operates on an skb, you'd
> still call the program through BPF_PROG_RUN(prog, skb). However, your
> *_convert_ctx_access() would map from __wifi_sk_buff to the actual
> skb member, for example:
>
> [...]
> case offsetof(struct __wifi_sk_buff, len):
> BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, len) != 4);
>
> *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> offsetof(struct sk_buff, len));
> break;
> [...]
>
> Your *_func_proto() would just reuse the available skb helpers like:
>
> switch (func_id) {
> case BPF_FUNC_skb_load_bytes:
> return &bpf_skb_load_bytes_proto;
> [...]
> }
>
> So, it's not that you need a local struct xdp_buff -like definition
> in the kernel and convert all helpers to it, reusing skb in kernel
> works just fine this way.
Sure.
> The mapping in samples/bpf/bpf_helpers.h, for example, for mentioned
> bpf_skb_load_bytes() would also work out of the box, since it takes a
> void *ctx as an argument, so you can just pass the __wifi_sk_buff
> pointer as ctx there from program side.
Hah. That's what I was missing - I always assumed the argument was
"struct __sk_buff *" without ever checking that assumption.
> Assuming __wifi_sk_buff would get few wifi specific attributes over
> time which cannot be reused in other types, it's probably fine to
> go with a __wifi_sk_buff uapi definition. If helper functions
> dedicated to wifi program type can extract all necessary information,
> it's probably okay as well to go that route.
The thing is that __wifi_sk_buff doesn't have much information that's
generally useful available - for example, there's not much point in
allowing it to access the raw rate data, having the actual converted
bitrate is much more useful, and that requires a function call since I
don't want the overhead of calculating that in cases the program
doesn't need it.
> If the data passed to such a helper function would eventually be a
> __wifi_sk_buff-like struct that gets extended with further attributes
> over time, then it's probably easier to use __wifi_sk_buff itself as
> a ctx instead of argument for helpers. Reason is that once a helper
> is set in stone, you need to keep compatibility on the passed struct,
> meaning you need to test the passed length of the struct like we do
> in case of bpf_skb_get_tunnel_key() / bpf_skb_set_tunnel_key()
> helpers and only copy meta data up to that length for older programs.
Right.
johannes
next prev parent reply other threads:[~2017-04-18 11:28 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-12 11:07 [RFC 1/3] bpf/wireless: add wifimon program type Johannes Berg
2017-04-12 11:07 ` [RFC 2/3] cfg80211: add API to attach monitor filter program Johannes Berg
2017-04-12 11:07 ` [RFC 3/3] mac80211: support bpf monitor filter Johannes Berg
[not found] ` <20170412110726.9689-3-johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
2017-04-12 14:29 ` Johannes Berg
[not found] ` <1492007347.2855.12.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
2017-04-12 15:22 ` David Miller
[not found] ` <20170412.112212.441025205054195351.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
2017-04-12 15:25 ` Johannes Berg
2017-04-13 6:00 ` Johannes Berg
[not found] ` <20170412110726.9689-1-johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
2017-04-12 14:27 ` [RFC 1/3] bpf/wireless: add wifimon program type Johannes Berg
[not found] ` <1492007254.2855.10.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
2017-04-12 15:19 ` David Miller
2017-04-12 15:30 ` Johannes Berg
[not found] ` <1492011040.2855.18.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
2017-04-14 18:51 ` Alexei Starovoitov
2017-04-18 9:55 ` Johannes Berg
2017-04-18 10:58 ` Daniel Borkmann
[not found] ` <58F5F148.1090700-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
2017-04-18 11:28 ` Johannes Berg [this message]
2017-04-18 11:35 ` Johannes Berg
2017-04-12 19:47 ` Marcel Holtmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1492514882.2472.22.camel@sipsolutions.net \
--to=johannes-cdvu00un1vgdhxzaddlk8q@public.gmane.org \
--cc=alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
--cc=ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org \
--cc=davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org \
--cc=linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).