| Message ID | 20241108141159.305966-1-alexandre.ferrieux@orange.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v6] net: sched: cls_u32: Fix u32's systematic failure to free IDR entries for hnodes. | expand |
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 >
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 !
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.
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
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.
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)
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
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.
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
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
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.
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" + ] } ]
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(-)