[PATCH bpf-next v4 0/6] Support defragmenting IPv(4|6) packets in BPF

[PATCH bpf-next v4 0/6] Support defragmenting IPv(4|6) packets in BPF

[Daniel Xu]

=== Context ===

In the context of a middlebox, fragmented packets are tricky to handle.
The full 5-tuple of a packet is often only available in the first
fragment which makes enforcing consistent policy difficult. There are
really only two stateless options, neither of which are very nice:

1. Enforce policy on first fragment and accept all subsequent fragments.
   This works but may let in certain attacks or allow data exfiltration.

2. Enforce policy on first fragment and drop all subsequent fragments.
   This does not really work b/c some protocols may rely on
   fragmentation. For example, DNS may rely on oversized UDP packets for
   large responses.

So stateful tracking is the only sane option. RFC 8900 [0] calls this
out as well in section 6.3:

    Middleboxes [...] should process IP fragments in a manner that is
    consistent with [RFC0791] and [RFC8200]. In many cases, middleboxes
    must maintain state in order to achieve this goal.

=== BPF related bits ===

Policy has traditionally been enforced from XDP/TC hooks. Both hooks
run before kernel reassembly facilities. However, with the new
BPF_PROG_TYPE_NETFILTER, we can rather easily hook into existing
netfilter reassembly infra.

The basic idea is we bump a refcnt on the netfilter defrag module and
then run the bpf prog after the defrag module runs. This allows bpf
progs to transparently see full, reassembled packets. The nice thing
about this is that progs don't have to carry around logic to detect
fragments.

=== Changelog ===

Changes from v3:
* Correctly initialize `addrlen` stack var for recvmsg()

Changes from v2:

* module_put() if ->enable() fails
* Fix CI build errors

Changes from v1:

* Drop bpf_program__attach_netfilter() patches
* static -> static const where appropriate
* Fix callback assignment order during registration
* Only request_module() if callbacks are missing
* Fix retval when modprobe fails in userspace
* Fix v6 defrag module name (nf_defrag_ipv6_hooks -> nf_defrag_ipv6)
* Simplify priority checking code
* Add warning if module doesn't assign callbacks in the future
* Take refcnt on module while defrag link is active


[0]: https://datatracker.ietf.org/doc/html/rfc8900


Daniel Xu (6):
  netfilter: defrag: Add glue hooks for enabling/disabling defrag
  netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link
  netfilter: bpf: Prevent defrag module unload while link active
  bpf: selftests: Support not connecting client socket
  bpf: selftests: Support custom type and proto for client sockets
  bpf: selftests: Add defrag selftests

 include/linux/netfilter.h                     |  15 +
 include/uapi/linux/bpf.h                      |   5 +
 net/ipv4/netfilter/nf_defrag_ipv4.c           |  17 +-
 net/ipv6/netfilter/nf_defrag_ipv6_hooks.c     |  11 +
 net/netfilter/core.c                          |   6 +
 net/netfilter/nf_bpf_link.c                   | 150 +++++++++-
 tools/include/uapi/linux/bpf.h                |   5 +
 tools/testing/selftests/bpf/Makefile          |   4 +-
 .../selftests/bpf/generate_udp_fragments.py   |  90 ++++++
 .../selftests/bpf/ip_check_defrag_frags.h     |  57 ++++
 tools/testing/selftests/bpf/network_helpers.c |  26 +-
 tools/testing/selftests/bpf/network_helpers.h |   3 +
 .../bpf/prog_tests/ip_check_defrag.c          | 283 ++++++++++++++++++
 .../selftests/bpf/progs/ip_check_defrag.c     | 104 +++++++
 14 files changed, 754 insertions(+), 22 deletions(-)
 create mode 100755 tools/testing/selftests/bpf/generate_udp_fragments.py
 create mode 100644 tools/testing/selftests/bpf/ip_check_defrag_frags.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/ip_check_defrag.c
 create mode 100644 tools/testing/selftests/bpf/progs/ip_check_defrag.c

-- 
2.41.0

[PATCH bpf-next v4 1/6] netfilter: defrag: Add glue hooks for enabling/disabling defrag

[Daniel Xu]

We want to be able to enable/disable IP packet defrag from core
bpf/netfilter code. In other words, execute code from core that could
possibly be built as a module.

To help avoid symbol resolution errors, use glue hooks that the modules
will register callbacks with during module init.

Reviewed-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 include/linux/netfilter.h                 | 12 ++++++++++++
 net/ipv4/netfilter/nf_defrag_ipv4.c       | 16 +++++++++++++++-
 net/ipv6/netfilter/nf_defrag_ipv6_hooks.c | 10 ++++++++++
 net/netfilter/core.c                      |  6 ++++++
 4 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index d4fed4c508ca..77a637b681f2 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -481,6 +481,18 @@ struct nfnl_ct_hook {
 };
 extern const struct nfnl_ct_hook __rcu *nfnl_ct_hook;
 
