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

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

[Johannes Berg]

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.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/linux/bpf_types.h |  3 +++
 include/uapi/linux/bpf.h  |  1 +
 net/core/filter.c         | 41 +++++++++++++++++++++++++++++++++++++++++
 net/wireless/Kconfig      |  8 ++++++++
 4 files changed, 53 insertions(+)

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

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

[Johannes Berg]

From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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.

Signed-off-by: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 include/linux/bpf_types.h |  3 +++
 include/uapi/linux/bpf.h  |  1 +
 net/core/filter.c         | 41 +++++++++++++++++++++++++++++++++++++++++
 net/wireless/Kconfig      |  8 ++++++++
 4 files changed, 53 insertions(+)

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

[RFC 2/3] cfg80211: add API to attach monitor filter program

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Some people have suggested that the nl80211 API to look at
management frames should be extended to allow multiple
registrations, in order to allow applications other than
the usual controllers (wpa_supplicant/hostapd) to look out
for frames, e.g. for statistics or similar. Suggestions to
use monitor interfaces were rejected, because the overhead
is too big, even when a socket filter is installed. Adding
this to the nl80211 API is, however, problematic because
it then becomes undefined who is responsible for a frame,
e.g. when action frames have multiple listeners - but only
one can be responsible for handling them.

To solve this problem, we can allow installing a BPF filter
program that gets run on the raw 802.11 frame received from
the driver, before the frame is even considered for copying
the SKB or building the radiotap header for a given monitor
interface.

Add the necessary API to attach a monitor filter program
to a newly created monitor interface, or allow changing
it. Also allow clearing it by passing -1 as the FD.

NOTE/WARNING
We really should first figure out what we want to do with
802.3 frames that are received by the driver. Do they at
all show up on monitor? For the use case described above,
that doesn't matter - however, it's easy to imagine other
use cases, like using BPF for packet statistics (like the
never-merged per-rate statistics), and then seeing data
frames as well would be good. Perhaps the program can get
a different metadata struct rather than __sk_buff, that
indiciates whether the frames has 802.11 or 802.3 format?
However, if it already has 802.3 format, the TA might no
longer be present (in AP -> STA frames), which would then
no longer allow collecting such statistics anyway ...

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/cfg80211.h       |  3 +++
 include/uapi/linux/nl80211.h |  7 +++++++
 net/wireless/core.c          |  5 +++++
 net/wireless/nl80211.c       | 27 +++++++++++++++++++++++++++
 4 files changed, 42 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index a8faf9f0cac2..a59a8917a74c 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -376,6 +376,8 @@ static inline void wiphy_read_of_freq_limits(struct wiphy *wiphy)
  *	belonging to that MU-MIMO groupID; %NULL if not changed
  * @vht_mumimo_follow_addr: MU-MIMO follow address, used for monitoring
  *	MU-MIMO packets going to the specified station; %NULL if not changed
