Skip to contentSkip to conversation

net_sched: use idr to allocate bpf filter handles

PATCH Applied Cong Wang 3 participants 5 messages Sep 25, 2017 – Sep 28, 2017

Subsystems (5)
lore
Share & tools
lore

Cong Wang

author
#0 lore raw

PATCH v2 1/1 net_sched: use idr to allocate bpf filter handles

Instead of calling cls_bpf_get() in a loop to find a unused handle, just switch to idr API to allocate new handles.

Cc Daniel Borkmann <[email protected]>
Cc Chris Mi <[email protected]>
Cc Jamal Hadi Salim <[email protected]>
---
 net/sched/cls_bpf.c | 57 ++++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 29 deletions(-)
net/sched/cls_bpf.c +28 -29
index 520c5027646a..e19c3c3c27cc 100644
@@ -17,6 +17,7 @@
 #include <linux/skbuff.h>
 #include <linux/filter.h>
 #include <linux/bpf.h>
+#include <linux/idr.h>
 
 #include <net/rtnetlink.h>
 #include <net/pkt_cls.h>
@@ -32,7 +33,7 @@ MODULE_DESCRIPTION("TC BPF based classifier");
 
 struct cls_bpf_head {
 	struct list_head plist;
-	u32 hgen;
+	struct idr handle_idr;
 	struct rcu_head rcu;
 };
 
@@ -238,6 +239,7 @@ static int cls_bpf_init(struct tcf_proto *tp)
 		return -ENOBUFS;
 
 	INIT_LIST_HEAD_RCU(&head->plist);
+	idr_init(&head->handle_idr);
 	rcu_assign_pointer(tp->root, head);
 
 	return 0;
@@ -264,6 +266,9 @@ static void cls_bpf_delete_prog_rcu(struct rcu_head *rcu)
 
 static void __cls_bpf_delete(struct tcf_proto *tp, struct cls_bpf_prog *prog)
 {
+	struct cls_bpf_head *head = rtnl_dereference(tp->root);
+
+	idr_remove_ext(&head->handle_idr, prog->handle);
 	cls_bpf_stop_offload(tp, prog);
 	list_del_rcu(&prog->link);
 	tcf_unbind_filter(tp, &prog->res);
@@ -287,6 +292,7 @@ static void cls_bpf_destroy(struct tcf_proto *tp)
 	list_for_each_entry_safe(prog, tmp, &head->plist, link)
 		__cls_bpf_delete(tp, prog);
 
+	idr_destroy(&head->handle_idr);
 	kfree_rcu(head, rcu);
 }
 
@@ -421,27 +427,6 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
 	return 0;
 }
 
-static u32 cls_bpf_grab_new_handle(struct tcf_proto *tp,
-				   struct cls_bpf_head *head)
-{
-	unsigned int i = 0x80000000;
-	u32 handle;
-
-	do {
-		if (++head->hgen == 0x7FFFFFFF)
-			head->hgen = 1;
-	} while (--i > 0 && cls_bpf_get(tp, head->hgen));
-
-	if (unlikely(i == 0)) {
-		pr_err("Insufficient number of handles\n");
-		handle = 0;
-	} else {
-		handle = head->hgen;
-	}
-
-	return handle;
-}
-
 static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 			  struct tcf_proto *tp, unsigned long base,
 			  u32 handle, struct nlattr **tca,
@@ -451,6 +436,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	struct cls_bpf_prog *oldprog = *arg;
 	struct nlattr *tb[TCA_BPF_MAX + 1];
 	struct cls_bpf_prog *prog;
+	unsigned long idr_index;
 	int ret;
 
 	if (tca[TCA_OPTIONS] == NULL)
@@ -476,21 +462,30 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 		}
 	}
 
