diff mbox series

[net-next] net_sched: use idr to allocate bpf filter handles

Message ID 20170921234302.26558-1-xiyou.wangcong@gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series [net-next] net_sched: use idr to allocate bpf filter handles | expand

Commit Message

Cong Wang Sept. 21, 2017, 11:43 p.m. UTC 
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Chris Mi <chrism@mellanox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_bpf.c | 57 ++++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

Comments

David Miller Sept. 24, 2017, 12:15 a.m. UTC | #1 
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 21 Sep 2017 16:43:00 -0700

> @@ -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;
> -}
> -

If I read the code properly, the existing code allows IDs from '1' to
and including '0x7ffffffe', whereas the new code allows up to and
including '0x7ffffffff'.

Whether intentional or not this is a change in behavior.  If it's OK,
it should be at least documented either in a comment or in the
commit log itself.

Similarly for your other scheduler IDR changes.

Thanks.
Cong Wang Sept. 25, 2017, 4:46 p.m. UTC | #2 
On Sat, Sep 23, 2017 at 5:15 PM, David Miller <davem@davemloft.net> wrote:
> If I read the code properly, the existing code allows IDs from '1' to
> and including '0x7ffffffe', whereas the new code allows up to and
> including '0x7ffffffff'.
>
> Whether intentional or not this is a change in behavior.  If it's OK,
> it should be at least documented either in a comment or in the
> commit log itself.
>
> Similarly for your other scheduler IDR changes.

Good catch! Obviously off-by-one. I agree we should keep
the old behavior no matter the reason.

I will update the patches.

Thanks.
Cong Wang Sept. 25, 2017, 4:49 p.m. UTC | #3 
On Mon, Sep 25, 2017 at 9:46 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sat, Sep 23, 2017 at 5:15 PM, David Miller <davem@davemloft.net> wrote:
>> If I read the code properly, the existing code allows IDs from '1' to
>> and including '0x7ffffffe', whereas the new code allows up to and
>> including '0x7ffffffff'.
>>
>> Whether intentional or not this is a change in behavior.  If it's OK,
>> it should be at least documented either in a comment or in the
>> commit log itself.
>>
>> Similarly for your other scheduler IDR changes.
>
> Good catch! Obviously off-by-one. I agree we should keep
> the old behavior no matter the reason.
>
> I will update the patches.
>

By the way, same problem for commit fe2502e49b58606580c77.
:)
diff mbox series

Patch

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 520c5027646a..4fd352a1778e 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -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, 0x80000000, 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);