+ * @filter: wifi monitor BPF program, %NULL if not changed, an ERR_PTR()
+ *	if the program should be removed
  */
 struct vif_params {
 	u32 flags;
@@ -383,6 +385,7 @@ struct vif_params {
 	u8 macaddr[ETH_ALEN];
 	const u8 *vht_mumimo_groups;
 	const u8 *vht_mumimo_follow_addr;
+	struct bpf_prog *filter;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 5ed257c4cd4e..4a33cac2a41a 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2012,6 +2012,8 @@ enum nl80211_commands {
  *	u32 attribute with an &enum nl80211_timeout_reason value. This is used,
  *	e.g., with %NL80211_CMD_CONNECT event.
  *
+ * @NL80211_ATTR_BPF_FD: BPF file descriptor (s32), use -1 to remove a program
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2423,6 +2425,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_TIMEOUT_REASON,
 
+	NL80211_ATTR_BPF_FD,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -4753,6 +4757,8 @@ enum nl80211_feature_flags {
  * @NL80211_EXT_FEATURE_SCHED_SCAN_RELATIVE_RSSI: The driver supports sched_scan
  *	for reporting BSSs with better RSSI than the current connected BSS
  *	(%NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI).
+ * @NL80211_EXT_FEATURE_WIFIMON_BPF: supports running BPF filter programs
+ *	for frames seen on monitor interfaces
  *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -4771,6 +4777,7 @@ enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_MGMT_TX_RANDOM_TA,
 	NL80211_EXT_FEATURE_MGMT_TX_RANDOM_TA_CONNECTED,
 	NL80211_EXT_FEATURE_SCHED_SCAN_RELATIVE_RSSI,
+	NL80211_EXT_FEATURE_WIFIMON_BPF,
 
 	/* add new features before the definition below */
 	NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/core.c b/net/wireless/core.c
index e55e05bc4805..6d9a38e9a375 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -691,6 +691,11 @@ int wiphy_register(struct wiphy *wiphy)
 		    (wiphy->bss_select_support & ~(BIT(__NL80211_BSS_SELECT_ATTR_AFTER_LAST) - 2))))
 		return -EINVAL;
 
+	if (WARN_ON(!IS_ENABLED(CONFIG_BPF_WIFIMON) &&
+		    wiphy_ext_feature_isset(wiphy,
+					    NL80211_EXT_FEATURE_WIFIMON_BPF)))
+		return -EINVAL;
+
 	if (wiphy->addresses)
 		memcpy(wiphy->perm_addr, wiphy->addresses[0].addr, ETH_ALEN);
 
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 9516840b6e5f..505e6db633c6 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -17,6 +17,7 @@
 #include <linux/rtnetlink.h>
 #include <linux/netlink.h>
 #include <linux/etherdevice.h>
+#include <linux/bpf.h>
 #include <net/net_namespace.h>
 #include <net/genetlink.h>
 #include <net/cfg80211.h>
@@ -410,6 +411,7 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 		.len = sizeof(struct nl80211_bss_select_rssi_adjust)
 	},
 	[NL80211_ATTR_TIMEOUT_REASON] = { .type = NLA_U32 },
+	[NL80211_ATTR_BPF_FD] = { .type = NLA_U32 },
 };
 
 /* policy for the key attributes */
@@ -2768,6 +2770,28 @@ static int nl80211_parse_mon_options(struct cfg80211_registered_device *rdev,
 		change = true;
 	}
 
+	if (info->attrs[NL80211_ATTR_BPF_FD]) {
+		struct bpf_prog *prog = ERR_PTR(-ENODATA);
+		int fd = nla_get_s32(info->attrs[NL80211_ATTR_BPF_FD]);
+		u32 cap_flag = NL80211_EXT_FEATURE_WIFIMON_BPF;
+
+		if (!IS_ENABLED(CONFIG_BPF_WIFIMON) ||
+		    !wiphy_ext_feature_isset(&rdev->wiphy, cap_flag))
+			return -EOPNOTSUPP;
+
+		if (type != NL80211_IFTYPE_MONITOR)
+			return -EINVAL;
+
+		if (fd >= 0) {
+			prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_WIFIMON);
+			if (IS_ERR(prog))
+				return PTR_ERR(prog);
+		}
+
+		params->filter = prog;
+		change = true;
+	}
+
 	return change ? 1 : 0;
 }
 
@@ -2857,6 +2881,9 @@ static int nl80211_set_interface(struct sk_buff *skb, struct genl_info *info)
 	else
 		err = 0;
 
+	if (err && params.filter)
+		bpf_prog_put(params.filter);
+
 	if (!err && params.use_4addr != -1)
 		dev->ieee80211_ptr->use_4addr = params.use_4addr;
 
-- 
2.11.0

[RFC 3/3] mac80211: support bpf monitor filter

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Add the necessary hooks for running monitor filter programs
in mac80211's RX path, before a frame is handed off to a
monitor interface. If the frame isn't accepted then this
will save the overhead of creating a new SKB and building
the radiotap header.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/Kconfig       |  1 +
 net/mac80211/cfg.c         | 13 +++++++++++++
 net/mac80211/ieee80211_i.h |  6 ++++++
 net/mac80211/iface.c       |  9 ++++++++-
 net/mac80211/main.c        |  3 +++
 net/mac80211/rx.c          | 45 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/Kconfig b/net/mac80211/Kconfig
