| Message ID | 20170412110726.9689-1-johannes@sipsolutions.net |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2017-04-12 at 13:07 +0200, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > Add a new program type for wifi monitor interfaces. This program > type can > * access __sk_buff, but only skb->len > * call bpf_skb_load_bytes() > > The program type is only enabled when something selects the new > Kconfig symbol WANT_BPF_WIFIMON, which will be done by mac80211 > in a follow-up patch. This works. An example BPF program is here: https://p.sipsolutions.net/ca32264f2b705e5e.txt I haven't really done any performance measurements and only tested it in a virtual environment, but it's nice to see that in principle I can get it working relatively easily. One thing I'm not so sure about is the usage of __sk_buff. Clearly, I need to pass the struct sk_buff internally for bpf_skb_load_bytes() to work. However, using __sk_buff seems a bit strange since I only let the program access the len field. Perhaps adding a __wifi_sk_buff would work, but then clang will probably complain if you pass that to bpf_skb_load_bytes(). The alternative will be to add any wifi fields we might need eventually to __sk_buff, which perhaps isn't such a big deal since accesses are optimized away anyway through convert_ctx_access. Instead it may make more sense to just have a "wifi_info(skb, info)" function you can call, e.g. with a structure that has various fields and flags to see which you care about. johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: Wed, 12 Apr 2017 16:27:34 +0200 > This works. An example BPF program is here: > https://p.sipsolutions.net/ca32264f2b705e5e.txt ... > One thing I'm not so sure about is the usage of __sk_buff. ... > Instead it may make more sense to just have a "wifi_info(skb, info)" > function you can call, e.g. with a structure that has various fields > and flags to see which you care about. I would advise against this, let the parsing part use __sk_buff and therefore have maximum flexibility. You really cannot predict the future, so in my opinion it might be unwise to constrain at this point.
On Wed, 2017-04-12 at 11:19 -0400, David Miller wrote: > > > Instead it may make more sense to just have a "wifi_info(skb, > info)" > > function you can call, e.g. with a structure that has various > fields > > and flags to see which you care about. > > I would advise against this, let the parsing part use __sk_buff and > therefore have maximum flexibility. You really cannot predict the > future, so in my opinion it might be unwise to constrain at this > point. So my point with the wifi_info() function to call from the BPF program was just that putting something that's not already in struct sk_buff into __sk_buff doesn't really make a lot of sense - we still have to actually build it somewhere/somehow without knowing if it's going to be needed by the program. We already have things like prog->cb_access and prog->dst_needed now, but I'm not sure we want to blow that up much. At the same time, technically I *do* have the information in skb->cb, but the format thereof is something I really wouldn't want to let trickle into the ABI. Therefore, I think if somebody needs something like the bitrate, we should have a wifi_info() function that can return the bitrate (among other things) without having to pre-build the data. We can still cache it in mac80211 if multiple programs are there, dunno if that makes sense. Nevertheless, I think if I don't use __sk_buff as the program argument then things get really messy, so I'll stick to that, and avoid adding anything to it as much as possible. johannes
Hi Johannes, > Add a new program type for wifi monitor interfaces. This program > type can > * access __sk_buff, but only skb->len > * call bpf_skb_load_bytes() > > The program type is only enabled when something selects the new > Kconfig symbol WANT_BPF_WIFIMON, which will be done by mac80211 > in a follow-up patch. we used to stay away from WIFI naming in the kernel. Any reasoning why this is used instead of WLAN now? Regards Marcel
On Wed, Apr 12, 2017 at 05:30:40PM +0200, Johannes Berg wrote: > On Wed, 2017-04-12 at 11:19 -0400, David Miller wrote: > > > > > Instead it may make more sense to just have a "wifi_info(skb, > > info)" > > > function you can call, e.g. with a structure that has various > > fields > > > and flags to see which you care about. > > > > I would advise against this, let the parsing part use __sk_buff and > > therefore have maximum flexibility. You really cannot predict the > > future, so in my opinion it might be unwise to constrain at this > > point. > > So my point with the wifi_info() function to call from the BPF program > was just that putting something that's not already in struct sk_buff > into __sk_buff doesn't really make a lot of sense - we still have to > actually build it somewhere/somehow without knowing if it's going to be > needed by the program. We already have things like prog->cb_access and > prog->dst_needed now, but I'm not sure we want to blow that up much. > > At the same time, technically I *do* have the information in skb->cb, > but the format thereof is something I really wouldn't want to let > trickle into the ABI. Therefore, I think if somebody needs something > like the bitrate, we should have a wifi_info() function that can return > the bitrate (among other things) without having to pre-build the data. > We can still cache it in mac80211 if multiple programs are there, dunno > if that makes sense. > > Nevertheless, I think if I don't use __sk_buff as the program argument > then things get really messy, so I'll stick to that, and avoid adding > anything to it as much as possible. so today only 'len' field is needed, but the plan to add wifi specific stuff to bpf context? If so, adding these very wifi specific fields to __sk_buff will confuse other program types, so new program type (like you did) and new 'struct bpf_wifi_md' are probably cleaner. 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; }; 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.
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
On 04/18/2017 11:55 AM, Johannes Berg wrote: > 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. 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. >> 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. Yeah, *_is_valid_access() callbacks would need to reject these members for most of the program types. >> 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. 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. 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. 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. 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. >> 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
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
So actually, come to think of it ... > > 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. Given this, I think I'll actually make a __wifi_sk_buff. > The thing is that __wifi_sk_buff doesn't have much information that's > generally useful available Because I just realized that this isn't true. To make sense of the SKB beyond the 802.11 header, which may not be possible at all though due to encryption happening in software later, it will have to know a few things like whether or not it was encrypted and if the IV was stripped etc. Actually, perhaps we should just restrict it to just look at the header ;-) johannes
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h index 03bf223f18be..fcaba84128dd 100644 --- a/include/linux/bpf_types.h +++ b/include/linux/bpf_types.h @@ -16,6 +16,9 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_KPROBE, kprobe_prog_ops) BPF_PROG_TYPE(BPF_PROG_TYPE_TRACEPOINT, tracepoint_prog_ops) BPF_PROG_TYPE(BPF_PROG_TYPE_PERF_EVENT, perf_event_prog_ops) #endif +#ifdef CONFIG_BPF_WIFIMON +BPF_PROG_TYPE(BPF_PROG_TYPE_WIFIMON, wifimon_prog_ops) +#endif BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops) BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_ARRAY, percpu_array_map_ops) diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 1e062bb54eec..b0dfe8a79b3f 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -115,6 +115,7 @@ enum bpf_prog_type { BPF_PROG_TYPE_LWT_IN, BPF_PROG_TYPE_LWT_OUT, BPF_PROG_TYPE_LWT_XMIT, + BPF_PROG_TYPE_WIFIMON, }; enum bpf_attach_type { diff --git a/net/core/filter.c b/net/core/filter.c index ce2a19da8aa4..c20624c81f6b 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3415,3 +3415,44 @@ int sk_get_filter(struct sock *sk, struct sock_filter __user *ubuf, release_sock(sk); return ret; } + +#ifdef CONFIG_BPF_WIFIMON +static const struct bpf_func_proto * +wifimon_func_proto(enum bpf_func_id func_id) +{ + switch (func_id) { + case BPF_FUNC_skb_load_bytes: + return &bpf_skb_load_bytes_proto; + default: + return bpf_base_func_proto(func_id); + } +} + +static bool wifimon_is_valid_access(int off, int size, + enum bpf_access_type type, + enum bpf_reg_type *reg_type) +{ + if (type == BPF_WRITE) + return false; + + switch (off) { + case offsetof(struct __sk_buff, len): + break; + default: + return false; + } + /* The verifier guarantees that size > 0. */ + if (off % size != 0) + return false; + if (size != sizeof(__u32)) + return false; + + return true; +} + +const struct bpf_verifier_ops wifimon_prog_ops = { + .get_func_proto = wifimon_func_proto, + .is_valid_access = wifimon_is_valid_access, + .convert_ctx_access = bpf_convert_ctx_access, +}; +#endif diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig index 6c606120abfe..50ffebafce46 100644 --- a/net/wireless/Kconfig +++ b/net/wireless/Kconfig @@ -214,3 +214,11 @@ config LIB80211_DEBUG from lib80211. If unsure, say N. + +config WANT_BPF_WIFIMON + bool + +config BPF_WIFIMON + bool + default y + depends on WANT_BPF_WIFIMON && BPF_SYSCALL