On Thu, Dec 21, 2023 at 11:06 PM D. Wythe <alibuda@xxxxxxxxxxxxxxxxx> wrote:
Why do you think so?
On 12/21/23 5:11 AM, Alexei Starovoitov wrote:
On Wed, Dec 20, 2023 at 6:09 AM D. Wythe <alibuda@xxxxxxxxxxxxxxxxx> wrote:Introducing rcu_head/kfree_rcu is to address the situation where the
From: "D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx>I have to point out the same issues as before, but
To support the prog update, we need to ensure that the prog seen
within the hook is always valid. Considering that hooks are always
protected by rcu_read_lock(), which provide us the ability to
access the prog under rcu.
Signed-off-by: D. Wythe <alibuda@xxxxxxxxxxxxxxxxx>
---
net/netfilter/nf_bpf_link.c | 63 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
index e502ec0..9bc91d1 100644
--- a/net/netfilter/nf_bpf_link.c
+++ b/net/netfilter/nf_bpf_link.c
@@ -8,17 +8,8 @@
#include <net/netfilter/nf_bpf_link.h>
#include <uapi/linux/netfilter_ipv4.h>
-static unsigned int nf_hook_run_bpf(void *bpf_prog, struct sk_buff *skb,
- const struct nf_hook_state *s)
-{
- const struct bpf_prog *prog = bpf_prog;
- struct bpf_nf_ctx ctx = {
- .state = s,
- .skb = skb,
- };
-
- return bpf_prog_run(prog, &ctx);
-}
+/* protect link update in parallel */
+static DEFINE_MUTEX(bpf_nf_mutex);
struct bpf_nf_link {
struct bpf_link link;
@@ -26,8 +17,20 @@ struct bpf_nf_link {
struct net *net;
u32 dead;
const struct nf_defrag_hook *defrag_hook;
+ struct rcu_head head;
will ask them differently...
Why do you think above rcu_head is necessary?
};Why is this needed ?
+static unsigned int nf_hook_run_bpf(void *bpf_link, struct sk_buff *skb,
+ const struct nf_hook_state *s)
+{
+ const struct bpf_nf_link *nf_link = bpf_link;
+ struct bpf_nf_ctx ctx = {
+ .state = s,
+ .skb = skb,
+ };
+ return bpf_prog_run(rcu_dereference_raw(nf_link->link.prog), &ctx);
+}
+
#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4) || IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
static const struct nf_defrag_hook *
get_proto_defrag_hook(struct bpf_nf_link *link,
@@ -126,8 +129,7 @@ static void bpf_nf_link_release(struct bpf_link *link)
static void bpf_nf_link_dealloc(struct bpf_link *link)
{
struct bpf_nf_link *nf_link = container_of(link, struct bpf_nf_link, link);
-
- kfree(nf_link);
+ kfree_rcu(nf_link, head);
Have you looked at tcx_link_lops ?
netfilter hooks might
still access the link after bpf_nf_link_dealloc.
nf_hook_run_bpfWhere do you see such code in tcx_link_lops ?
const struct
bpf_nf_link *nf_link = bpf_link;
bpf_nf_link_release
nf_unregister_net_hook(nf_link->net, &nf_link->hook_ops);
bpf_nf_link_dealloc
free(link)
bpf_prog_run(link->prog);
I had checked the tcx_link_lops ,it's seems it use the synchronize_rcu()
to solve the
same problem, which is also the way we used in the first version.You're correct that sys_bpf() doesn't lock anything.
https://lore.kernel.org/bpf/1702467945-38866-1-git-send-email-alibuda@xxxxxxxxxxxxxxxxx/
However, we have received some opposing views, believing that this is a
bit overkill,
so we decided to use kfree_rcu.
https://lore.kernel.org/bpf/20231213222415.GA13818@xxxxxxxxxxxxx/
To avoid user update a link with differ prog at the same time. I noticed}Why do you need this mutex?
static int bpf_nf_link_detach(struct bpf_link *link)
@@ -162,7 +164,34 @@ static int bpf_nf_link_fill_link_info(const struct bpf_link *link,
static int bpf_nf_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
struct bpf_prog *old_prog)
{
- return -EOPNOTSUPP;
+ struct bpf_nf_link *nf_link = container_of(link, struct bpf_nf_link, link);
+ int err = 0;
+
+ mutex_lock(&bpf_nf_mutex);
What race does it solve?
that sys_bpf()
doesn't seem to prevent being invoked by user at the same time. Have I
missed something?
But what are you serializing in this bpf_nf_link_update() ?
What will happen if multiple bpf_nf_link_update()
without mutex run on different CPUs in parallel ?