index 76e30f4797fb..080e0c705c72 100644
--- a/net/mac80211/Kconfig
+++ b/net/mac80211/Kconfig
@@ -8,6 +8,7 @@ config MAC80211
 	select CRYPTO_GCM
 	select CRYPTO_CMAC
 	select CRC32
+	select WANT_BPF_WIFIMON
 	---help---
 	  This option enables the hardware independent IEEE 802.11
 	  networking stack.
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 4ff03c88022e..c394c08ed0e0 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -106,6 +106,19 @@ static int ieee80211_set_mon_options(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
+	if (params->filter) {
+		struct bpf_prog *old = rtnl_dereference(sdata->u.mntr.filter);
+
+		if (IS_ERR(params->filter))
+			RCU_INIT_POINTER(sdata->u.mntr.filter, NULL);
+		else
+			rcu_assign_pointer(sdata->u.mntr.filter,
+					   params->filter);
+
+		if (old)
+			bpf_prog_put(old);
+	}
+
 	return 0;
 }
 
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 0e718437d080..06a2e2cdde83 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -27,6 +27,8 @@
 #include <linux/leds.h>
 #include <linux/idr.h>
 #include <linux/rhashtable.h>
+#include <linux/filter.h>
+#include <linux/bpf.h>
 #include <net/ieee80211_radiotap.h>
 #include <net/cfg80211.h>
 #include <net/mac80211.h>
@@ -839,6 +841,10 @@ struct txq_info {
 struct ieee80211_if_mntr {
 	u32 flags;
 	u8 mu_follow_addr[ETH_ALEN] __aligned(2);
+#ifdef CONFIG_BPF_WIFIMON
+	struct bpf_prog *filter;
+	bool deliver;
+#endif
 };
 
 /**
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 5bb0c5012819..05258fd9dda2 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1107,8 +1107,15 @@ static void ieee80211_teardown_sdata(struct ieee80211_sub_if_data *sdata)
 		__skb_queue_purge(&sdata->fragments[i].skb_list);
 	sdata->fragment_next = 0;
 
-	if (ieee80211_vif_is_mesh(&sdata->vif))
+	if (ieee80211_vif_is_mesh(&sdata->vif)) {
 		ieee80211_mesh_teardown_sdata(sdata);
+	} else if (sdata->vif.type == NL80211_IFTYPE_MONITOR) {
+		struct bpf_prog *old = rtnl_dereference(sdata->u.mntr.filter);
+
+		RCU_INIT_POINTER(sdata->u.mntr.filter, NULL);
+		if (old)
+			bpf_prog_put(old);
+	}
 }
 
 static void ieee80211_uninit(struct net_device *dev)
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 56fb47953b72..e9d13dedcca4 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -551,6 +551,9 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 			   NL80211_FEATURE_FULL_AP_CLIENT_STATE;
 	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_FILS_STA);
 
+	if (IS_ENABLED(CONFIG_BPF_WIFIMON))
+		wiphy_ext_feature_isset(wiphy, NL80211_EXT_FEATURE_WIFIMON_BPF);
+
 	if (!ops->hw_scan)
 		wiphy->features |= NL80211_FEATURE_LOW_PRIORITY_SCAN |
 				   NL80211_FEATURE_AP_SCAN;
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 335c7843169f..8c811deaf3cd 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -508,6 +508,7 @@ ieee80211_rx_monitor(struct ieee80211_local *local, struct sk_buff *origskb,
 	struct ieee80211_mgmt *mgmt;
 	struct ieee80211_sub_if_data *monitor_sdata =
 		rcu_dereference(local->monitor_sdata);
+	bool deliver __maybe_unused = false;
 
 	if (unlikely(status->flag & RX_FLAG_RADIOTAP_VENDOR_DATA)) {
 		struct ieee80211_vendor_radiotap *rtap = (void *)origskb->data;
@@ -552,6 +553,45 @@ ieee80211_rx_monitor(struct ieee80211_local *local, struct sk_buff *origskb,
 		return origskb;
 	}
 
+#ifdef CONFIG_BPF_WIFIMON
+	/* pretend all the monitor info isn't there */
+	__pskb_pull(origskb, rtap_vendor_space);
+	origskb->len -= present_fcs_len;
+
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		const struct bpf_prog *filter;
+
+		if (sdata->vif.type != NL80211_IFTYPE_MONITOR)
+			continue;
+
+		if (sdata->u.mntr.flags & MONITOR_FLAG_COOK_FRAMES)
+			continue;
+
+		if (!ieee80211_sdata_running(sdata))
+			continue;
+
+		filter = rcu_dereference(sdata->u.mntr.filter);
+		if (filter) {
+			sdata->u.mntr.deliver = BPF_PROG_RUN(filter, origskb);
+			if (sdata->u.mntr.deliver)
+				deliver = true;
+		} else {
+			sdata->u.mntr.deliver = true;
+			deliver = true;
+		}
+	}
+
+	/* stop pretending the monitor info isn't there */
+	origskb->len += present_fcs_len;
+	__skb_push(origskb, rtap_vendor_space);
+
+	if (!deliver) {
+		remove_monitor_info(local, origskb, present_fcs_len,
+				    rtap_vendor_space);
+		return origskb;
+	}
+#endif
+
 	/* room for the radiotap header based on driver features */
 	rt_hdrlen = ieee80211_rx_radiotap_hdrlen(local, status, origskb);
 	needed_headroom = rt_hdrlen - rtap_vendor_space;
