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

[PATCH net v6] 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, along with a corresponding tdc test.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
---
v6: big speedup of the tdc test with batch tc
v5: fix title - again
v4: add tdc test
v3: prepend title with subsystem ident
v2: use u32 type in handle encoder/decoder

 net/sched/cls_u32.c                           | 18 ++++++++++----
 .../tc-testing/tc-tests/filters/u32.json      | 24 +++++++++++++++++++
 2 files changed, 38 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;
 		}
diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/u32.json b/tools/testing/selftests/tc-testing/tc-tests/filters/u32.json
index 24bd0c2a3014..b2ca9d4e991b 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/u32.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/u32.json
@@ -329,5 +329,29 @@
         "teardown": [
             "$TC qdisc del dev $DEV1 parent root drr"
         ]
+    },
+    {
+        "id": "1234",
+        "name": "Exercise IDR leaks by creating/deleting a filter many (2048) times",
+        "category": [
+            "filter",
+            "u32"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$TC qdisc add dev $DEV1 parent root handle 10: drr",
+            "$TC filter add dev $DEV1 parent 10:0 protocol ip prio 2 u32 match ip src 0.0.0.2/32 action drop",
+            "$TC filter add dev $DEV1 parent 10:0 protocol ip prio 3 u32 match ip src 0.0.0.3/32 action drop"
+        ],
+        "cmdUnderTest": "bash -c 'for i in {1..2048} ;do echo filter delete dev $DEV1 pref 3;echo filter add dev $DEV1 parent 10:0 protocol ip prio 3 u32 match ip src 0.0.0.3/32 action drop;done | $TC -b -'",
+        "expExitCode": "0",
+        "verifyCmd": "$TC filter show dev $DEV1",
+        "matchPattern": "protocol ip pref 3 u32",
+        "matchCount": "3",
+        "teardown": [
+            "$TC qdisc del dev $DEV1 parent root drr"
+        ]
     }
 ]
-- 
2.30.2

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

[Jamal Hadi Salim]

On Fri, Nov 8, 2024 at 9:12 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, along with a corresponding tdc test.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>


Ok, looks good.
Please split the test into a separate patch targeting net-next. Also
your "Fixes" should be:
commit e7614370d6f04711c4e4b48f7055e5008fa4ed42
When you send the next version please include my Acked-by:

cheers,
jamal

> ---
> v6: big speedup of the tdc test with batch tc
> v5: fix title - again
> v4: add tdc test
> v3: prepend title with subsystem ident
> v2: use u32 type in handle encoder/decoder
>
>  net/sched/cls_u32.c                           | 18 ++++++++++----
>  .../tc-testing/tc-tests/filters/u32.json      | 24 +++++++++++++++++++
>  2 files changed, 38 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;
>                 }
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/u32.json b/tools/testing/selftests/tc-testing/tc-tests/filters/u32.json
> index 24bd0c2a3014..b2ca9d4e991b 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/filters/u32.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/filters/u32.json
> @@ -329,5 +329,29 @@
>          "teardown": [
>              "$TC qdisc del dev $DEV1 parent root drr"
>          ]
> +    },
> +    {
> +        "id": "1234",
> +        "name": "Exercise IDR leaks by creating/deleting a filter many (2048) times",
> +        "category": [
> +            "filter",
> +            "u32"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [
> +            "$TC qdisc add dev $DEV1 parent root handle 10: drr",
> +            "$TC filter add dev $DEV1 parent 10:0 protocol ip prio 2 u32 match ip src 0.0.0.2/32 action drop",
> +            "$TC filter add dev $DEV1 parent 10:0 protocol ip prio 3 u32 match ip src 0.0.0.3/32 action drop"
> +        ],
> +        "cmdUnderTest": "bash -c 'for i in {1..2048} ;do echo filter delete dev $DEV1 pref 3;echo filter add dev $DEV1 parent 10:0 protocol ip prio 3 u32 match ip src 0.0.0.3/32 action drop;done | $TC -b -'",
> +        "expExitCode": "0",
> +        "verifyCmd": "$TC filter show dev $DEV1",
> +        "matchPattern": "protocol ip pref 3 u32",
> +        "matchCount": "3",
> +        "teardown": [
> +            "$TC qdisc del dev $DEV1 parent root drr"
> +        ]
>      }
>  ]
> --
> 2.30.2
>

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

