[PATCH net v3] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.

[PATCH net v3] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

To generate hnode handles (in gen_new_htid()), u32 uses IDR and
encodes the returned small integer into a structured 32-bit
word. Unfortunately, at disposal time, the needed decoding
is not done. As a result, idr_remove() fails, and the IDR
fills up. Since its size is 2048, the following script ends up
with "Filter already exists":

  tc filter add dev myve $FILTER1
  tc filter add dev myve $FILTER2
  for i in {1..2048}
  do
    echo $i
    tc filter del dev myve $FILTER2
    tc filter add dev myve $FILTER2
  done

This patch adds the missing decoding logic for handles that
deserve it.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
---
v3: prepend title with subsystem ident
v2: use u32 type in handle encoder/decoder

 net/sched/cls_u32.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 9412d88a99bc..6da94b809926 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -41,6 +41,16 @@
 #include <linux/idr.h>
 #include <net/tc_wrapper.h>
 
+static inline u32 handle2id(u32 h)
+{
+	return ((h & 0x80000000) ? ((h >> 20) & 0x7FF) : h);
+}
+
+static inline u32 id2handle(u32 id)
+{
+	return (id | 0x800U) << 20;
+}
+
 struct tc_u_knode {
 	struct tc_u_knode __rcu	*next;
 	u32			handle;
@@ -310,7 +320,7 @@ static u32 gen_new_htid(struct tc_u_common *tp_c, struct tc_u_hnode *ptr)
 	int id = idr_alloc_cyclic(&tp_c->handle_idr, ptr, 1, 0x7FF, GFP_KERNEL);
 	if (id < 0)
 		return 0;
-	return (id | 0x800U) << 20;
+	return id2handle(id);
 }
 
 static struct hlist_head *tc_u_common_hash;
@@ -360,7 +370,7 @@ static int u32_init(struct tcf_proto *tp)
 		return -ENOBUFS;
 
 	refcount_set(&root_ht->refcnt, 1);
-	root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000;
+	root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : id2handle(0);
 	root_ht->prio = tp->prio;
 	root_ht->is_root = true;
 	idr_init(&root_ht->handle_idr);