+struct nf_defrag_v4_hook {
+	int (*enable)(struct net *net);
+	void (*disable)(struct net *net);
+};
+extern const struct nf_defrag_v4_hook __rcu *nf_defrag_v4_hook;
+
+struct nf_defrag_v6_hook {
+	int (*enable)(struct net *net);
+	void (*disable)(struct net *net);
+};
+extern const struct nf_defrag_v6_hook __rcu *nf_defrag_v6_hook;
+
 /*
  * nf_skb_duplicated - TEE target has sent a packet
  *
diff --git a/net/ipv4/netfilter/nf_defrag_ipv4.c b/net/ipv4/netfilter/nf_defrag_ipv4.c
index e61ea428ea18..1f3e0e893b7a 100644
--- a/net/ipv4/netfilter/nf_defrag_ipv4.c
+++ b/net/ipv4/netfilter/nf_defrag_ipv4.c
@@ -7,6 +7,7 @@
 #include <linux/ip.h>
 #include <linux/netfilter.h>
 #include <linux/module.h>
+#include <linux/rcupdate.h>
 #include <linux/skbuff.h>
 #include <net/netns/generic.h>
 #include <net/route.h>
@@ -113,17 +114,30 @@ static void __net_exit defrag4_net_exit(struct net *net)
 	}
 }
 
+static const struct nf_defrag_v4_hook defrag_hook = {
+	.enable = nf_defrag_ipv4_enable,
+	.disable = nf_defrag_ipv4_disable,
+};
+
 static struct pernet_operations defrag4_net_ops = {
 	.exit = defrag4_net_exit,
 };
 
 static int __init nf_defrag_init(void)
 {
-	return register_pernet_subsys(&defrag4_net_ops);
+	int err;
+
+	err = register_pernet_subsys(&defrag4_net_ops);
+	if (err)
+		return err;
+
+	rcu_assign_pointer(nf_defrag_v4_hook, &defrag_hook);
+	return err;
 }
 
 static void __exit nf_defrag_fini(void)
 {
+	rcu_assign_pointer(nf_defrag_v4_hook, NULL);
 	unregister_pernet_subsys(&defrag4_net_ops);
 }
 
diff --git a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
index cb4eb1d2c620..f7c7ee31c472 100644
--- a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
+++ b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/skbuff.h>
 #include <linux/icmp.h>
+#include <linux/rcupdate.h>
 #include <linux/sysctl.h>
 #include <net/ipv6_frag.h>
 
@@ -96,6 +97,11 @@ static void __net_exit defrag6_net_exit(struct net *net)
 	}
 }
 
+static const struct nf_defrag_v6_hook defrag_hook = {
+	.enable = nf_defrag_ipv6_enable,
+	.disable = nf_defrag_ipv6_disable,
+};
+
 static struct pernet_operations defrag6_net_ops = {
 	.exit = defrag6_net_exit,
 };
@@ -114,6 +120,9 @@ static int __init nf_defrag_init(void)
 		pr_err("nf_defrag_ipv6: can't register pernet ops\n");
 		goto cleanup_frag6;
 	}
+
+	rcu_assign_pointer(nf_defrag_v6_hook, &defrag_hook);
+
 	return ret;
 
 cleanup_frag6:
@@ -124,6 +133,7 @@ static int __init nf_defrag_init(void)
 
 static void __exit nf_defrag_fini(void)
 {
+	rcu_assign_pointer(nf_defrag_v6_hook, NULL);
 	unregister_pernet_subsys(&defrag6_net_ops);
 	nf_ct_frag6_cleanup();
 }
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 5f76ae86a656..34845155bb85 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -680,6 +680,12 @@ EXPORT_SYMBOL_GPL(nfnl_ct_hook);
 const struct nf_ct_hook __rcu *nf_ct_hook __read_mostly;
 EXPORT_SYMBOL_GPL(nf_ct_hook);
 
+const struct nf_defrag_v4_hook __rcu *nf_defrag_v4_hook __read_mostly;
+EXPORT_SYMBOL_GPL(nf_defrag_v4_hook);
+
+const struct nf_defrag_v6_hook __rcu *nf_defrag_v6_hook __read_mostly;
+EXPORT_SYMBOL_GPL(nf_defrag_v6_hook);
+
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
 u8 nf_ctnetlink_has_listener;
 EXPORT_SYMBOL_GPL(nf_ctnetlink_has_listener);
-- 
2.41.0

[PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link

[Daniel Xu]

This commit adds support for enabling IP defrag using pre-existing
netfilter defrag support. Basically all the flag does is bump a refcnt
while the link the active. Checks are also added to ensure the prog
requesting defrag support is run _after_ netfilter defrag hooks.

Reviewed-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 include/uapi/linux/bpf.h       |   5 ++
 net/netfilter/nf_bpf_link.c    | 129 ++++++++++++++++++++++++++++++---
 tools/include/uapi/linux/bpf.h |   5 ++
 3 files changed, 128 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 600d0caebbd8..c820076c38db 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1180,6 +1180,11 @@ enum bpf_perf_event_type {
  */
 #define BPF_F_KPROBE_MULTI_RETURN	(1U << 0)
 
+/* link_create.netfilter.flags used in LINK_CREATE command for
+ * BPF_PROG_TYPE_NETFILTER to enable IP packet defragmentation.
+ */
+#define BPF_F_NETFILTER_IP_DEFRAG (1U << 0)
+
 /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
  * the following extensions:
  *
diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
index c36da56d756f..5b72aa246577 100644
--- a/net/netfilter/nf_bpf_link.c
+++ b/net/netfilter/nf_bpf_link.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/bpf.h>
 #include <linux/filter.h>
+#include <linux/kmod.h>
 #include <linux/netfilter.h>
 
 #include <net/netfilter/nf_bpf_link.h>
@@ -23,8 +24,98 @@ struct bpf_nf_link {
 	struct nf_hook_ops hook_ops;
 	struct net *net;
 	u32 dead;
+	bool defrag;
 };
 
+static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
+{
+	const struct nf_defrag_v4_hook __maybe_unused *v4_hook;
+	const struct nf_defrag_v6_hook __maybe_unused *v6_hook;
+	int err;
+
+	switch (link->hook_ops.pf) {
+#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
+	case NFPROTO_IPV4:
+		rcu_read_lock();
+		v4_hook = rcu_dereference(nf_defrag_v4_hook);
+		if (!v4_hook) {
+			rcu_read_unlock();
+			err = request_module("nf_defrag_ipv4");
+			if (err)
+				return err < 0 ? err : -EINVAL;
+
+			rcu_read_lock();
+			v4_hook = rcu_dereference(nf_defrag_v4_hook);
+			if (!v4_hook) {
+				WARN_ONCE(1, "nf_defrag_ipv4 bad registration");
+				err = -ENOENT;
+				goto out_v4;
+			}
+		}
+
+		err = v4_hook->enable(link->net);
+out_v4:
+		rcu_read_unlock();
+		return err;
+#endif
+#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
+	case NFPROTO_IPV6:
+		rcu_read_lock();
+		v6_hook = rcu_dereference(nf_defrag_v6_hook);
+		if (!v6_hook) {
+			rcu_read_unlock();
+			err = request_module("nf_defrag_ipv6");
+			if (err)
+				return err < 0 ? err : -EINVAL;
+
+			rcu_read_lock();
+			v6_hook = rcu_dereference(nf_defrag_v6_hook);
+			if (!v6_hook) {
+				WARN_ONCE(1, "nf_defrag_ipv6_hooks bad registration");
+				err = -ENOENT;
+				goto out_v6;
+			}
+		}
+
+		err = v6_hook->enable(link->net);
+out_v6:
+		rcu_read_unlock();
+		return err;
+#endif
+	default:
+		return -EAFNOSUPPORT;
+	}
+}
+
+static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
+{
+	const struct nf_defrag_v4_hook __maybe_unused *v4_hook;
+	const struct nf_defrag_v6_hook __maybe_unused *v6_hook;
+
+	switch (link->hook_ops.pf) {
+#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
+	case NFPROTO_IPV4:
+		rcu_read_lock();
+		v4_hook = rcu_dereference(nf_defrag_v4_hook);
+		if (v4_hook)
+			v4_hook->disable(link->net);
+		rcu_read_unlock();
+
+		break;
+#endif
+#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
+	case NFPROTO_IPV6:
+		rcu_read_lock();
+		v6_hook = rcu_dereference(nf_defrag_v6_hook);
+		if (v6_hook)
+			v6_hook->disable(link->net);
+		rcu_read_unlock();
+
+		break;
+	}
+#endif
+}
+
 static void bpf_nf_link_release(struct bpf_link *link)
 {
 	struct bpf_nf_link *nf_link = container_of(link, struct bpf_nf_link, link);
@@ -37,6 +128,9 @@ static void bpf_nf_link_release(struct bpf_link *link)
 	 */
 	if (!cmpxchg(&nf_link->dead, 0, 1))
 		nf_unregister_net_hook(nf_link->net, &nf_link->hook_ops);
+
+	if (nf_link->defrag)
+		bpf_nf_disable_defrag(nf_link);
 }
 
 static void bpf_nf_link_dealloc(struct bpf_link *link)