[Alexandre Ferrieux]

On 09/11/2024 13:50, Jamal Hadi Salim wrote:
> On Fri, Nov 8, 2024 at 9:12 AM Alexandre Ferrieux
> <alexandre.ferrieux@gmail.com> wrote:
>>
>> This patch adds the missing decoding logic for handles that
>> deserve it, along with a corresponding tdc test.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Reviewed-by: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com>
> 
> 
> Ok, looks good.
> Please split the test into a separate patch targeting net-next.

I'm unfamiliar with the net/net-next dance (beyond the dichotomy between fixes
and new features). Can you please explain why the test should not be in the same
commit ?


> Also your "Fixes" should be:> commit e7614370d6f04711c4e4b48f7055e5008fa4ed42

Ah OK, I see, that's the IDR conversion. I had mistakenly believed it was an
isofunctional change, and upon seeing the "(|0x800)<<20" without "(>>20)&0x7FF"
in the initial commit, I believed it was a 19-year old bug. Then it's only a
7-year-old, thanks a lot for the correction.

> When you send the next version please include my Acked-by:

Will do. Thanks a lot !

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

[Jakub Kicinski]

On Sat, 9 Nov 2024 07:50:53 -0500 Jamal Hadi Salim wrote:
> Please split the test into a separate patch targeting net-next.

Separate patch - okay, but why are you asking people to send the tests
to net-next? These sort of requests lead people to try to run
linux-next tests on stable trees.

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

[Jamal Hadi Salim]

On Mon, Nov 11, 2024 at 1:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 9 Nov 2024 07:50:53 -0500 Jamal Hadi Salim wrote:
> > Please split the test into a separate patch targeting net-next.
>
> Separate patch - okay, but why are you asking people to send the tests
> to net-next? These sort of requests lead people to try to run
> linux-next tests on stable trees.

AFAIK, those are the rules.
The test case is not a fix therefore cant go to -net.
You wait until the fix shows up in net-next then you push the test
case into net-next.
I should have clarified to Alexandre the "wait until the fix shows up
in net-next" part.

cheers,
jamal

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

[Alexandre Ferrieux]

On 12/11/2024 13:23, Jamal Hadi Salim wrote:
> On Mon, Nov 11, 2024 at 1:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Sat, 9 Nov 2024 07:50:53 -0500 Jamal Hadi Salim wrote:
>> > Please split the test into a separate patch targeting net-next.
>>
>> Separate patch - okay, but why are you asking people to send the tests
>> to net-next? These sort of requests lead people to try to run
>> linux-next tests on stable trees.
> 
> AFAIK, those are the rules.
> The test case is not a fix therefore cant go to -net.
> You wait until the fix shows up in net-next then you push the test
> case into net-next.
> I should have clarified to Alexandre the "wait until the fix shows up
> in net-next" part.

OK, thank you fro the clarification, will do that :)

The v7 I posted includes all the remarks received so far:
 - static [inline]
 - test split apart
 - u32 type
 - Fixes: with proper commitid
 - Acked-by you ;)

Please tell me about any remaining issue.

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

[Jakub Kicinski]

On Tue, 12 Nov 2024 07:23:29 -0500 Jamal Hadi Salim wrote:
> > Separate patch - okay, but why are you asking people to send the tests
> > to net-next? These sort of requests lead people to try to run
> > linux-next tests on stable trees.  
> 
> AFAIK, those are the rules.

Do you have more info, or this is more of a "your understanding" thing?
E.g. rules for which subsystem? are they specified somewhere?
I'm used to merging the fix with the selftest, two minor reasons pro:
 - less burden on submitter
 - backporters can see and use the test to validate, immediately
con:
 - higher risk of conflicts, but that's my problem (we really need to
   alpha-sort the makefiles, sigh)

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

[Jamal Hadi Salim]