@@ -612,7 +622,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
 		if (phn == ht) {
 			u32_clear_hw_hnode(tp, ht, extack);
 			idr_destroy(&ht->handle_idr);
-			idr_remove(&tp_c->handle_idr, ht->handle);
+			idr_remove(&tp_c->handle_idr, handle2id(ht->handle));
 			RCU_INIT_POINTER(*hn, ht->next);
 			kfree_rcu(ht, rcu);
 			return 0;
@@ -989,7 +999,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		err = u32_replace_hw_hnode(tp, ht, userflags, extack);
 		if (err) {
-			idr_remove(&tp_c->handle_idr, handle);
+			idr_remove(&tp_c->handle_idr, handle2id(handle));
 			kfree(ht);
 			return err;
 		}
-- 
2.30.2

Re: [PATCH net v3] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.

[Jamal Hadi Salim]

Hi,

On Wed, Nov 6, 2024 at 9:32 AM Alexandre Ferrieux
<alexandre.ferrieux@gmail.com> wrote:
>
> To generate hnode handles (in gen_new_htid()), u32 uses IDR and
> encodes the returned small integer into a structured 32-bit
> word. Unfortunately, at disposal time, the needed decoding
> is not done. As a result, idr_remove() fails, and the IDR
> fills up. Since its size is 2048, the following script ends up
> with "Filter already exists":
>
>   tc filter add dev myve $FILTER1
>   tc filter add dev myve $FILTER2
>   for i in {1..2048}
>   do
>     echo $i
>     tc filter del dev myve $FILTER2
>     tc filter add dev myve $FILTER2
>   done
>
> This patch adds the missing decoding logic for handles that
> deserve it.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>

I'd like to take a closer look at this - just tied up with something
at the moment. Give me a day or so.
Did you run tdc tests after your patch?

cheers,
jamal

> ---
> v3: prepend title with subsystem ident
> v2: use u32 type in handle encoder/decoder
>
>  net/sched/cls_u32.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 9412d88a99bc..6da94b809926 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -41,6 +41,16 @@
>  #include <linux/idr.h>
>  #include <net/tc_wrapper.h>
>
> +static inline u32 handle2id(u32 h)
> +{
> +       return ((h & 0x80000000) ? ((h >> 20) & 0x7FF) : h);
> +}
> +
> +static inline u32 id2handle(u32 id)
> +{
> +       return (id | 0x800U) << 20;
> +}
> +
>  struct tc_u_knode {
>         struct tc_u_knode __rcu *next;
>         u32                     handle;
> @@ -310,7 +320,7 @@ static u32 gen_new_htid(struct tc_u_common *tp_c, struct tc_u_hnode *ptr)
>         int id = idr_alloc_cyclic(&tp_c->handle_idr, ptr, 1, 0x7FF, GFP_KERNEL);
>         if (id < 0)
>                 return 0;
> -       return (id | 0x800U) << 20;
> +       return id2handle(id);
>  }
>
>  static struct hlist_head *tc_u_common_hash;
> @@ -360,7 +370,7 @@ static int u32_init(struct tcf_proto *tp)
>                 return -ENOBUFS;
>
>         refcount_set(&root_ht->refcnt, 1);
> -       root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000;
> +       root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : id2handle(0);
>         root_ht->prio = tp->prio;
>         root_ht->is_root = true;
>         idr_init(&root_ht->handle_idr);
> @@ -612,7 +622,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
>                 if (phn == ht) {
>                         u32_clear_hw_hnode(tp, ht, extack);
>                         idr_destroy(&ht->handle_idr);
> -                       idr_remove(&tp_c->handle_idr, ht->handle);
> +                       idr_remove(&tp_c->handle_idr, handle2id(ht->handle));
>                         RCU_INIT_POINTER(*hn, ht->next);
>                         kfree_rcu(ht, rcu);
>                         return 0;
> @@ -989,7 +999,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>
>                 err = u32_replace_hw_hnode(tp, ht, userflags, extack);
>                 if (err) {
> -                       idr_remove(&tp_c->handle_idr, handle);
> +                       idr_remove(&tp_c->handle_idr, handle2id(handle));
>                         kfree(ht);
>                         return err;
>                 }
> --
> 2.30.2
>

Re: [PATCH net v3] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.

[Jamal Hadi Salim]

On Thu, Nov 7, 2024 at 9:45 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> Hi,
>
> On Wed, Nov 6, 2024 at 9:32 AM Alexandre Ferrieux
> <alexandre.ferrieux@gmail.com> wrote:
> >
> > To generate hnode handles (in gen_new_htid()), u32 uses IDR and
> > encodes the returned small integer into a structured 32-bit
> > word. Unfortunately, at disposal time, the needed decoding
> > is not done. As a result, idr_remove() fails, and the IDR
> > fills up. Since its size is 2048, the following script ends up
> > with "Filter already exists":
> >
> >   tc filter add dev myve $FILTER1
> >   tc filter add dev myve $FILTER2
> >   for i in {1..2048}
> >   do
> >     echo $i
> >     tc filter del dev myve $FILTER2
> >     tc filter add dev myve $FILTER2
> >   done
> >
> > This patch adds the missing decoding logic for handles that
> > deserve it.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
>
> I'd like to take a closer look at this - just tied up with something
> at the moment. Give me a day or so.
> Did you run tdc tests after your patch?

Also, for hero status points, consider submitting a tdc test case.

cheers,
jamal

Re: [PATCH net v3] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

On 07/11/2024 15:47, Jamal Hadi Salim wrote:
> On Thu, Nov 7, 2024 at 9:45 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>> Hi,
>>
>> On Wed, Nov 6, 2024 at 9:32 AM Alexandre Ferrieux
>> <alexandre.ferrieux@gmail.com> wrote:
>> >
>> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> > Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
>>
>> I'd like to take a closer look at this - just tied up with something
>> at the moment. Give me a day or so.
>> Did you run tdc tests after your patch?
> 
> Also, for hero status points, consider submitting a tdc test case.

Hi Jamal, thanks for looking into this.
Just posted a v4 with a tdc test case.
Of course, I also verified that "tdc -c u32" has no regression :)

Re: [PATCH net v3] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

On 07/11/2024 23:58, Alexandre Ferrieux wrote:
> On 07/11/2024 15:47, Jamal Hadi Salim wrote:
>> On Thu, Nov 7, 2024 at 9:45 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>
>>> Hi,
>>>
>>> On Wed, Nov 6, 2024 at 9:32 AM Alexandre Ferrieux
>>> <alexandre.ferrieux@gmail.com> wrote:
>>> >
>>> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> > Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
>>>
>>> I'd like to take a closer look at this - just tied up with something
>>> at the moment. Give me a day or so.
>>> Did you run tdc tests after your patch?
>> 
>> Also, for hero status points, consider submitting a tdc test case.
> 
> Hi Jamal, thanks for looking into this.
> Just posted a v4 with a tdc test case.
> Of course, I also verified that "tdc -c u32" has no regression :)