@@ -92,6 +186,8 @@ static const struct bpf_link_ops bpf_nf_link_lops = {
 
 static int bpf_nf_check_pf_and_hooks(const union bpf_attr *attr)
 {
+	int prio;
+
 	switch (attr->link_create.netfilter.pf) {
 	case NFPROTO_IPV4:
 	case NFPROTO_IPV6:
@@ -102,19 +198,18 @@ static int bpf_nf_check_pf_and_hooks(const union bpf_attr *attr)
 		return -EAFNOSUPPORT;
 	}
 
-	if (attr->link_create.netfilter.flags)
+	if (attr->link_create.netfilter.flags & ~BPF_F_NETFILTER_IP_DEFRAG)
 		return -EOPNOTSUPP;
 
-	/* make sure conntrack confirm is always last.
-	 *
-	 * In the future, if userspace can e.g. request defrag, then
-	 * "defrag_requested && prio before NF_IP_PRI_CONNTRACK_DEFRAG"
-	 * should fail.
-	 */
-	switch (attr->link_create.netfilter.priority) {
-	case NF_IP_PRI_FIRST: return -ERANGE; /* sabotage_in and other warts */
-	case NF_IP_PRI_LAST: return -ERANGE; /* e.g. conntrack confirm */
-	}
+	/* make sure conntrack confirm is always last */
+	prio = attr->link_create.netfilter.priority;
+	if (prio == NF_IP_PRI_FIRST)
+		return -ERANGE;  /* sabotage_in and other warts */
+	else if (prio == NF_IP_PRI_LAST)
+		return -ERANGE;  /* e.g. conntrack confirm */
+	else if ((attr->link_create.netfilter.flags & BPF_F_NETFILTER_IP_DEFRAG) &&
+		 prio <= NF_IP_PRI_CONNTRACK_DEFRAG)
+		return -ERANGE;  /* cannot use defrag if prog runs before nf_defrag */
 
 	return 0;
 }
@@ -156,6 +251,18 @@ int bpf_nf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 		return err;
 	}
 
+	if (attr->link_create.netfilter.flags & BPF_F_NETFILTER_IP_DEFRAG) {
+		err = bpf_nf_enable_defrag(link);
+		if (err) {
+			bpf_link_cleanup(&link_primer);
+			return err;
+		}
+		/* only mark defrag enabled if enabling succeeds so cleanup path
+		 * doesn't disable without a corresponding enable
+		 */
+		link->defrag = true;
+	}
+
 	err = nf_register_net_hook(net, &link->hook_ops);
 	if (err) {
 		bpf_link_cleanup(&link_primer);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 600d0caebbd8..c820076c38db 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1180,6 +1180,11 @@ enum bpf_perf_event_type {
  */
 #define BPF_F_KPROBE_MULTI_RETURN	(1U << 0)
 
+/* link_create.netfilter.flags used in LINK_CREATE command for
+ * BPF_PROG_TYPE_NETFILTER to enable IP packet defragmentation.
+ */
+#define BPF_F_NETFILTER_IP_DEFRAG (1U << 0)
+
 /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
  * the following extensions:
  *
-- 
2.41.0

[PATCH bpf-next v4 3/6] netfilter: bpf: Prevent defrag module unload while link active

[Daniel Xu]

While in practice we could handle the module being unloaded while a
netfilter link (that requested defrag) was active, it's a better user
experience to prevent the defrag module from going away. It would
violate user expectations if fragmented packets started showing up if
some other part of the system tried to unload defrag module.

Reviewed-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 include/linux/netfilter.h                 |  3 +++
 net/ipv4/netfilter/nf_defrag_ipv4.c       |  1 +
 net/ipv6/netfilter/nf_defrag_ipv6_hooks.c |  1 +
 net/netfilter/nf_bpf_link.c               | 25 +++++++++++++++++++++--
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 77a637b681f2..a160dc1e23bf 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -11,6 +11,7 @@
 #include <linux/wait.h>
 #include <linux/list.h>
 #include <linux/static_key.h>
+#include <linux/module.h>
 #include <linux/netfilter_defs.h>
 #include <linux/netdevice.h>
 #include <linux/sockptr.h>
@@ -482,12 +483,14 @@ struct nfnl_ct_hook {
 extern const struct nfnl_ct_hook __rcu *nfnl_ct_hook;
 
 struct nf_defrag_v4_hook {
+	struct module *owner;
 	int (*enable)(struct net *net);
 	void (*disable)(struct net *net);
 };
 extern const struct nf_defrag_v4_hook __rcu *nf_defrag_v4_hook;
 
 struct nf_defrag_v6_hook {
+	struct module *owner;
 	int (*enable)(struct net *net);
 	void (*disable)(struct net *net);
 };
diff --git a/net/ipv4/netfilter/nf_defrag_ipv4.c b/net/ipv4/netfilter/nf_defrag_ipv4.c
index 1f3e0e893b7a..fb133bf3131d 100644
--- a/net/ipv4/netfilter/nf_defrag_ipv4.c
+++ b/net/ipv4/netfilter/nf_defrag_ipv4.c
@@ -115,6 +115,7 @@ static void __net_exit defrag4_net_exit(struct net *net)
 }
 
 static const struct nf_defrag_v4_hook defrag_hook = {
+	.owner = THIS_MODULE,
 	.enable = nf_defrag_ipv4_enable,
 	.disable = nf_defrag_ipv4_disable,
 };
diff --git a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
index f7c7ee31c472..29d31721c9c0 100644
--- a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
+++ b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
@@ -98,6 +98,7 @@ static void __net_exit defrag6_net_exit(struct net *net)
 }
 
 static const struct nf_defrag_v6_hook defrag_hook = {
+	.owner = THIS_MODULE,
 	.enable = nf_defrag_ipv6_enable,
 	.disable = nf_defrag_ipv6_disable,
 };
diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
index 5b72aa246577..77ffbf26ba3d 100644
--- a/net/netfilter/nf_bpf_link.c
+++ b/net/netfilter/nf_bpf_link.c
@@ -2,6 +2,7 @@
 #include <linux/bpf.h>
 #include <linux/filter.h>
 #include <linux/kmod.h>
+#include <linux/module.h>
 #include <linux/netfilter.h>
 
 #include <net/netfilter/nf_bpf_link.h>
@@ -53,7 +54,15 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
 			}
 		}
 
+		/* Prevent defrag module from going away while in use */
+		if (!try_module_get(v4_hook->owner)) {
+			err = -ENOENT;
+			goto out_v4;
+		}
+
 		err = v4_hook->enable(link->net);
+		if (err)
+			module_put(v4_hook->owner);
 out_v4:
 		rcu_read_unlock();
 		return err;
@@ -77,7 +86,15 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
 			}
 		}
 
+		/* Prevent defrag module from going away while in use */
+		if (!try_module_get(v6_hook->owner)) {
+			err = -ENOENT;
+			goto out_v6;
+		}
+
 		err = v6_hook->enable(link->net);
+		if (err)
+			module_put(v6_hook->owner);
 out_v6:
 		rcu_read_unlock();
 		return err;
@@ -97,8 +114,10 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
 	case NFPROTO_IPV4:
 		rcu_read_lock();
 		v4_hook = rcu_dereference(nf_defrag_v4_hook);
