Commit 386829cd authored by Jakub Kicinski's avatar Jakub Kicinski
Browse files

Merge branch 'replace-direct-dequeue-call-with-qdisc_dequeue_peeked' 

Jamal Hadi Salim says:

====================
Replace direct dequeue call with qdisc_dequeue_peeked

When sfb and red qdiscs have children (eg qfq qdisc) whose peek() callback is
qdisc_peek_dequeued(), we could get a kernel panic. When the parent of such
qdiscs (eg illustrated in patch #3 as tbf) wants to retrieve an skb from
its child (red/sfb in this case), it will do the following:
 1a. do a peek() - and when sensing there's an skb the child can offer, then
     - the child in this case(red/sfb) calls its child's (qfq) peek.
        qfq does the right thing and will return the gso_skb queue packet.
        Note: if there wasnt a gso_skb entry then qfq will store it there.
 1b. invoke a dequeue() on the child (red/sfb). And herein lies the problem.
     - red/sfb will call the child's dequeue() which will essentially just
       try to grab something of qfq's queue.

The right thing to do in #1b is to grab the skb off gso_skb queue.
This patchset fixes that issue by changing #1b to use qdisc_dequeue_peeked()
method instead.

Patch 1 fixes the issue for red qdisc. Patch 2 fixes it for sfb.
Patch 3 adds testcases for the two setups.
====================

Link: https://patch.msgid.link/20260430152957.194015-1-jhs@mojatatu.com


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parents 383d0fb8 3a3a30c1

net/sched/sch_red.c

+1 −1
Original line number Diff line number Diff line
@@ -162,7 +162,7 @@ static struct sk_buff *red_dequeue(struct Qdisc *sch) 
	struct red_sched_data *q = qdisc_priv(sch);

	struct Qdisc *child = q->qdisc;



	skb = child->dequeue(child);

	skb = qdisc_dequeue_peeked(child);

	if (skb) {

		qdisc_bstats_update(sch, skb);

		qdisc_qstats_backlog_dec(sch, skb);

net/sched/sch_sfb.c

+1 −1
Original line number Diff line number Diff line
@@ -441,7 +441,7 @@ static struct sk_buff *sfb_dequeue(struct Qdisc *sch) 
	struct Qdisc *child = q->qdisc;

	struct sk_buff *skb;



	skb = child->dequeue(q->qdisc);

	skb = qdisc_dequeue_peeked(child);



	if (skb) {

		qdisc_bstats_update(sch, skb);

tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json

+148 −0
Original line number Diff line number Diff line
@@ -1136,5 +1136,153 @@ 
        "teardown": [

            "$TC qdisc del dev $DUMMY handle 1: root"

        ]

    },

    {

        "id": "7a5f",

        "name": "Force red to dequeue from its child's gso_skb with qfq leaf",

        "category": [

            "qdisc",

            "tbf",

            "red",

            "qfq"

        ],

        "plugins": {

            "requires": "nsPlugin"

        },

        "setup": [

            "$IP link set dev $DUMMY up || true",

            "$IP addr add 10.10.11.10/24 dev $DUMMY || true",

            "$TC qdisc add dev $DUMMY root handle 1: tbf rate 88bit burst 1661b peakrate 2257333 minburst 1024 limit 7b",

            "$TC qdisc add dev $DUMMY parent 1: handle 2: red limit 757 min 16 max 24 avpkt 16",

            "$TC qdisc add dev $DUMMY parent 2: handle 3: qfq",

            "$TC class add dev $DUMMY classid 3:1 parent 3: qfq maxpkt 512 weight 1",

            "$TC filter add dev $DUMMY parent 3: protocol ip prio 1 matchall classid 3:1 action ok"

        ],

        "cmdUnderTest": "ping -c 1 10.10.10.1 -W0.01 -I$DUMMY || true",

        "expExitCode": "0",

        "verifyCmd": "$TC -s -j qdisc ls dev $DUMMY parent 1:",

        "matchJSON": [

            {

                "kind": "red",

                "handle": "2:",

                "bytes": 98,

                "packets": 1,

                "backlog": 0,

                "qlen": 0

            }

        ],

        "teardown": [

            "$TC qdisc del dev $DUMMY handle 1: root"

        ]

    },

    {

        "id": "cdae",

        "name": "Force sfb to dequeue from its child's gso_skb with qfq leaf",

        "category": [

            "qdisc",

            "tbf",

            "sfb",

            "qfq"

        ],

        "plugins": {

            "requires": "nsPlugin"

        },

        "setup": [

            "$IP link set dev $DUMMY up || true",

            "$IP addr add 10.10.11.10/24 dev $DUMMY || true",

            "$TC qdisc add dev $DUMMY root handle 1: tbf rate 88bit burst 1661b peakrate 2257333 minburst 1024 limit 7b",

            "$TC qdisc add dev $DUMMY parent 1: handle 2: sfb",

            "$TC qdisc add dev $DUMMY parent 2: handle 3: qfq",

            "$TC class add dev $DUMMY classid 3:1 parent 3: qfq maxpkt 512 weight 1",

            "$TC filter add dev $DUMMY parent 3: protocol ip prio 1 matchall classid 3:1 action ok"

        ],

        "cmdUnderTest": "ping -c 1 10.10.10.1 -W0.01 -I$DUMMY || true",

        "expExitCode": "0",

        "verifyCmd": "$TC -s -j qdisc ls dev $DUMMY parent 1:",

        "matchJSON": [

            {

                "kind": "sfb",

                "handle": "2:",

                "bytes": 98,

                "packets": 1,

                "backlog": 0,

                "qlen": 0

            }

        ],

        "teardown": [

            "$TC qdisc del dev $DUMMY handle 1: root"

        ]

    },

    {

        "id": "291d",

        "name": "Force red to dequeue from its child's gso_skb with dualpi2 leaf",

        "category": [

            "qdisc",

            "tbf",

            "red",

            "dualpi2"

        ],

        "plugins": {

            "requires": "nsPlugin"

        },

        "setup": [

            "$IP link set dev $DUMMY up || true",

            "$IP addr add 10.10.11.10/24 dev $DUMMY || true",

            "$TC qdisc add dev $DUMMY root handle 1: tbf rate 88bit burst 1661b peakrate 2257333 minburst 1024 limit 7b",

            "$TC qdisc add dev $DUMMY parent 1: handle 2: red limit 757 min 16 max 24 avpkt 16",

            "$TC qdisc add dev $DUMMY parent 2: handle 3: dualpi2"

        ],

        "cmdUnderTest": "ping -c 1 10.10.10.1 -W0.01 -I$DUMMY || true",

        "expExitCode": "0",

        "verifyCmd": "$TC -s -j qdisc ls dev $DUMMY parent 1:",

        "matchJSON": [

            {

                "kind": "red",

                "handle": "2:",

                "bytes": 98,

                "packets": 1,

                "backlog": 0,

                "qlen": 0

            }

        ],

        "teardown": [

            "$TC qdisc del dev $DUMMY handle 1: root"

        ]

    },

    {

        "id": "9c6d",

        "name": "Force sfb to dequeue from its child's gso_skb with dualpi2 leaf",

        "category": [

            "qdisc",

            "tbf",

            "sfb",

            "dualpi2"

        ],

        "plugins": {

            "requires": "nsPlugin"

        },

        "setup": [

            "$IP link set dev $DUMMY up || true",

            "$IP addr add 10.10.11.10/24 dev $DUMMY || true",

            "$TC qdisc add dev $DUMMY root handle 1: tbf rate 88bit burst 1661b peakrate 2257333 minburst 1024 limit 7b",

            "$TC qdisc add dev $DUMMY parent 1: handle 2: sfb",

            "$TC qdisc add dev $DUMMY parent 2: handle 3: dualpi2"

        ],

        "cmdUnderTest": "ping -c 1 10.10.10.1 -W0.01 -I$DUMMY || true",

        "expExitCode": "0",

        "verifyCmd": "$TC -s -j qdisc ls dev $DUMMY parent 1:",

        "matchJSON": [

            {

                "kind": "sfb",

                "handle": "2:",

                "bytes": 98,

                "packets": 1,

                "backlog": 0,

                "qlen": 0

            }

        ],

        "teardown": [

            "$TC qdisc del dev $DUMMY handle 1: root"

        ]

    }

]