And a v5 with a proper title as requested by Eric, along with his Reviewed-by.
(Sorry for violating the rule "one version per 24h" but there's no code change)

Re: [PATCH net v3] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.

[Jamal Hadi Salim]

On Thu, Nov 7, 2024 at 6:14 PM Alexandre Ferrieux
<alexandre.ferrieux@gmail.com> wrote:
>
> On 07/11/2024 23:58, Alexandre Ferrieux wrote:
> > On 07/11/2024 15:47, Jamal Hadi Salim wrote:
> >> On Thu, Nov 7, 2024 at 9:45 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Wed, Nov 6, 2024 at 9:32 AM Alexandre Ferrieux
> >>> <alexandre.ferrieux@gmail.com> wrote:
> >>> >
> >>> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> >>> > Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
> >>>
> >>> I'd like to take a closer look at this - just tied up with something
> >>> at the moment. Give me a day or so.
> >>> Did you run tdc tests after your patch?
> >>
> >> Also, for hero status points, consider submitting a tdc test case.
> >
> > Hi Jamal, thanks for looking into this.
> > Just posted a v4 with a tdc test case.
> > Of course, I also verified that "tdc -c u32" has no regression :)
>
> And a v5 with a proper title as requested by Eric, along with his Reviewed-by.
> (Sorry for violating the rule "one version per 24h" but there's no code change)
>

BTW, what is your interest in u32? I am always curious about use
cases. I gave a talk here:
https://netdevconf.info/0x13/session.html?talk-tc-u-classifier

cheers,
jamal

Re: [PATCH net v3] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes.

[Alexandre Ferrieux]

On 09/11/2024 14:07, Jamal Hadi Salim wrote:
> 
> BTW, what is your interest in u32? I am always curious about use
> cases. I gave a talk here:
> https://netdevconf.info/0x13/session.html?talk-tc-u-classifier

In first approximation, my motivation for u32 is very akin to your humorous
depiction pitting flower-for-humans against u32-for-machines... more seriously,
genericity is my primary concern, and I'm instantly convinced by the notion of a
scriptable mechanism that is really universal, as in "parse my custom protocol
without writing a kernel module".

Now today there is also nftables in raw payload mode, and with its hashing
features it might be possible to emulate a full u32 graph of hnodes/knodes. Not
sure about the perf though. And of course, in case of hardware offload, u32 wins.

I am also aware of tc-bpf. For trivial things without hash, directly writing the
cBPF assembly by hand is a serious contender. But if a hash is needed you must
go eBPF, with its heavier infrastructure.

Overall, it seems to me u32 still sits at a "sweet spot" of the
flexibility-performance tradeoff. Its actual usability by mortals is another
story, as Tom hinted at in the QA session of your talk :)