-		if (v4_hook)
+		if (v4_hook) {
 			v4_hook->disable(link->net);
+			module_put(v4_hook->owner);
+		}
 		rcu_read_unlock();
 
 		break;
@@ -107,8 +126,10 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
 	case NFPROTO_IPV6:
 		rcu_read_lock();
 		v6_hook = rcu_dereference(nf_defrag_v6_hook);
-		if (v6_hook)
+		if (v6_hook) {
 			v6_hook->disable(link->net);
+			module_put(v6_hook->owner);
+		}
 		rcu_read_unlock();
 
 		break;
-- 
2.41.0

Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link

[Alexei Starovoitov]

On Wed, Jul 12, 2023 at 4:44 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> +#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
> +       case NFPROTO_IPV6:
> +               rcu_read_lock();
> +               v6_hook = rcu_dereference(nf_defrag_v6_hook);
> +               if (!v6_hook) {
> +                       rcu_read_unlock();
> +                       err = request_module("nf_defrag_ipv6");
> +                       if (err)
> +                               return err < 0 ? err : -EINVAL;
> +
> +                       rcu_read_lock();
> +                       v6_hook = rcu_dereference(nf_defrag_v6_hook);
> +                       if (!v6_hook) {
> +                               WARN_ONCE(1, "nf_defrag_ipv6_hooks bad registration");
> +                               err = -ENOENT;
> +                               goto out_v6;
> +                       }
> +               }
> +
> +               err = v6_hook->enable(link->net);

I was about to apply, but luckily caught this issue in my local test:

[   18.462448] BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:283
[   18.463238] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid:
2042, name: test_progs
[   18.463927] preempt_count: 0, expected: 0
[   18.464249] RCU nest depth: 1, expected: 0
[   18.464631] CPU: 15 PID: 2042 Comm: test_progs Tainted: G
O       6.4.0-04319-g6f6ec4fa00dc #4896
[   18.465480] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
[   18.466531] Call Trace:
[   18.466767]  <TASK>
[   18.466975]  dump_stack_lvl+0x32/0x40
[   18.467325]  __might_resched+0x129/0x180
[   18.467691]  mutex_lock+0x1a/0x40
[   18.468057]  nf_defrag_ipv4_enable+0x16/0x70
[   18.468467]  bpf_nf_link_attach+0x141/0x300
[   18.468856]  __sys_bpf+0x133e/0x26d0

You cannot call mutex under rcu_read_lock.

Please make sure you have all kernel debug flags on in your testing.

Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link

[Daniel Xu]

Hi Alexei,

On Wed, Jul 12, 2023 at 05:43:49PM -0700, Alexei Starovoitov wrote:
> On Wed, Jul 12, 2023 at 4:44 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > +#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
> > +       case NFPROTO_IPV6:
> > +               rcu_read_lock();
> > +               v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > +               if (!v6_hook) {
> > +                       rcu_read_unlock();
> > +                       err = request_module("nf_defrag_ipv6");
> > +                       if (err)
> > +                               return err < 0 ? err : -EINVAL;
> > +
> > +                       rcu_read_lock();
> > +                       v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > +                       if (!v6_hook) {
> > +                               WARN_ONCE(1, "nf_defrag_ipv6_hooks bad registration");
> > +                               err = -ENOENT;
> > +                               goto out_v6;
> > +                       }
> > +               }
> > +
> > +               err = v6_hook->enable(link->net);
> 
> I was about to apply, but luckily caught this issue in my local test:
> 
> [   18.462448] BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:283
> [   18.463238] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid:
> 2042, name: test_progs
> [   18.463927] preempt_count: 0, expected: 0
> [   18.464249] RCU nest depth: 1, expected: 0
> [   18.464631] CPU: 15 PID: 2042 Comm: test_progs Tainted: G
> O       6.4.0-04319-g6f6ec4fa00dc #4896
> [   18.465480] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> [   18.466531] Call Trace:
> [   18.466767]  <TASK>
> [   18.466975]  dump_stack_lvl+0x32/0x40
> [   18.467325]  __might_resched+0x129/0x180
> [   18.467691]  mutex_lock+0x1a/0x40
> [   18.468057]  nf_defrag_ipv4_enable+0x16/0x70
> [   18.468467]  bpf_nf_link_attach+0x141/0x300
> [   18.468856]  __sys_bpf+0x133e/0x26d0
> 
> You cannot call mutex under rcu_read_lock.

Whoops, my bad. I think this patch should fix it:

```
From 7e8927c44452db07ddd7cf0e30bb49215fc044ed Mon Sep 17 00:00:00 2001
Message-ID: <7e8927c44452db07ddd7cf0e30bb49215fc044ed.1689211250.git.dxu@dxuuu.xyz>
From: Daniel Xu <dxu@dxuuu.xyz>
Date: Wed, 12 Jul 2023 19:17:35 -0600
Subject: [PATCH] netfilter: bpf: Don't hold rcu_read_lock during
 enable/disable

->enable()/->disable() takes a mutex which can sleep. You can't sleep
during RCU read side critical section.

Our refcnt on the module will protect us from ->enable()/->disable()
from going away while we call it.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 net/netfilter/nf_bpf_link.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
index 77ffbf26ba3d..79704cc596aa 100644
--- a/net/netfilter/nf_bpf_link.c
+++ b/net/netfilter/nf_bpf_link.c
@@ -60,9 +60,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
                        goto out_v4;
                }

+               rcu_read_unlock();
                err = v4_hook->enable(link->net);
                if (err)
                        module_put(v4_hook->owner);
+
+               return err;
 out_v4:
                rcu_read_unlock();
                return err;
@@ -92,9 +95,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
                        goto out_v6;
                }

+               rcu_read_unlock();
                err = v6_hook->enable(link->net);
                if (err)
                        module_put(v6_hook->owner);
+
+               return err;
 out_v6:
                rcu_read_unlock();
                return err;
@@ -114,11 +120,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
        case NFPROTO_IPV4:
                rcu_read_lock();
                v4_hook = rcu_dereference(nf_defrag_v4_hook);
+               rcu_read_unlock();
                if (v4_hook) {
                        v4_hook->disable(link->net);
                        module_put(v4_hook->owner);
                }
-               rcu_read_unlock();

                break;
 #endif
@@ -126,11 +132,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
        case NFPROTO_IPV6:
                rcu_read_lock();
                v6_hook = rcu_dereference(nf_defrag_v6_hook);
+               rcu_read_unlock();
                if (v6_hook) {
                        v6_hook->disable(link->net);
                        module_put(v6_hook->owner);
                }
-               rcu_read_unlock();

                break;
        }
--
2.41.0
```

I'll send out a v5 tomorrow morning unless you feel like applying the
series + this patch today.

> 
> Please make sure you have all kernel debug flags on in your testing.
> 

Ack. Will make sure lockdep is on.


Thanks,
Daniel

Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link

[Alexei Starovoitov]

