netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch net v3 0/4] net_sched: two security bug fixes and test cases
@ 2025-02-04  0:58 Cong Wang
  2025-02-04  0:58 ` [Patch net v3 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Cong Wang @ 2025-02-04  0:58 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, pctammela, mincho, quanglex97, Cong Wang

From: Cong Wang <cong.wang@bytedance.com>

This patchset contains two bug fixes reported in security mailing list,
and test cases for both of them.

---
v3: added Fixes tag for patch 1/4
    removed a redudant ip link add/del in patch 2/4 (thanks to Pedro Tammela)

v2: replaced dummy2 with $DUMMY in pfifo_head_drop test
    reduced the number of ping's in pfifo_head_drop test
    improved commit messages

Cong Wang (2):
  netem: Update sch->q.qlen before qdisc_tree_reduce_backlog()
  selftests/tc-testing: Add a test case for qdisc_tree_reduce_backlog()

Quang Le (2):
  pfifo_tail_enqueue: Drop new packet when sch->limit == 0
  selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when
    limit==0

 net/sched/sch_fifo.c                          |  3 ++
 net/sched/sch_netem.c                         |  2 +-
 .../tc-testing/tc-tests/infra/qdiscs.json     | 34 ++++++++++++++++++-
 .../tc-testing/tc-tests/qdiscs/fifo.json      | 23 +++++++++++++
 4 files changed, 60 insertions(+), 2 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Patch net v3 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0
  2025-02-04  0:58 [Patch net v3 0/4] net_sched: two security bug fixes and test cases Cong Wang
@ 2025-02-04  0:58 ` Cong Wang
  2025-02-04 11:32   ` Simon Horman
  2025-02-04  0:58 ` [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0 Cong Wang
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2025-02-04  0:58 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, pctammela, mincho, quanglex97, Cong Wang

From: Quang Le <quanglex97@gmail.com>

Expected behaviour:
In case we reach scheduler's limit, pfifo_tail_enqueue() will drop a
packet in scheduler's queue and decrease scheduler's qlen by one.
Then, pfifo_tail_enqueue() enqueue new packet and increase
scheduler's qlen by one. Finally, pfifo_tail_enqueue() return
`NET_XMIT_CN` status code.

Weird behaviour:
In case we set `sch->limit == 0` and trigger pfifo_tail_enqueue() on a
scheduler that has no packet, the 'drop a packet' step will do nothing.
This means the scheduler's qlen still has value equal 0.
Then, we continue to enqueue new packet and increase scheduler's qlen by
one. In summary, we can leverage pfifo_tail_enqueue() to increase qlen by
one and return `NET_XMIT_CN` status code.

The problem is:
Let's say we have two qdiscs: Qdisc_A and Qdisc_B.
 - Qdisc_A's type must have '->graft()' function to create parent/child relationship.
   Let's say Qdisc_A's type is `hfsc`. Enqueue packet to this qdisc will trigger `hfsc_enqueue`.
 - Qdisc_B's type is pfifo_head_drop. Enqueue packet to this qdisc will trigger `pfifo_tail_enqueue`.
 - Qdisc_B is configured to have `sch->limit == 0`.
 - Qdisc_A is configured to route the enqueued's packet to Qdisc_B.

Enqueue packet through Qdisc_A will lead to:
 - hfsc_enqueue(Qdisc_A) -> pfifo_tail_enqueue(Qdisc_B)
 - Qdisc_B->q.qlen += 1
 - pfifo_tail_enqueue() return `NET_XMIT_CN`
 - hfsc_enqueue() check for `NET_XMIT_SUCCESS` and see `NET_XMIT_CN` => hfsc_enqueue() don't increase qlen of Qdisc_A.

The whole process lead to a situation where Qdisc_A->q.qlen == 0 and Qdisc_B->q.qlen == 1.
Replace 'hfsc' with other type (for example: 'drr') still lead to the same problem.
This violate the design where parent's qlen should equal to the sum of its childrens'qlen.

Bug impact: This issue can be used for user->kernel privilege escalation when it is reachable.

Fixes: f70f90672a2c ("sched: add head drop fifo queue")
Reported-by: Quang Le <quanglex97@gmail.com>
Signed-off-by: Quang Le <quanglex97@gmail.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 net/sched/sch_fifo.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c
index b50b2c2cc09b..e6bfd39ff339 100644
--- a/net/sched/sch_fifo.c
+++ b/net/sched/sch_fifo.c
@@ -40,6 +40,9 @@ static int pfifo_tail_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 {
 	unsigned int prev_backlog;
 
+	if (unlikely(READ_ONCE(sch->limit) == 0))
+		return qdisc_drop(skb, sch, to_free);
+
 	if (likely(sch->q.qlen < READ_ONCE(sch->limit)))
 		return qdisc_enqueue_tail(skb, sch);
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0
  2025-02-04  0:58 [Patch net v3 0/4] net_sched: two security bug fixes and test cases Cong Wang
  2025-02-04  0:58 ` [Patch net v3 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
@ 2025-02-04  0:58 ` Cong Wang
  2025-02-04 11:37   ` Simon Horman
  2025-02-04  0:58 ` [Patch net v3 3/4] netem: Update sch->q.qlen before qdisc_tree_reduce_backlog() Cong Wang
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Cong Wang @ 2025-02-04  0:58 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, pctammela, mincho, quanglex97, Cong Wang

From: Quang Le <quanglex97@gmail.com>

When limit == 0, pfifo_tail_enqueue() must drop new packet and
increase dropped packets count of the qdisc.

All test results:

1..16
ok 1 a519 - Add bfifo qdisc with system default parameters on egress
ok 2 585c - Add pfifo qdisc with system default parameters on egress
ok 3 a86e - Add bfifo qdisc with system default parameters on egress with handle of maximum value
ok 4 9ac8 - Add bfifo qdisc on egress with queue size of 3000 bytes
ok 5 f4e6 - Add pfifo qdisc on egress with queue size of 3000 packets
ok 6 b1b1 - Add bfifo qdisc with system default parameters on egress with invalid handle exceeding maximum value
ok 7 8d5e - Add bfifo qdisc on egress with unsupported argument
ok 8 7787 - Add pfifo qdisc on egress with unsupported argument
ok 9 c4b6 - Replace bfifo qdisc on egress with new queue size
ok 10 3df6 - Replace pfifo qdisc on egress with new queue size
ok 11 7a67 - Add bfifo qdisc on egress with queue size in invalid format
ok 12 1298 - Add duplicate bfifo qdisc on egress
ok 13 45a0 - Delete nonexistent bfifo qdisc
ok 14 972b - Add prio qdisc on egress with invalid format for handles
ok 15 4d39 - Delete bfifo qdisc twice
ok 16 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0

Signed-off-by: Quang Le <quanglex97@gmail.com>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 .../tc-testing/tc-tests/qdiscs/fifo.json      | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fifo.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fifo.json
index ae3d286a32b2..6f20d033670d 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fifo.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fifo.json
@@ -313,6 +313,29 @@
         "matchPattern": "qdisc bfifo 1: root",
         "matchCount": "0",
         "teardown": [
+	]
+    },
+    {
+        "id": "d774",
+        "name": "Check pfifo_head_drop qdisc enqueue behaviour when limit == 0",
+        "category": [
+            "qdisc",
+            "pfifo_head_drop"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY root handle 1: pfifo_head_drop limit 0",
+            "$IP link set dev $DUMMY up || true"
+        ],
+        "cmdUnderTest": "ping -c2 -W0.01 -I $DUMMY 10.10.10.1",
+        "expExitCode": "1",
+        "verifyCmd": "$TC -s qdisc show dev $DUMMY",
+        "matchPattern": "dropped 2",
+        "matchCount": "1",
+        "teardown": [
         ]
     }
 ]
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net v3 3/4] netem: Update sch->q.qlen before qdisc_tree_reduce_backlog()
  2025-02-04  0:58 [Patch net v3 0/4] net_sched: two security bug fixes and test cases Cong Wang
  2025-02-04  0:58 ` [Patch net v3 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
  2025-02-04  0:58 ` [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0 Cong Wang
@ 2025-02-04  0:58 ` Cong Wang
  2025-02-04  0:58 ` [Patch net v3 4/4] selftests/tc-testing: Add a test case for qdisc_tree_reduce_backlog() Cong Wang
  2025-02-06  2:20 ` [Patch net v3 0/4] net_sched: two security bug fixes and test cases patchwork-bot+netdevbpf
  4 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2025-02-04  0:58 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, pctammela, mincho, quanglex97, Cong Wang,
	Martin Ottens

From: Cong Wang <cong.wang@bytedance.com>

qdisc_tree_reduce_backlog() notifies parent qdisc only if child
qdisc becomes empty, therefore we need to reduce the backlog of the
child qdisc before calling it. Otherwise it would miss the opportunity
to call cops->qlen_notify(), in the case of DRR, it resulted in UAF
since DRR uses ->qlen_notify() to maintain its active list.

Fixes: f8d4bc455047 ("net/sched: netem: account for backlog updates from child qdisc")
Cc: Martin Ottens <martin.ottens@fau.de>
Reported-by: Mingi Cho <mincho@theori.io>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 net/sched/sch_netem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 71ec9986ed37..fdd79d3ccd8c 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -749,9 +749,9 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch)
 				if (err != NET_XMIT_SUCCESS) {
 					if (net_xmit_drop_count(err))
 						qdisc_qstats_drop(sch);
-					qdisc_tree_reduce_backlog(sch, 1, pkt_len);
 					sch->qstats.backlog -= pkt_len;
 					sch->q.qlen--;
+					qdisc_tree_reduce_backlog(sch, 1, pkt_len);
 				}
 				goto tfifo_dequeue;
 			}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Patch net v3 4/4] selftests/tc-testing: Add a test case for qdisc_tree_reduce_backlog()
  2025-02-04  0:58 [Patch net v3 0/4] net_sched: two security bug fixes and test cases Cong Wang
                   ` (2 preceding siblings ...)
  2025-02-04  0:58 ` [Patch net v3 3/4] netem: Update sch->q.qlen before qdisc_tree_reduce_backlog() Cong Wang
@ 2025-02-04  0:58 ` Cong Wang
  2025-02-06  2:20 ` [Patch net v3 0/4] net_sched: two security bug fixes and test cases patchwork-bot+netdevbpf
  4 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2025-02-04  0:58 UTC (permalink / raw)
  To: netdev; +Cc: jhs, jiri, pctammela, mincho, quanglex97, Cong Wang

From: Cong Wang <cong.wang@bytedance.com>

Integrate the test case provided by Mingi Cho into TDC.

All test results:

1..4
ok 1 ca5e - Check class delete notification for ffff:
ok 2 e4b7 - Check class delete notification for root ffff:
ok 3 33a9 - Check ingress is not searchable on backlog update
ok 4 a4b9 - Test class qlen notification

Cc: Mingi Cho <mincho@theori.io>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 34 ++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
index d3dd65b05b5f..9044ac054167 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -94,5 +94,37 @@
             "$TC qdisc del dev $DUMMY ingress",
             "$IP addr del 10.10.10.10/24 dev $DUMMY"
         ]
-    }
+    },
+    {
+	"id": "a4b9",
+	"name": "Test class qlen notification",
+	"category": [
+	    "qdisc"
+	],
+	"plugins": {
+	    "requires": "nsPlugin"
+	},
+	"setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY root handle 1: drr",
+            "$TC filter add dev $DUMMY parent 1: basic classid 1:1",
+            "$TC class add dev $DUMMY parent 1: classid 1:1 drr",
+            "$TC qdisc add dev $DUMMY parent 1:1 handle 2: netem",
+            "$TC qdisc add dev $DUMMY parent 2: handle 3: drr",
+            "$TC filter add dev $DUMMY parent 3: basic action drop",
+            "$TC class add dev $DUMMY parent 3: classid 3:1 drr",
+            "$TC class del dev $DUMMY classid 1:1",
+            "$TC class add dev $DUMMY parent 1: classid 1:1 drr"
+        ],
+        "cmdUnderTest": "ping -c1 -W0.01 -I $DUMMY 10.10.10.1",
+        "expExitCode": "1",
+        "verifyCmd": "$TC qdisc ls dev $DUMMY",
+        "matchPattern": "drr 1: root",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY root handle 1: drr",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY"
+        ]
+   }
 ]
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Patch net v3 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0
  2025-02-04  0:58 ` [Patch net v3 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
@ 2025-02-04 11:32   ` Simon Horman
  2025-02-06 17:40     ` Cong Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Simon Horman @ 2025-02-04 11:32 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, jhs, jiri, pctammela, mincho, quanglex97, Cong Wang

On Mon, Feb 03, 2025 at 04:58:38PM -0800, Cong Wang wrote:
> From: Quang Le <quanglex97@gmail.com>
> 
> Expected behaviour:
> In case we reach scheduler's limit, pfifo_tail_enqueue() will drop a
> packet in scheduler's queue and decrease scheduler's qlen by one.
> Then, pfifo_tail_enqueue() enqueue new packet and increase
> scheduler's qlen by one. Finally, pfifo_tail_enqueue() return
> `NET_XMIT_CN` status code.
> 
> Weird behaviour:
> In case we set `sch->limit == 0` and trigger pfifo_tail_enqueue() on a
> scheduler that has no packet, the 'drop a packet' step will do nothing.
> This means the scheduler's qlen still has value equal 0.
> Then, we continue to enqueue new packet and increase scheduler's qlen by
> one. In summary, we can leverage pfifo_tail_enqueue() to increase qlen by
> one and return `NET_XMIT_CN` status code.
> 
> The problem is:
> Let's say we have two qdiscs: Qdisc_A and Qdisc_B.
>  - Qdisc_A's type must have '->graft()' function to create parent/child relationship.
>    Let's say Qdisc_A's type is `hfsc`. Enqueue packet to this qdisc will trigger `hfsc_enqueue`.
>  - Qdisc_B's type is pfifo_head_drop. Enqueue packet to this qdisc will trigger `pfifo_tail_enqueue`.
>  - Qdisc_B is configured to have `sch->limit == 0`.
>  - Qdisc_A is configured to route the enqueued's packet to Qdisc_B.
> 
> Enqueue packet through Qdisc_A will lead to:
>  - hfsc_enqueue(Qdisc_A) -> pfifo_tail_enqueue(Qdisc_B)
>  - Qdisc_B->q.qlen += 1
>  - pfifo_tail_enqueue() return `NET_XMIT_CN`
>  - hfsc_enqueue() check for `NET_XMIT_SUCCESS` and see `NET_XMIT_CN` => hfsc_enqueue() don't increase qlen of Qdisc_A.
> 
> The whole process lead to a situation where Qdisc_A->q.qlen == 0 and Qdisc_B->q.qlen == 1.
> Replace 'hfsc' with other type (for example: 'drr') still lead to the same problem.
> This violate the design where parent's qlen should equal to the sum of its childrens'qlen.
> 
> Bug impact: This issue can be used for user->kernel privilege escalation when it is reachable.
> 
> Fixes: f70f90672a2c ("sched: add head drop fifo queue")

Hi Cong,

Not a proper review, but I believe the hash in mainline for the cited
commit is 57dbb2d83d100ea.

> Reported-by: Quang Le <quanglex97@gmail.com>
> Signed-off-by: Quang Le <quanglex97@gmail.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>

...

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0
  2025-02-04  0:58 ` [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0 Cong Wang
@ 2025-02-04 11:37   ` Simon Horman
  2025-02-04 16:46     ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Simon Horman @ 2025-02-04 11:37 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, jhs, jiri, pctammela, mincho, quanglex97, Cong Wang

On Mon, Feb 03, 2025 at 04:58:39PM -0800, Cong Wang wrote:
> From: Quang Le <quanglex97@gmail.com>
> 
> When limit == 0, pfifo_tail_enqueue() must drop new packet and
> increase dropped packets count of the qdisc.
> 
> All test results:
> 
> 1..16
> ok 1 a519 - Add bfifo qdisc with system default parameters on egress
> ok 2 585c - Add pfifo qdisc with system default parameters on egress
> ok 3 a86e - Add bfifo qdisc with system default parameters on egress with handle of maximum value
> ok 4 9ac8 - Add bfifo qdisc on egress with queue size of 3000 bytes
> ok 5 f4e6 - Add pfifo qdisc on egress with queue size of 3000 packets
> ok 6 b1b1 - Add bfifo qdisc with system default parameters on egress with invalid handle exceeding maximum value
> ok 7 8d5e - Add bfifo qdisc on egress with unsupported argument
> ok 8 7787 - Add pfifo qdisc on egress with unsupported argument
> ok 9 c4b6 - Replace bfifo qdisc on egress with new queue size
> ok 10 3df6 - Replace pfifo qdisc on egress with new queue size
> ok 11 7a67 - Add bfifo qdisc on egress with queue size in invalid format
> ok 12 1298 - Add duplicate bfifo qdisc on egress
> ok 13 45a0 - Delete nonexistent bfifo qdisc
> ok 14 972b - Add prio qdisc on egress with invalid format for handles
> ok 15 4d39 - Delete bfifo qdisc twice
> ok 16 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
> 
> Signed-off-by: Quang Le <quanglex97@gmail.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>

Hi Cong,

Unfortunately this test still seems to be failing in the CI.

# not ok 577 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
# Could not match regex pattern. Verify command output:
# qdisc pfifo_head_drop 1: root refcnt 2 limit 0p
#  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
#  backlog 0b 0p requeues 0

https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/977485/1-tdc-sh/stdout

-- 
pw-bot: cr

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0
  2025-02-04 11:37   ` Simon Horman
@ 2025-02-04 16:46     ` Jakub Kicinski
  2025-02-05  2:21       ` Pedro Tammela
  0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2025-02-04 16:46 UTC (permalink / raw)
  To: pctammela
  Cc: Simon Horman, Cong Wang, netdev, jhs, jiri, mincho, quanglex97,
	Cong Wang

On Tue, 4 Feb 2025 11:37:03 +0000 Simon Horman wrote:
> On Mon, Feb 03, 2025 at 04:58:39PM -0800, Cong Wang wrote:
> > From: Quang Le <quanglex97@gmail.com>
> > 
> > When limit == 0, pfifo_tail_enqueue() must drop new packet and
> > increase dropped packets count of the qdisc.
> > 
> > All test results:
> > 
> > 1..16
> > ok 1 a519 - Add bfifo qdisc with system default parameters on egress
> > ok 2 585c - Add pfifo qdisc with system default parameters on egress
> > ok 3 a86e - Add bfifo qdisc with system default parameters on egress with handle of maximum value
> > ok 4 9ac8 - Add bfifo qdisc on egress with queue size of 3000 bytes
> > ok 5 f4e6 - Add pfifo qdisc on egress with queue size of 3000 packets
> > ok 6 b1b1 - Add bfifo qdisc with system default parameters on egress with invalid handle exceeding maximum value
> > ok 7 8d5e - Add bfifo qdisc on egress with unsupported argument
> > ok 8 7787 - Add pfifo qdisc on egress with unsupported argument
> > ok 9 c4b6 - Replace bfifo qdisc on egress with new queue size
> > ok 10 3df6 - Replace pfifo qdisc on egress with new queue size
> > ok 11 7a67 - Add bfifo qdisc on egress with queue size in invalid format
> > ok 12 1298 - Add duplicate bfifo qdisc on egress
> > ok 13 45a0 - Delete nonexistent bfifo qdisc
> > ok 14 972b - Add prio qdisc on egress with invalid format for handles
> > ok 15 4d39 - Delete bfifo qdisc twice
> > ok 16 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
> > 
> > Signed-off-by: Quang Le <quanglex97@gmail.com>
> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>  
> 
> Hi Cong,
> 
> Unfortunately this test still seems to be failing in the CI.
> 
> # not ok 577 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
> # Could not match regex pattern. Verify command output:
> # qdisc pfifo_head_drop 1: root refcnt 2 limit 0p
> #  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> #  backlog 0b 0p requeues 0
> 
> https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/977485/1-tdc-sh/stdout

This is starting to feel too much like a setup issue.
Pedro, would you be able to take this series and investigate
why it fails on the TDC runner?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0
  2025-02-04 16:46     ` Jakub Kicinski
@ 2025-02-05  2:21       ` Pedro Tammela
  2025-02-05  2:38         ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Pedro Tammela @ 2025-02-05  2:21 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Simon Horman, Cong Wang, netdev, jhs, jiri, mincho, quanglex97,
	Cong Wang

On 04/02/2025 13:46, Jakub Kicinski wrote:
> On Tue, 4 Feb 2025 11:37:03 +0000 Simon Horman wrote:
>> On Mon, Feb 03, 2025 at 04:58:39PM -0800, Cong Wang wrote:
>>> From: Quang Le <quanglex97@gmail.com>
>>>
>>> When limit == 0, pfifo_tail_enqueue() must drop new packet and
>>> increase dropped packets count of the qdisc.
>>>
>>> All test results:
>>>
>>> 1..16
>>> ok 1 a519 - Add bfifo qdisc with system default parameters on egress
>>> ok 2 585c - Add pfifo qdisc with system default parameters on egress
>>> ok 3 a86e - Add bfifo qdisc with system default parameters on egress with handle of maximum value
>>> ok 4 9ac8 - Add bfifo qdisc on egress with queue size of 3000 bytes
>>> ok 5 f4e6 - Add pfifo qdisc on egress with queue size of 3000 packets
>>> ok 6 b1b1 - Add bfifo qdisc with system default parameters on egress with invalid handle exceeding maximum value
>>> ok 7 8d5e - Add bfifo qdisc on egress with unsupported argument
>>> ok 8 7787 - Add pfifo qdisc on egress with unsupported argument
>>> ok 9 c4b6 - Replace bfifo qdisc on egress with new queue size
>>> ok 10 3df6 - Replace pfifo qdisc on egress with new queue size
>>> ok 11 7a67 - Add bfifo qdisc on egress with queue size in invalid format
>>> ok 12 1298 - Add duplicate bfifo qdisc on egress
>>> ok 13 45a0 - Delete nonexistent bfifo qdisc
>>> ok 14 972b - Add prio qdisc on egress with invalid format for handles
>>> ok 15 4d39 - Delete bfifo qdisc twice
>>> ok 16 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
>>>
>>> Signed-off-by: Quang Le <quanglex97@gmail.com>
>>> Signed-off-by: Cong Wang <cong.wang@bytedance.com>
>>
>> Hi Cong,
>>
>> Unfortunately this test still seems to be failing in the CI.
>>
>> # not ok 577 d774 - Check pfifo_head_drop qdisc enqueue behaviour when limit == 0
>> # Could not match regex pattern. Verify command output:
>> # qdisc pfifo_head_drop 1: root refcnt 2 limit 0p
>> #  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>> #  backlog 0b 0p requeues 0
>>
>> https://github.com/p4tc-dev/tc-executor/blob/storage/artifacts/977485/1-tdc-sh/stdout
> 
> This is starting to feel too much like a setup issue.
> Pedro, would you be able to take this series and investigate
> why it fails on the TDC runner?

It should be OK now

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0
  2025-02-05  2:21       ` Pedro Tammela
@ 2025-02-05  2:38         ` Jakub Kicinski
  2025-02-05 17:20           ` Simon Horman
  0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2025-02-05  2:38 UTC (permalink / raw)
  To: Pedro Tammela
  Cc: Simon Horman, Cong Wang, netdev, jhs, jiri, mincho, quanglex97,
	Cong Wang

On Tue, 4 Feb 2025 23:21:07 -0300 Pedro Tammela wrote:
> > This is starting to feel too much like a setup issue.
> > Pedro, would you be able to take this series and investigate
> > why it fails on the TDC runner?  
> 
> It should be OK now

Thank you!

Revived in PW, should get into the run 22 min from now.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0
  2025-02-05  2:38         ` Jakub Kicinski
@ 2025-02-05 17:20           ` Simon Horman
  2025-02-06 17:37             ` Cong Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Simon Horman @ 2025-02-05 17:20 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Pedro Tammela, Cong Wang, netdev, jhs, jiri, mincho, quanglex97,
	Cong Wang

On Tue, Feb 04, 2025 at 06:38:51PM -0800, Jakub Kicinski wrote:
> On Tue, 4 Feb 2025 23:21:07 -0300 Pedro Tammela wrote:
> > > This is starting to feel too much like a setup issue.
> > > Pedro, would you be able to take this series and investigate
> > > why it fails on the TDC runner?  
> > 
> > It should be OK now
> 
> Thank you!
> 
> Revived in PW, should get into the run 22 min from now.

TDC and all other tests appear to be passing now :)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net v3 0/4] net_sched: two security bug fixes and test cases
  2025-02-04  0:58 [Patch net v3 0/4] net_sched: two security bug fixes and test cases Cong Wang
                   ` (3 preceding siblings ...)
  2025-02-04  0:58 ` [Patch net v3 4/4] selftests/tc-testing: Add a test case for qdisc_tree_reduce_backlog() Cong Wang
@ 2025-02-06  2:20 ` patchwork-bot+netdevbpf
  4 siblings, 0 replies; 14+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-02-06  2:20 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, jhs, jiri, pctammela, mincho, quanglex97, cong.wang

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  3 Feb 2025 16:58:37 -0800 you wrote:
> From: Cong Wang <cong.wang@bytedance.com>
> 
> This patchset contains two bug fixes reported in security mailing list,
> and test cases for both of them.
> 
> ---
> v3: added Fixes tag for patch 1/4
>     removed a redudant ip link add/del in patch 2/4 (thanks to Pedro Tammela)
> 
> [...]

Here is the summary with links:
  - [net,v3,1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0
    https://git.kernel.org/netdev/net/c/647cef20e649
  - [net,v3,2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0
    https://git.kernel.org/netdev/net/c/3fe5648d1df1
  - [net,v3,3/4] netem: Update sch->q.qlen before qdisc_tree_reduce_backlog()
    https://git.kernel.org/netdev/net/c/638ba5089324
  - [net,v3,4/4] selftests/tc-testing: Add a test case for qdisc_tree_reduce_backlog()
    https://git.kernel.org/netdev/net/c/91aadc16ee73

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0
  2025-02-05 17:20           ` Simon Horman
@ 2025-02-06 17:37             ` Cong Wang
  0 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2025-02-06 17:37 UTC (permalink / raw)
  To: Simon Horman
  Cc: Jakub Kicinski, Pedro Tammela, netdev, jhs, jiri, mincho,
	quanglex97, Cong Wang

On Wed, Feb 05, 2025 at 05:20:21PM +0000, Simon Horman wrote:
> On Tue, Feb 04, 2025 at 06:38:51PM -0800, Jakub Kicinski wrote:
> > On Tue, 4 Feb 2025 23:21:07 -0300 Pedro Tammela wrote:
> > > > This is starting to feel too much like a setup issue.
> > > > Pedro, would you be able to take this series and investigate
> > > > why it fails on the TDC runner?  
> > > 
> > > It should be OK now
> > 
> > Thank you!
> > 
> > Revived in PW, should get into the run 22 min from now.
> 
> TDC and all other tests appear to be passing now :)

Great. Thanks to everyone here for the help!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Patch net v3 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0
  2025-02-04 11:32   ` Simon Horman
@ 2025-02-06 17:40     ` Cong Wang
  0 siblings, 0 replies; 14+ messages in thread
From: Cong Wang @ 2025-02-06 17:40 UTC (permalink / raw)
  To: Simon Horman; +Cc: netdev, jhs, jiri, pctammela, mincho, quanglex97, Cong Wang

On Tue, Feb 04, 2025 at 11:32:07AM +0000, Simon Horman wrote:
> Hi Cong,
> 
> Not a proper review, but I believe the hash in mainline for the cited
> commit is 57dbb2d83d100ea.
> 

Oops, my bad. I noticed (probably) Jakub fixed it before applying, I
really appreciate that.

Thanks!

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2025-02-06 17:40 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-04  0:58 [Patch net v3 0/4] net_sched: two security bug fixes and test cases Cong Wang
2025-02-04  0:58 ` [Patch net v3 1/4] pfifo_tail_enqueue: Drop new packet when sch->limit == 0 Cong Wang
2025-02-04 11:32   ` Simon Horman
2025-02-06 17:40     ` Cong Wang
2025-02-04  0:58 ` [Patch net v3 2/4] selftests/tc-testing: Add a test case for pfifo_head_drop qdisc when limit==0 Cong Wang
2025-02-04 11:37   ` Simon Horman
2025-02-04 16:46     ` Jakub Kicinski
2025-02-05  2:21       ` Pedro Tammela
2025-02-05  2:38         ` Jakub Kicinski
2025-02-05 17:20           ` Simon Horman
2025-02-06 17:37             ` Cong Wang
2025-02-04  0:58 ` [Patch net v3 3/4] netem: Update sch->q.qlen before qdisc_tree_reduce_backlog() Cong Wang
2025-02-04  0:58 ` [Patch net v3 4/4] selftests/tc-testing: Add a test case for qdisc_tree_reduce_backlog() Cong Wang
2025-02-06  2:20 ` [Patch net v3 0/4] net_sched: two security bug fixes and test cases patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).