-	if (handle == 0)
-		prog->handle = cls_bpf_grab_new_handle(tp, head);
-	else
+	if (handle == 0) {
+		ret = idr_alloc_ext(&head->handle_idr, prog, &idr_index,
+				    1, 0x7FFFFFFF, GFP_KERNEL);
+		if (ret)
+			goto errout;
+		prog->handle = idr_index;
+	} else {
+		if (!oldprog) {
+			ret = idr_alloc_ext(&head->handle_idr, prog, &idr_index,
+					    handle, handle + 1, GFP_KERNEL);
+			if (ret)
+				goto errout;
+		}
 		prog->handle = handle;
-	if (prog->handle == 0) {
-		ret = -EINVAL;
-		goto errout;
 	}
 
 	ret = cls_bpf_set_parms(net, tp, prog, base, tb, tca[TCA_RATE], ovr);
 	if (ret < 0)
-		goto errout;
+		goto errout_idr;
 
 	ret = cls_bpf_offload(tp, prog, oldprog);
 	if (ret) {
+		if (!oldprog)
+			idr_remove_ext(&head->handle_idr, prog->handle);
 		__cls_bpf_delete_prog(prog);
 		return ret;
 	}
@@ -499,6 +494,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 		prog->gen_flags |= TCA_CLS_FLAGS_NOT_IN_HW;
 
 	if (oldprog) {
+		idr_replace_ext(&head->handle_idr, prog, handle);
 		list_replace_rcu(&oldprog->link, &prog->link);
 		tcf_unbind_filter(tp, &oldprog->res);
 		call_rcu(&oldprog->rcu, cls_bpf_delete_prog_rcu);
@@ -509,6 +505,9 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	*arg = prog;
 	return 0;
 
+errout_idr:
+	if (!oldprog)
+		idr_remove_ext(&head->handle_idr, prog->handle);
 errout:
 	tcf_exts_destroy(&prog->exts);
 	kfree(prog);
Signature · 1 line
2.13.0
maintainer
#1 lore raw
Quoted 8 lines · Cong Wang

Instead of calling cls_bpf_get() in a loop to find a unused handle, just switch to idr API to allocate new handles.

Cc Daniel Borkmann <[email protected]>
Cc Chris Mi <[email protected]>
Cc Jamal Hadi Salim <[email protected]>

[...]

Quoted 25 lines
net/sched/cls_bpf.c +13 -6
@@ -476,21 +462,30 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
  		}
  	}
 