On Wed, Jul 12, 2023 at 6:22 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Hi Alexei,
>
> On Wed, Jul 12, 2023 at 05:43:49PM -0700, Alexei Starovoitov wrote:
> > On Wed, Jul 12, 2023 at 4:44 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > +#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
> > > +       case NFPROTO_IPV6:
> > > +               rcu_read_lock();
> > > +               v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > +               if (!v6_hook) {
> > > +                       rcu_read_unlock();
> > > +                       err = request_module("nf_defrag_ipv6");
> > > +                       if (err)
> > > +                               return err < 0 ? err : -EINVAL;
> > > +
> > > +                       rcu_read_lock();
> > > +                       v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > +                       if (!v6_hook) {
> > > +                               WARN_ONCE(1, "nf_defrag_ipv6_hooks bad registration");
> > > +                               err = -ENOENT;
> > > +                               goto out_v6;
> > > +                       }
> > > +               }
> > > +
> > > +               err = v6_hook->enable(link->net);
> >
> > I was about to apply, but luckily caught this issue in my local test:
> >
> > [   18.462448] BUG: sleeping function called from invalid context at
> > kernel/locking/mutex.c:283
> > [   18.463238] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid:
> > 2042, name: test_progs
> > [   18.463927] preempt_count: 0, expected: 0
> > [   18.464249] RCU nest depth: 1, expected: 0
> > [   18.464631] CPU: 15 PID: 2042 Comm: test_progs Tainted: G
> > O       6.4.0-04319-g6f6ec4fa00dc #4896
> > [   18.465480] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> > [   18.466531] Call Trace:
> > [   18.466767]  <TASK>
> > [   18.466975]  dump_stack_lvl+0x32/0x40
> > [   18.467325]  __might_resched+0x129/0x180
> > [   18.467691]  mutex_lock+0x1a/0x40
> > [   18.468057]  nf_defrag_ipv4_enable+0x16/0x70
> > [   18.468467]  bpf_nf_link_attach+0x141/0x300
> > [   18.468856]  __sys_bpf+0x133e/0x26d0
> >
> > You cannot call mutex under rcu_read_lock.
>
> Whoops, my bad. I think this patch should fix it:
>
> ```
> From 7e8927c44452db07ddd7cf0e30bb49215fc044ed Mon Sep 17 00:00:00 2001
> Message-ID: <7e8927c44452db07ddd7cf0e30bb49215fc044ed.1689211250.git.dxu@dxuuu.xyz>
> From: Daniel Xu <dxu@dxuuu.xyz>
> Date: Wed, 12 Jul 2023 19:17:35 -0600
> Subject: [PATCH] netfilter: bpf: Don't hold rcu_read_lock during
>  enable/disable
>
> ->enable()/->disable() takes a mutex which can sleep. You can't sleep
> during RCU read side critical section.
>
> Our refcnt on the module will protect us from ->enable()/->disable()
> from going away while we call it.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  net/netfilter/nf_bpf_link.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
> index 77ffbf26ba3d..79704cc596aa 100644
> --- a/net/netfilter/nf_bpf_link.c
> +++ b/net/netfilter/nf_bpf_link.c
> @@ -60,9 +60,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
>                         goto out_v4;
>                 }
>
> +               rcu_read_unlock();
>                 err = v4_hook->enable(link->net);
>                 if (err)
>                         module_put(v4_hook->owner);
> +
> +               return err;
>  out_v4:
>                 rcu_read_unlock();
>                 return err;
> @@ -92,9 +95,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
>                         goto out_v6;
>                 }
>
> +               rcu_read_unlock();
>                 err = v6_hook->enable(link->net);
>                 if (err)
>                         module_put(v6_hook->owner);
> +
> +               return err;
>  out_v6:
>                 rcu_read_unlock();
>                 return err;
> @@ -114,11 +120,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
>         case NFPROTO_IPV4:
>                 rcu_read_lock();
>                 v4_hook = rcu_dereference(nf_defrag_v4_hook);
> +               rcu_read_unlock();
>                 if (v4_hook) {
>                         v4_hook->disable(link->net);
>                         module_put(v4_hook->owner);
>                 }
> -               rcu_read_unlock();
>
>                 break;
>  #endif
> @@ -126,11 +132,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
>         case NFPROTO_IPV6:
>                 rcu_read_lock();
>                 v6_hook = rcu_dereference(nf_defrag_v6_hook);
> +               rcu_read_unlock();

No. v6_hook is gone as soon as you unlock it.

Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link

[Daniel Xu]

On Wed, Jul 12, 2023 at 06:26:13PM -0700, Alexei Starovoitov wrote:
> On Wed, Jul 12, 2023 at 6:22 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > Hi Alexei,
> >
> > On Wed, Jul 12, 2023 at 05:43:49PM -0700, Alexei Starovoitov wrote:
> > > On Wed, Jul 12, 2023 at 4:44 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > +#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
> > > > +       case NFPROTO_IPV6:
> > > > +               rcu_read_lock();
> > > > +               v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > > +               if (!v6_hook) {
> > > > +                       rcu_read_unlock();
> > > > +                       err = request_module("nf_defrag_ipv6");
> > > > +                       if (err)
> > > > +                               return err < 0 ? err : -EINVAL;
> > > > +
> > > > +                       rcu_read_lock();
> > > > +                       v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > > +                       if (!v6_hook) {
> > > > +                               WARN_ONCE(1, "nf_defrag_ipv6_hooks bad registration");
> > > > +                               err = -ENOENT;
> > > > +                               goto out_v6;
> > > > +                       }
> > > > +               }
> > > > +
> > > > +               err = v6_hook->enable(link->net);
> > >
> > > I was about to apply, but luckily caught this issue in my local test:
> > >
> > > [   18.462448] BUG: sleeping function called from invalid context at
> > > kernel/locking/mutex.c:283
> > > [   18.463238] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid:
> > > 2042, name: test_progs
> > > [   18.463927] preempt_count: 0, expected: 0
> > > [   18.464249] RCU nest depth: 1, expected: 0
> > > [   18.464631] CPU: 15 PID: 2042 Comm: test_progs Tainted: G
> > > O       6.4.0-04319-g6f6ec4fa00dc #4896
> > > [   18.465480] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > > BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> > > [   18.466531] Call Trace:
> > > [   18.466767]  <TASK>
> > > [   18.466975]  dump_stack_lvl+0x32/0x40
> > > [   18.467325]  __might_resched+0x129/0x180
> > > [   18.467691]  mutex_lock+0x1a/0x40
> > > [   18.468057]  nf_defrag_ipv4_enable+0x16/0x70
> > > [   18.468467]  bpf_nf_link_attach+0x141/0x300
> > > [   18.468856]  __sys_bpf+0x133e/0x26d0
> > >
> > > You cannot call mutex under rcu_read_lock.
> >
> > Whoops, my bad. I think this patch should fix it:
> >
> > ```
> > From 7e8927c44452db07ddd7cf0e30bb49215fc044ed Mon Sep 17 00:00:00 2001
> > Message-ID: <7e8927c44452db07ddd7cf0e30bb49215fc044ed.1689211250.git.dxu@dxuuu.xyz>
> > From: Daniel Xu <dxu@dxuuu.xyz>
> > Date: Wed, 12 Jul 2023 19:17:35 -0600
> > Subject: [PATCH] netfilter: bpf: Don't hold rcu_read_lock during
> >  enable/disable
> >
> > ->enable()/->disable() takes a mutex which can sleep. You can't sleep
> > during RCU read side critical section.
> >
> > Our refcnt on the module will protect us from ->enable()/->disable()
> > from going away while we call it.
> >
> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > ---
> >  net/netfilter/nf_bpf_link.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
> > index 77ffbf26ba3d..79704cc596aa 100644
> > --- a/net/netfilter/nf_bpf_link.c
> > +++ b/net/netfilter/nf_bpf_link.c
> > @@ -60,9 +60,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
> >                         goto out_v4;
> >                 }
> >
> > +               rcu_read_unlock();
> >                 err = v4_hook->enable(link->net);
> >                 if (err)
> >                         module_put(v4_hook->owner);
> > +
> > +               return err;
> >  out_v4:
> >                 rcu_read_unlock();
> >                 return err;
> > @@ -92,9 +95,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
> >                         goto out_v6;
> >                 }
> >
> > +               rcu_read_unlock();
> >                 err = v6_hook->enable(link->net);
> >                 if (err)
> >                         module_put(v6_hook->owner);
> > +
> > +               return err;
> >  out_v6:
> >                 rcu_read_unlock();
> >                 return err;
> > @@ -114,11 +120,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
> >         case NFPROTO_IPV4:
> >                 rcu_read_lock();
> >                 v4_hook = rcu_dereference(nf_defrag_v4_hook);
> > +               rcu_read_unlock();
> >                 if (v4_hook) {
> >                         v4_hook->disable(link->net);
> >                         module_put(v4_hook->owner);
> >                 }
> > -               rcu_read_unlock();
> >
> >                 break;
> >  #endif
> > @@ -126,11 +132,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
> >         case NFPROTO_IPV6:
> >                 rcu_read_lock();
> >                 v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > +               rcu_read_unlock();
> 
> No. v6_hook is gone as soon as you unlock it.