@@ -598,11 +638,16 @@ ieee80211_rx_monitor(struct ieee80211_local *local, struct sk_buff *origskb,
 		if (sdata->vif.type != NL80211_IFTYPE_MONITOR)
 			continue;
 
+#ifdef CONFIG_BPF_WIFIMON
+		if (!sdata->u.mntr.deliver)
+			continue;
+#else
 		if (sdata->u.mntr.flags & MONITOR_FLAG_COOK_FRAMES)
 			continue;
 
 		if (!ieee80211_sdata_running(sdata))
 			continue;
+#endif
 
 		if (prev_dev) {
 			skb2 = skb_clone(skb, GFP_ATOMIC);
-- 
2.11.0

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

[Johannes Berg]

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

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

[Johannes Berg]

On Wed, 2017-04-12 at 13:07 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> 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

Re: [RFC 3/3] mac80211: support bpf monitor filter

[Johannes Berg]

On Wed, 2017-04-12 at 13:07 +0200, Johannes Berg wrote:
> 
>  struct ieee80211_if_mntr {
>  	u32 flags;
> 
[...]
+	bool deliver;

That's ... broken for multi-queue RX. I haven't really found a good
other way to do it. The best way will likely be to copy the SKB the
first time it's needed, build the radiotap header, and then keep a
reference to it to be able to clone it later if it's needed again.

johannes

Re: [RFC 3/3] mac80211: support bpf monitor filter

[Johannes Berg]

On Wed, 2017-04-12 at 13:07 +0200, Johannes Berg wrote:
> 
>  struct ieee80211_if_mntr {
>  	u32 flags;
> 
[...]
+	bool deliver;

That's ... broken for multi-queue RX. I haven't really found a good
other way to do it. The best way will likely be to copy the SKB the
first time it's needed, build the radiotap header, and then keep a
reference to it to be able to clone it later if it's needed again.

johannes

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

[David Miller]

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.

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

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
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.

Re: [RFC 3/3] mac80211: support bpf monitor filter

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 12 Apr 2017 16:29:07 +0200

> On Wed, 2017-04-12 at 13:07 +0200, Johannes Berg wrote:
>> 
>>  struct ieee80211_if_mntr {
>>  	u32 flags;
>> 
> [...]
> +	bool deliver;
> 
> That's ... broken for multi-queue RX. I haven't really found a good
> other way to do it. The best way will likely be to copy the SKB the
> first time it's needed, build the radiotap header, and then keep a
> reference to it to be able to clone it later if it's needed again.

If you don't recurse into the receive path for different devices before
you are done with this boolean, simply make a global per-cpu boolean
and use that.

Re: [RFC 3/3] mac80211: support bpf monitor filter

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Wed, 12 Apr 2017 16:29:07 +0200

> On Wed, 2017-04-12 at 13:07 +0200, Johannes Berg wrote:
>> 
>>  struct ieee80211_if_mntr {
>>  	u32 flags;
>> 
> [...]
> +	bool deliver;
> 
> That's ... broken for multi-queue RX. I haven't really found a good
> other way to do it. The best way will likely be to copy the SKB the
> first time it's needed, build the radiotap header, and then keep a
> reference to it to be able to clone it later if it's needed again.

If you don't recurse into the receive path for different devices before
you are done with this boolean, simply make a global per-cpu boolean
and use that.

Re: [RFC 3/3] mac80211: support bpf monitor filter

[Johannes Berg]

On Wed, 2017-04-12 at 11:22 -0400, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Wed, 12 Apr 2017 16:29:07 +0200
> 
> > On Wed, 2017-04-12 at 13:07 +0200, Johannes Berg wrote:
> >> 
> >>  struct ieee80211_if_mntr {
> >>      u32 flags;
> >> 
> > [...]
> > +     bool deliver;
> > 
> > That's ... broken for multi-queue RX. I haven't really found a good
> > other way to do it. The best way will likely be to copy the SKB the
> > first time it's needed, build the radiotap header, and then keep a
> > reference to it to be able to clone it later if it's needed again.
> 
> If you don't recurse into the receive path for different devices
> before you are done with this boolean, simply make a global per-cpu
> boolean and use that.

No, that won't work. We don't recurse, but this is a per-interface
bool, as we can have multiple monitor interfaces (possibly with
different filters).

The problem comes from the fact that I did

for_each_interface()
	iface.deliver = run_bpf_program();

if (nobody_wanted_it)
	return;

skb = build_monitor_skb()

for_each_interface()
	if (iface.monitor)
		deliver(skb);


What I should be doing is something like this:

for_each_interface() {
	if (run_bpf_program()) {
		if (!skb)
			skb = build_monitor_skb();
		deliver(skb);
	}
}

where deliver() does skb_clone() internally or so.

johannes

Re: [RFC 3/3] mac80211: support bpf monitor filter

[Johannes Berg]

On Wed, 2017-04-12 at 11:22 -0400, David Miller wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Wed, 12 Apr 2017 16:29:07 +0200
> 
> > On Wed, 2017-04-12 at 13:07 +0200, Johannes Berg wrote:
> >> 
> >>  struct ieee80211_if_mntr {
> >>      u32 flags;
> >> 
> > [...]
> > +     bool deliver;
> > 
> > That's ... broken for multi-queue RX. I haven't really found a good
> > other way to do it. The best way will likely be to copy the SKB the
> > first time it's needed, build the radiotap header, and then keep a
> > reference to it to be able to clone it later if it's needed again.
> 
> If you don't recurse into the receive path for different devices
> before you are done with this boolean, simply make a global per-cpu
> boolean and use that.

No, that won't work. We don't recurse, but this is a per-interface
bool, as we can have multiple monitor interfaces (possibly with
different filters).

The problem comes from the fact that I did

for_each_interface()
	iface.deliver = run_bpf_program();

if (nobody_wanted_it)
	return;

skb = build_monitor_skb()

for_each_interface()
	if (iface.monitor)
		deliver(skb);


What I should be doing is something like this:

for_each_interface() {
	if (run_bpf_program()) {
		if (!skb)
			skb = build_monitor_skb();
		deliver(skb);
	}
}

where deliver() does skb_clone() internally or so.

johannes

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

[Johannes Berg]

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

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

[Marcel Holtmann]

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

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

[Marcel Holtmann]

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

Re: [RFC 3/3] mac80211: support bpf monitor filter

[Johannes Berg]

> @@ -551,6 +551,9 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t
> priv_data_len,
>  			   NL80211_FEATURE_FULL_AP_CLIENT_STATE;
>  	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_FILS_STA);
>  
> +	if (IS_ENABLED(CONFIG_BPF_WIFIMON))
> +		wiphy_ext_feature_isset(wiphy,
> NL80211_EXT_FEATURE_WIFIMON_BPF);
> 
That obviously needs to be _set(), not _isset().

johannes

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

[Alexei Starovoitov]

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.

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

[Alexei Starovoitov]

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.

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

[Johannes Berg]

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

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

[Daniel Borkmann]

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

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

[Johannes Berg]

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

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

[Johannes Berg]

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

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

[Johannes Berg]

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