On 2023/5/29 3:05, Jamal Hadi Salim wrote:

On Sat, May 27, 2023 at 5:30 AM Zhengchao Shao [off-list ref] wrote:

quoted

When use the following command to test:
1)ip link add bond0 type bond
2)ip link set bond0 up
3)tc qdisc add dev bond0 root handle ffff: mq
4)tc qdisc replace dev bond0 parent ffff:fff1 handle ffff: mq

This is fixed by Peilin in this ongoing discussion:
https://lore.kernel.org/netdev/cover.1684887977.git.peilin.ye@bytedance.com/ (local)

cheers,
jamal

Hi jamal:
	Thank you for your reply. I have notice Peilin's patches before,
and test after the patch is incorporated in local host. But it still
triggers the problem.
	Peilin's patches can be filtered out when the query result of
qdisc_lookup is of the ingress type. Here is 4/6 patch in his patches.
+if (q->flags & TCQ_F_INGRESS) {
+	NL_SET_ERR_MSG(extack,
+		       "Cannot regraft ingress or clsact Qdiscs");
+	return -EINVAL;
+}
	However, the query result of my test case in qdisc_lookup is mq.
Therefore, the patch cannot solve my problem.
Thank you.

Zhengchao Shao

quoted

The kernel reports NULL pointer dereference issue. The stack information
is as follows:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
Internal error: Oops: 0000000096000006 [#1] SMP
Modules linked in:
pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : mq_attach+0x44/0xa0
lr : qdisc_graft+0x20c/0x5cc
sp : ffff80000e2236a0
x29: ffff80000e2236a0 x28: ffff0000c0e59d80 x27: ffff0000c0be19c0
x26: ffff0000cae3e800 x25: 0000000000000010 x24: 00000000fffffff1
x23: 0000000000000000 x22: ffff0000cae3e800 x21: ffff0000c9df4000
x20: ffff0000c9df4000 x19: 0000000000000000 x18: ffff80000a934000
x17: ffff8000f5b56000 x16: ffff80000bb08000 x15: 0000000000000000
x14: 0000000000000000 x13: 6b6b6b6b6b6b6b6b x12: 6b6b6b6b00000001
x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
x8 : ffff0000c0be0730 x7 : bbbbbbbbbbbbbbbb x6 : 0000000000000008
x5 : ffff0000cae3e864 x4 : 0000000000000000 x3 : 0000000000000001
x2 : 0000000000000001 x1 : ffff8000090bc23c x0 : 0000000000000000
Call trace:
mq_attach+0x44/0xa0
qdisc_graft+0x20c/0x5cc
tc_modify_qdisc+0x1c4/0x664
rtnetlink_rcv_msg+0x354/0x440
netlink_rcv_skb+0x64/0x144
rtnetlink_rcv+0x28/0x34
netlink_unicast+0x1e8/0x2a4
netlink_sendmsg+0x308/0x4a0
sock_sendmsg+0x64/0xac
____sys_sendmsg+0x29c/0x358
___sys_sendmsg+0x90/0xd0
__sys_sendmsg+0x7c/0xd0
__arm64_sys_sendmsg+0x2c/0x38
invoke_syscall+0x54/0x114
el0_svc_common.constprop.1+0x90/0x174
do_el0_svc+0x3c/0xb0
el0_svc+0x24/0xec
el0t_64_sync_handler+0x90/0xb4
el0t_64_sync+0x174/0x178

This is because when mq is added for the first time, qdiscs in mq is set
to NULL in mq_attach(). Therefore, when replacing mq after adding mq, we
need to initialize qdiscs in the mq before continuing to graft. Otherwise,
it will couse NULL pointer dereference issue in mq_attach(). And the same
issue will occur in the attach functions of mqprio, taprio and htb.
ffff:fff1 means that the repalce qdisc is ingress. Ingress does not allow
any qdisc to be attached. Therefore, ffff:fff1 is incorrectly used, and
the command should be dropped.

Fixes: 6ec1c69a8f64 ("net_sched: add classful multiqueue dummy scheduler")
Signed-off-by: Zhengchao Shao <redacted>
---
  net/sched/sch_api.c | 4 ++++
  1 file changed, 4 insertions(+)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index fdb8f429333d..dbc9cf5eea89 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c

@@ -1596,6 +1596,10 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
                                         NL_SET_ERR_MSG(extack, "Qdisc parent/child loop detected");
                                         return -ELOOP;
                                 }
+                               if (clid == TC_H_INGRESS) {
+                                       NL_SET_ERR_MSG(extack, "Ingress cannot graft directly");
+                                       return -EINVAL;
+                               }
                                 qdisc_refcount_inc(q);
                                 goto graft;
                         } else {
--

2.34.1