I think we're protected here by the try_module_get() on the enable path.
And we only disable defrag if enabling succeeds. The module shouldn't
be able to deregister its hooks until we call the module_put() later.

I think READ_ONCE() would've been more appropriate but I wasn't sure if
that was ok given nf_defrag_v(4|6)_hook is written to by
rcu_assign_pointer() and I was assuming symmetry is necessary.

Does that sound right?

Thanks,
Daniel

Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link

[Alexei Starovoitov]

On Wed, Jul 12, 2023 at 9:33 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> On Wed, Jul 12, 2023 at 06:26:13PM -0700, Alexei Starovoitov wrote:
> > On Wed, Jul 12, 2023 at 6:22 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > >
> > > Hi Alexei,
> > >
> > > On Wed, Jul 12, 2023 at 05:43:49PM -0700, Alexei Starovoitov wrote:
> > > > On Wed, Jul 12, 2023 at 4:44 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > +#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
> > > > > +       case NFPROTO_IPV6:
> > > > > +               rcu_read_lock();
> > > > > +               v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > > > +               if (!v6_hook) {
> > > > > +                       rcu_read_unlock();
> > > > > +                       err = request_module("nf_defrag_ipv6");
> > > > > +                       if (err)
> > > > > +                               return err < 0 ? err : -EINVAL;
> > > > > +
> > > > > +                       rcu_read_lock();
> > > > > +                       v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > > > +                       if (!v6_hook) {
> > > > > +                               WARN_ONCE(1, "nf_defrag_ipv6_hooks bad registration");
> > > > > +                               err = -ENOENT;
> > > > > +                               goto out_v6;
> > > > > +                       }
> > > > > +               }
> > > > > +
> > > > > +               err = v6_hook->enable(link->net);
> > > >
> > > > I was about to apply, but luckily caught this issue in my local test:
> > > >
> > > > [   18.462448] BUG: sleeping function called from invalid context at
> > > > kernel/locking/mutex.c:283
> > > > [   18.463238] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid:
> > > > 2042, name: test_progs
> > > > [   18.463927] preempt_count: 0, expected: 0
> > > > [   18.464249] RCU nest depth: 1, expected: 0
> > > > [   18.464631] CPU: 15 PID: 2042 Comm: test_progs Tainted: G
> > > > O       6.4.0-04319-g6f6ec4fa00dc #4896
> > > > [   18.465480] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > > > BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> > > > [   18.466531] Call Trace:
> > > > [   18.466767]  <TASK>
> > > > [   18.466975]  dump_stack_lvl+0x32/0x40
> > > > [   18.467325]  __might_resched+0x129/0x180
> > > > [   18.467691]  mutex_lock+0x1a/0x40
> > > > [   18.468057]  nf_defrag_ipv4_enable+0x16/0x70
> > > > [   18.468467]  bpf_nf_link_attach+0x141/0x300
> > > > [   18.468856]  __sys_bpf+0x133e/0x26d0
> > > >
> > > > You cannot call mutex under rcu_read_lock.
> > >
> > > Whoops, my bad. I think this patch should fix it:
> > >
> > > ```
> > > From 7e8927c44452db07ddd7cf0e30bb49215fc044ed Mon Sep 17 00:00:00 2001
> > > Message-ID: <7e8927c44452db07ddd7cf0e30bb49215fc044ed.1689211250.git.dxu@dxuuu.xyz>
> > > From: Daniel Xu <dxu@dxuuu.xyz>
> > > Date: Wed, 12 Jul 2023 19:17:35 -0600
> > > Subject: [PATCH] netfilter: bpf: Don't hold rcu_read_lock during
> > >  enable/disable
> > >
> > > ->enable()/->disable() takes a mutex which can sleep. You can't sleep
> > > during RCU read side critical section.
> > >
> > > Our refcnt on the module will protect us from ->enable()/->disable()
> > > from going away while we call it.
> > >
> > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > ---
> > >  net/netfilter/nf_bpf_link.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
> > > index 77ffbf26ba3d..79704cc596aa 100644
> > > --- a/net/netfilter/nf_bpf_link.c
> > > +++ b/net/netfilter/nf_bpf_link.c
> > > @@ -60,9 +60,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
> > >                         goto out_v4;
> > >                 }
> > >
> > > +               rcu_read_unlock();
> > >                 err = v4_hook->enable(link->net);
> > >                 if (err)
> > >                         module_put(v4_hook->owner);
> > > +
> > > +               return err;
> > >  out_v4:
> > >                 rcu_read_unlock();
> > >                 return err;
> > > @@ -92,9 +95,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
> > >                         goto out_v6;
> > >                 }
> > >
> > > +               rcu_read_unlock();
> > >                 err = v6_hook->enable(link->net);
> > >                 if (err)
> > >                         module_put(v6_hook->owner);
> > > +
> > > +               return err;
> > >  out_v6:
> > >                 rcu_read_unlock();
> > >                 return err;
> > > @@ -114,11 +120,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
> > >         case NFPROTO_IPV4:
> > >                 rcu_read_lock();
> > >                 v4_hook = rcu_dereference(nf_defrag_v4_hook);
> > > +               rcu_read_unlock();
> > >                 if (v4_hook) {
> > >                         v4_hook->disable(link->net);
> > >                         module_put(v4_hook->owner);
> > >                 }
> > > -               rcu_read_unlock();
> > >
> > >                 break;
> > >  #endif
> > > @@ -126,11 +132,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
> > >         case NFPROTO_IPV6:
> > >                 rcu_read_lock();
> > >                 v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > +               rcu_read_unlock();
> >
> > No. v6_hook is gone as soon as you unlock it.
>
> I think we're protected here by the try_module_get() on the enable path.
> And we only disable defrag if enabling succeeds. The module shouldn't
> be able to deregister its hooks until we call the module_put() later.
>
> I think READ_ONCE() would've been more appropriate but I wasn't sure if
> that was ok given nf_defrag_v(4|6)_hook is written to by
> rcu_assign_pointer() and I was assuming symmetry is necessary.

Why is rcu_assign_pointer() used?
If it's not RCU protected, what is the point of rcu_*() accessors
and rcu_read_lock() ?

In general, the pattern:
rcu_read_lock();
ptr = rcu_dereference(...);
rcu_read_unlock();
ptr->..
is a bug. 100%.

Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link

[Daniel Xu]

On Thu, Jul 13, 2023 at 04:10:03PM -0700, Alexei Starovoitov wrote:
> On Wed, Jul 12, 2023 at 9:33 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > On Wed, Jul 12, 2023 at 06:26:13PM -0700, Alexei Starovoitov wrote:
> > > On Wed, Jul 12, 2023 at 6:22 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > >
> > > > Hi Alexei,
> > > >
> > > > On Wed, Jul 12, 2023 at 05:43:49PM -0700, Alexei Starovoitov wrote:
> > > > > On Wed, Jul 12, 2023 at 4:44 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> > > > > > +#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
> > > > > > +       case NFPROTO_IPV6:
> > > > > > +               rcu_read_lock();
> > > > > > +               v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > > > > +               if (!v6_hook) {
> > > > > > +                       rcu_read_unlock();
> > > > > > +                       err = request_module("nf_defrag_ipv6");
> > > > > > +                       if (err)
> > > > > > +                               return err < 0 ? err : -EINVAL;
> > > > > > +
> > > > > > +                       rcu_read_lock();
> > > > > > +                       v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > > > > +                       if (!v6_hook) {
> > > > > > +                               WARN_ONCE(1, "nf_defrag_ipv6_hooks bad registration");
> > > > > > +                               err = -ENOENT;
> > > > > > +                               goto out_v6;
> > > > > > +                       }
> > > > > > +               }
> > > > > > +
> > > > > > +               err = v6_hook->enable(link->net);
> > > > >
> > > > > I was about to apply, but luckily caught this issue in my local test:
> > > > >
> > > > > [   18.462448] BUG: sleeping function called from invalid context at
> > > > > kernel/locking/mutex.c:283
> > > > > [   18.463238] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid:
> > > > > 2042, name: test_progs
> > > > > [   18.463927] preempt_count: 0, expected: 0
> > > > > [   18.464249] RCU nest depth: 1, expected: 0
> > > > > [   18.464631] CPU: 15 PID: 2042 Comm: test_progs Tainted: G
> > > > > O       6.4.0-04319-g6f6ec4fa00dc #4896
> > > > > [   18.465480] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > > > > BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> > > > > [   18.466531] Call Trace:
> > > > > [   18.466767]  <TASK>
> > > > > [   18.466975]  dump_stack_lvl+0x32/0x40
> > > > > [   18.467325]  __might_resched+0x129/0x180
> > > > > [   18.467691]  mutex_lock+0x1a/0x40
> > > > > [   18.468057]  nf_defrag_ipv4_enable+0x16/0x70
> > > > > [   18.468467]  bpf_nf_link_attach+0x141/0x300
> > > > > [   18.468856]  __sys_bpf+0x133e/0x26d0
> > > > >
> > > > > You cannot call mutex under rcu_read_lock.
> > > >
> > > > Whoops, my bad. I think this patch should fix it:
> > > >
> > > > ```
> > > > From 7e8927c44452db07ddd7cf0e30bb49215fc044ed Mon Sep 17 00:00:00 2001
> > > > Message-ID: <7e8927c44452db07ddd7cf0e30bb49215fc044ed.1689211250.git.dxu@dxuuu.xyz>
> > > > From: Daniel Xu <dxu@dxuuu.xyz>
> > > > Date: Wed, 12 Jul 2023 19:17:35 -0600
> > > > Subject: [PATCH] netfilter: bpf: Don't hold rcu_read_lock during
> > > >  enable/disable
> > > >
> > > > ->enable()/->disable() takes a mutex which can sleep. You can't sleep
> > > > during RCU read side critical section.
> > > >
> > > > Our refcnt on the module will protect us from ->enable()/->disable()
> > > > from going away while we call it.
> > > >
> > > > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > > > ---
> > > >  net/netfilter/nf_bpf_link.c | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
> > > > index 77ffbf26ba3d..79704cc596aa 100644
> > > > --- a/net/netfilter/nf_bpf_link.c
> > > > +++ b/net/netfilter/nf_bpf_link.c
> > > > @@ -60,9 +60,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
> > > >                         goto out_v4;
> > > >                 }
> > > >
> > > > +               rcu_read_unlock();
> > > >                 err = v4_hook->enable(link->net);
> > > >                 if (err)
> > > >                         module_put(v4_hook->owner);
> > > > +
> > > > +               return err;
> > > >  out_v4:
> > > >                 rcu_read_unlock();
> > > >                 return err;
> > > > @@ -92,9 +95,12 @@ static int bpf_nf_enable_defrag(struct bpf_nf_link *link)
> > > >                         goto out_v6;
> > > >                 }
> > > >
> > > > +               rcu_read_unlock();
> > > >                 err = v6_hook->enable(link->net);
> > > >                 if (err)
> > > >                         module_put(v6_hook->owner);
> > > > +
> > > > +               return err;
> > > >  out_v6:
> > > >                 rcu_read_unlock();
> > > >                 return err;
> > > > @@ -114,11 +120,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
> > > >         case NFPROTO_IPV4:
> > > >                 rcu_read_lock();
> > > >                 v4_hook = rcu_dereference(nf_defrag_v4_hook);
> > > > +               rcu_read_unlock();
> > > >                 if (v4_hook) {
> > > >                         v4_hook->disable(link->net);
> > > >                         module_put(v4_hook->owner);
> > > >                 }
> > > > -               rcu_read_unlock();
> > > >
> > > >                 break;
> > > >  #endif
> > > > @@ -126,11 +132,11 @@ static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
> > > >         case NFPROTO_IPV6:
> > > >                 rcu_read_lock();
> > > >                 v6_hook = rcu_dereference(nf_defrag_v6_hook);
> > > > +               rcu_read_unlock();
> > >
> > > No. v6_hook is gone as soon as you unlock it.
> >
> > I think we're protected here by the try_module_get() on the enable path.
> > And we only disable defrag if enabling succeeds. The module shouldn't
> > be able to deregister its hooks until we call the module_put() later.
> >
> > I think READ_ONCE() would've been more appropriate but I wasn't sure if
> > that was ok given nf_defrag_v(4|6)_hook is written to by
> > rcu_assign_pointer() and I was assuming symmetry is necessary.
> 
> Why is rcu_assign_pointer() used?
> If it's not RCU protected, what is the point of rcu_*() accessors
> and rcu_read_lock() ?
> 
> In general, the pattern:
> rcu_read_lock();
> ptr = rcu_dereference(...);
> rcu_read_unlock();
> ptr->..
> is a bug. 100%.
> 