On Tue, Nov 12, 2024 at 10:18 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 12 Nov 2024 07:23:29 -0500 Jamal Hadi Salim wrote:
> > > Separate patch - okay, but why are you asking people to send the tests
> > > to net-next? These sort of requests lead people to try to run
> > > linux-next tests on stable trees.
> >
> > AFAIK, those are the rules.
>
> Do you have more info, or this is more of a "your understanding" thing?
> E.g. rules for which subsystem? are they specified somewhere?

More "tribal knowledge" than written scripture onn networking subsystem.
Over time i have seen pushback from people of that nature and have
always followed that rule.

> I'm used to merging the fix with the selftest, two minor reasons pro:
>  - less burden on submitter
>  - backporters can see and use the test to validate, immediately
> con:
>  - higher risk of conflicts, but that's my problem (we really need to
>    alpha-sort the makefiles, sigh)

Sounds sensible to me - would help to burn it into scripture.

cheers,
jamal

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

[Jakub Kicinski]

On Tue, 12 Nov 2024 12:07:10 -0500 Jamal Hadi Salim wrote:
> On Tue, Nov 12, 2024 at 10:18 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > I'm used to merging the fix with the selftest, two minor reasons pro:
> >  - less burden on submitter
> >  - backporters can see and use the test to validate, immediately
> > con:
> >  - higher risk of conflicts, but that's my problem (we really need to
> >    alpha-sort the makefiles, sigh)  
> 
> Sounds sensible to me - would help to burn it into scripture.

How does this sound?

  Co-posting selftests
  --------------------

  Selftests should be part of the same series as the code changes.
  Specifically for fixes both code change and related test should go into
  the same tree (the tests may lack a Fixes tag, which is expected).
  Mixing code changes and test changes in a single commit is discouraged.

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

[Alexandre Ferrieux]

On 13/11/2024 02:57, Jakub Kicinski wrote:
> 
>   Co-posting selftests
>   --------------------
> 
>   Selftests should be part of the same series as the code changes.
>   Specifically for fixes both code change and related test should go into
>   the same tree (the tests may lack a Fixes tag, which is expected).
>   Mixing code changes and test changes in a single commit is discouraged.

Hi Jakub,

In accordance to this, I've just posted the test as a separate patch:

 [PATCH net] net: sched: u32: Add test case for systematic hnode IDR leaks

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

[Jamal Hadi Salim]

On Tue, Nov 12, 2024 at 8:57 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 12 Nov 2024 12:07:10 -0500 Jamal Hadi Salim wrote:
> > On Tue, Nov 12, 2024 at 10:18 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > I'm used to merging the fix with the selftest, two minor reasons pro:
> > >  - less burden on submitter
> > >  - backporters can see and use the test to validate, immediately
> > > con:
> > >  - higher risk of conflicts, but that's my problem (we really need to
> > >    alpha-sort the makefiles, sigh)
> >
> > Sounds sensible to me - would help to burn it into scripture.
>
> How does this sound?
>
>   Co-posting selftests
>   --------------------
>
>   Selftests should be part of the same series as the code changes.
>   Specifically for fixes both code change and related test should go into
>   the same tree (the tests may lack a Fixes tag, which is expected).
>   Mixing code changes and test changes in a single commit is discouraged.

Just the last sentence:
Mixing unrelated (to the fix) code changes and test changes in a
single commit is discouraged.

cheers,
jamal

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

[Jakub Kicinski]

On Wed, 13 Nov 2024 09:17:09 -0500 Jamal Hadi Salim wrote:
> >   Co-posting selftests
> >   --------------------
> >
> >   Selftests should be part of the same series as the code changes.
> >   Specifically for fixes both code change and related test should go into
> >   the same tree (the tests may lack a Fixes tag, which is expected).
> >   Mixing code changes and test changes in a single commit is discouraged.  
> 
> Just the last sentence:
> Mixing unrelated (to the fix) code changes and test changes in a
> single commit is discouraged.

Perfect example why things are not documented.
I have no idea what you're trying to say..
I think you're the only person who is getting this wrong,
so I'll toss the documentation patch.