-	if (handle == 0)
-		prog->handle = cls_bpf_grab_new_handle(tp, head);
-	else
+	if (handle == 0) {
+		ret = idr_alloc_ext(&head->handle_idr, prog, &idr_index,
+				    1, 0x7FFFFFFF, GFP_KERNEL);
+		if (ret)
+			goto errout;
+		prog->handle = idr_index;
+	} else {
+		if (!oldprog) {
+			ret = idr_alloc_ext(&head->handle_idr, prog, &idr_index,
+					    handle, handle + 1, GFP_KERNEL);
+			if (ret)
+				goto errout;
+		}
  		prog->handle = handle;
-	if (prog->handle == 0) {
-		ret = -EINVAL;
-		goto errout;
  	}

net/sched/cls_bpf.c static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, · line 488

Quoted 10 lines
net/sched/cls_bpf.c +3 -1
 
  	ret = cls_bpf_set_parms(net, tp, prog, base, tb, tca[TCA_RATE], ovr);
  	if (ret < 0)
-		goto errout;
+		goto errout_idr;
 
  	ret = cls_bpf_offload(tp, prog, oldprog);
  	if (ret) {
+		if (!oldprog)
+			idr_remove_ext(&head->handle_idr, prog->handle);

Shouldn't we also call idr_remove_ext() when there was an oldprog, but we didn't care about reusing the same handle, so it was handle == 0 initially?

There's this condition in the code before above idr allocations, I think also in other classifiers:

        if (oldprog) {
                if (handle && oldprog->handle != handle) {
                        ret = -EINVAL;
                        goto errout;
                }
        }

net/sched/cls_bpf.c static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, · line 497

Quoted 8 lines
net/sched/cls_bpf.c +1 -0
  		__cls_bpf_delete_prog(prog);
  		return ret;
  	}
@@ -499,6 +494,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
  		prog->gen_flags |= TCA_CLS_FLAGS_NOT_IN_HW;
 
  	if (oldprog) {
+		idr_replace_ext(&head->handle_idr, prog, handle);

And here, we should probably use prog->handle for the above mentioned case as well, no?

Would be great if all this (and e.g. the fact that we use idr itself) could optionally be hidden behind some handle generator api given we could reuse that api also for cls_basic and cls_u32. Could also be followed-up perhaps.

net/sched/cls_bpf.c static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, · line 510

Quoted 10 lines
net/sched/cls_bpf.c +3 -0
  		list_replace_rcu(&oldprog->link, &prog->link);
  		tcf_unbind_filter(tp, &oldprog->res);
  		call_rcu(&oldprog->rcu, cls_bpf_delete_prog_rcu);
@@ -509,6 +505,9 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
  	*arg = prog;
  	return 0;
 
+errout_idr:
+	if (!oldprog)
+		idr_remove_ext(&head->handle_idr, prog->handle);

(Likewise as the failing cls_bpf_offload().)

Quoted 4 lines
net/sched/cls_bpf.c +0 -0
  errout:
  	tcf_exts_destroy(&prog->exts);
  	kfree(prog);
author
#2 lore raw
Quoted 10 lines · Daniel Borkmann

Review

Quoted 4 lines · Cong Wang
        ret = cls_bpf_offload(tp, prog, oldprog);
        if (ret) {
(quoted diff) +2 -0
+               if (!oldprog)
+                       idr_remove_ext(&head->handle_idr, prog->handle);

Shouldn't we also call idr_remove_ext() when there was an oldprog, but we didn't care about reusing the same handle, so it was handle == 0 initially?

When oldprog is non-NULL, we are replacing the oldprog with a new one, therefore we should call idr_replace_ext() which happens after this. So no need to call idr_remove_ext() at this point.

Quoted 10 lines

There's this condition in the code before above idr allocations, I think also in other classifiers:

        if (oldprog) {
                if (handle && oldprog->handle != handle) {
                        ret = -EINVAL;
                        goto errout;
                }
        }

Sure. If we use handle to find oldprog, it should have the same handle. cls_bpf_get() guarantees it. This check is redundant.

Quoted 8 lines
Quoted 8 lines
                __cls_bpf_delete_prog(prog);
                return ret;
        }
net/sched/cls_bpf.c +0 -0
@@ -499,6 +494,7 @@ static int cls_bpf_change(struct net *net, struct
 sk_buff *in_skb,
                prog->gen_flags |= TCA_CLS_FLAGS_NOT_IN_HW;
 
Quoted 6 lines

net/sched/cls_bpf.c static int cls_bpf_change(struct net *net, struct sk_buff *in_skb, · line 497

Quoted 2 lines
net/sched/cls_bpf.c +1 -0
        if (oldprog) {
+               idr_replace_ext(&head->handle_idr, prog, handle);

And here, we should probably use prog->handle for the above mentioned case as well, no?

Since are replacing oldprog with a new one, prog->handle is same with handle.

Quoted 6 lines

Would be great if all this (and e.g. the fact that we use idr itself) could optionally be hidden behind some handle generator api given we could reuse that api also for cls_basic and cls_u32. Could also be followed-up perhaps.

Yeah, the idr_alloc_ext(.., handle, handle+1,) is ugly. Ideally we should specify the range during initialization rather than in each idr_alloc_ext(). Commit c15ab236d69d already did the same thing. We can refactor this later.

maintainer
#3 lore raw
Quoted 2 lines · Cong Wang
Quoted 1 line · Daniel Borkmann

On 09/25/2017 07:13 PM, Cong Wang wrote:

[...]

Quoted 10 lines
Quoted 9 lines

There's this condition in the code before above idr allocations, I think also in other classifiers:

         if (oldprog) {
                 if (handle && oldprog->handle != handle) {
                         ret = -EINVAL;
                         goto errout;
                 }
         }
Quoted 3 lines

Sure. If we use handle to find oldprog, it should have the same handle. cls_bpf_get() guarantees it. This check is redundant.

Good point, we should just test for 'oldprog && oldprog->handle != handle' and bail out then, otherwise it's just irritating. I can see to fix this up later. Seems fine to me then, thanks!