The reason I left it like this is b/c otherwise I think there is a race
with module unload and taking a refcnt. For example:

ptr = READ_ONCE(global_var)
                                             <module unload on other cpu>
// ptr invalid
try_module_get(ptr->owner) 

I think the the synchronize_rcu() call in
kernel/module/main.c:free_module() protects against that race based on
my reading.

Maybe the ->enable() path can store a copy of the hook ptr in
struct bpf_nf_link to get rid of the odd rcu_dereference()?

Open to other ideas too -- would appreciate any hints.

Thanks,
Daniel

Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link

[Florian Westphal]

Daniel Xu <dxu@dxuuu.xyz> wrote:
> On Thu, Jul 13, 2023 at 04:10:03PM -0700, Alexei Starovoitov wrote:
> > Why is rcu_assign_pointer() used?
> > If it's not RCU protected, what is the point of rcu_*() accessors
> > and rcu_read_lock() ?
> > 
> > In general, the pattern:
> > rcu_read_lock();
> > ptr = rcu_dereference(...);
> > rcu_read_unlock();
> > ptr->..
> > is a bug. 100%.

FWIW, I agree with Alexei, it does look... dodgy.

> The reason I left it like this is b/c otherwise I think there is a race
> with module unload and taking a refcnt. For example:
> 
> ptr = READ_ONCE(global_var)
>                                              <module unload on other cpu>
> // ptr invalid
> try_module_get(ptr->owner) 
>

