netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  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).