Yes, I agree.

> I think the the synchronize_rcu() call in
> kernel/module/main.c:free_module() protects against that race based on
> my reading.
> 
> Maybe the ->enable() path can store a copy of the hook ptr in
> struct bpf_nf_link to get rid of the odd rcu_dereference()?
> 
> Open to other ideas too -- would appreciate any hints.

I would suggest the following:

- Switch ordering of patches 2 and 3.
  What is currently patch 3 would add the .owner fields only.

Then, what is currently patch #2 would document the rcu/modref
interaction like this (omitting error checking for brevity):

rcu_read_lock();
v6_hook = rcu_dereference(nf_defrag_v6_hook);
if (!v6_hook) {
        rcu_read_unlock();
        err = request_module("nf_defrag_ipv6");
        if (err)
                 return err < 0 ? err : -EINVAL;
        rcu_read_lock();
	v6_hook = rcu_dereference(nf_defrag_v6_hook);
}

if (v6_hook && try_module_get(v6_hook->owner))
	v6_hook = rcu_pointer_handoff(v6_hook);
else
	v6_hook = NULL;

rcu_read_unlock();

if (!v6_hook)
	err();
v6_hook->enable();


I'd store the v4/6_hook pointer in the nf bpf link struct, its probably more
self-explanatory for the disable side in that we did pick up a module reference
that we still own at delete time, without need for any rcu involvement.

Because above handoff is repetitive for ipv4 and ipv6,
I suggest to add an agnostic helper for this.

I know you added distinct structures for ipv4 and ipv6 but if they would use
 the same one you could add

static const struct nf_defrag_hook *get_proto_frag_hook(const struct nf_defrag_hook __rcu *hook,
							const char *modulename);

And then use it like:

v4_hook = get_proto_frag_hook(nf_defrag_v4_hook, "nf_defrag_ipv4");

Without a need to copy the modprobe and handoff part.

What do you think?

Re: [PATCH bpf-next v4 2/6] netfilter: bpf: Support BPF_F_NETFILTER_IP_DEFRAG in netfilter link

[Daniel Xu]

Hi Florian,

On Fri, Jul 14, 2023 at 11:47:41AM +0200, Florian Westphal wrote:
> Daniel Xu <dxu@dxuuu.xyz> wrote:
> > On Thu, Jul 13, 2023 at 04:10:03PM -0700, Alexei Starovoitov wrote:
> > > Why is rcu_assign_pointer() used?
> > > If it's not RCU protected, what is the point of rcu_*() accessors
> > > and rcu_read_lock() ?
> > > 
> > > In general, the pattern:
> > > rcu_read_lock();
> > > ptr = rcu_dereference(...);
> > > rcu_read_unlock();
> > > ptr->..
> > > is a bug. 100%.
> 
> FWIW, I agree with Alexei, it does look... dodgy.
> 
> > The reason I left it like this is b/c otherwise I think there is a race
> > with module unload and taking a refcnt. For example:
> > 
> > ptr = READ_ONCE(global_var)
> >                                              <module unload on other cpu>
> > // ptr invalid
> > try_module_get(ptr->owner) 
> >
> 
> Yes, I agree.
> 
> > I think the the synchronize_rcu() call in
> > kernel/module/main.c:free_module() protects against that race based on
> > my reading.
> > 
> > Maybe the ->enable() path can store a copy of the hook ptr in
> > struct bpf_nf_link to get rid of the odd rcu_dereference()?
> > 
> > Open to other ideas too -- would appreciate any hints.
> 
> I would suggest the following:
> 
> - Switch ordering of patches 2 and 3.
>   What is currently patch 3 would add the .owner fields only.
> 
> Then, what is currently patch #2 would document the rcu/modref
> interaction like this (omitting error checking for brevity):
> 
> rcu_read_lock();
> v6_hook = rcu_dereference(nf_defrag_v6_hook);
> if (!v6_hook) {
>         rcu_read_unlock();
>         err = request_module("nf_defrag_ipv6");
>         if (err)
>                  return err < 0 ? err : -EINVAL;
>         rcu_read_lock();
> 	v6_hook = rcu_dereference(nf_defrag_v6_hook);
> }
> 
> if (v6_hook && try_module_get(v6_hook->owner))
> 	v6_hook = rcu_pointer_handoff(v6_hook);
> else
> 	v6_hook = NULL;
> 
> rcu_read_unlock();
> 
> if (!v6_hook)
> 	err();
> v6_hook->enable();
> 
> 
> I'd store the v4/6_hook pointer in the nf bpf link struct, its probably more
> self-explanatory for the disable side in that we did pick up a module reference
> that we still own at delete time, without need for any rcu involvement.
> 
> Because above handoff is repetitive for ipv4 and ipv6,
> I suggest to add an agnostic helper for this.
> 
> I know you added distinct structures for ipv4 and ipv6 but if they would use
>  the same one you could add
> 
> static const struct nf_defrag_hook *get_proto_frag_hook(const struct nf_defrag_hook __rcu *hook,
> 							const char *modulename);
> 
> And then use it like:
> 
> v4_hook = get_proto_frag_hook(nf_defrag_v4_hook, "nf_defrag_ipv4");
> 
> Without a need to copy the modprobe and handoff part.
> 
> What do you think?

That sounds reasonable to me. I'll give it a shot. Thanks for the input!

Daniel