* [PATCH v2 net 1/6] net/sched: sch_ingress: Only create under TC_H_INGRESS
2023-05-22 23:51 [PATCH v2 net 0/6] net/sched: Fixes for sch_ingress and sch_clsact Peilin Ye
@ 2023-05-22 23:53 ` Peilin Ye
2023-05-22 23:54 ` [PATCH v2 net 2/6] net/sched: sch_clsact: Only create under TC_H_CLSACT Peilin Ye
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Peilin Ye @ 2023-05-22 23:53 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Peilin Ye, Daniel Borkmann, John Fastabend, Vlad Buslov,
Pedro Tammela, Hillf Danton, netdev, linux-kernel, Cong Wang,
Peilin Ye
ingress Qdiscs are only supposed to be created under TC_H_INGRESS.
Similar to mq_init(), return -EOPNOTSUPP if 'parent' is not
TC_H_INGRESS.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+b53a9c0d1ea4ad62da8b@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/r/0000000000006cf87705f79acf1a@google.com/
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
net/sched/sch_ingress.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 84838128b9c5..3d71f7a3b4ad 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -80,6 +80,9 @@ static int ingress_init(struct Qdisc *sch, struct nlattr *opt,
struct net_device *dev = qdisc_dev(sch);
int err;
+ if (sch->parent != TC_H_INGRESS)
+ return -EOPNOTSUPP;
+
net_inc_ingress_queue();
mini_qdisc_pair_init(&q->miniqp, sch, &dev->miniq_ingress);
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 net 2/6] net/sched: sch_clsact: Only create under TC_H_CLSACT
2023-05-22 23:51 [PATCH v2 net 0/6] net/sched: Fixes for sch_ingress and sch_clsact Peilin Ye
2023-05-22 23:53 ` [PATCH v2 net 1/6] net/sched: sch_ingress: Only create under TC_H_INGRESS Peilin Ye
@ 2023-05-22 23:54 ` Peilin Ye
2023-05-22 23:54 ` [PATCH v2 net 3/6] net/sched: Reserve TC_H_INGRESS (TC_H_CLSACT) for ingress (clsact) Qdiscs Peilin Ye
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Peilin Ye @ 2023-05-22 23:54 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Peilin Ye, Daniel Borkmann, John Fastabend, Vlad Buslov,
Pedro Tammela, Hillf Danton, netdev, linux-kernel, Cong Wang,
Peilin Ye
clsact Qdiscs are only supposed to be created under TC_H_CLSACT (which
equals TC_H_INGRESS). Return -EOPNOTSUPP if 'parent' is not
TC_H_CLSACT.
Fixes: 1f211a1b929c ("net, sched: add clsact qdisc")
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
net/sched/sch_ingress.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 3d71f7a3b4ad..13218a1fe4a5 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -222,6 +222,9 @@ static int clsact_init(struct Qdisc *sch, struct nlattr *opt,
struct net_device *dev = qdisc_dev(sch);
int err;
+ if (sch->parent != TC_H_CLSACT)
+ return -EOPNOTSUPP;
+
net_inc_ingress_queue();
net_inc_egress_queue();
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 net 3/6] net/sched: Reserve TC_H_INGRESS (TC_H_CLSACT) for ingress (clsact) Qdiscs
2023-05-22 23:51 [PATCH v2 net 0/6] net/sched: Fixes for sch_ingress and sch_clsact Peilin Ye
2023-05-22 23:53 ` [PATCH v2 net 1/6] net/sched: sch_ingress: Only create under TC_H_INGRESS Peilin Ye
2023-05-22 23:54 ` [PATCH v2 net 2/6] net/sched: sch_clsact: Only create under TC_H_CLSACT Peilin Ye
@ 2023-05-22 23:54 ` Peilin Ye
2023-05-22 23:55 ` [PATCH v2 net 4/6] net/sched: Prohibit regrafting ingress or clsact Qdiscs Peilin Ye
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Peilin Ye @ 2023-05-22 23:54 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Peilin Ye, Daniel Borkmann, John Fastabend, Vlad Buslov,
Pedro Tammela, Hillf Danton, netdev, linux-kernel, Cong Wang,
Peilin Ye
Currently it is possible to add e.g. an HTB Qdisc under ffff:fff1
(TC_H_INGRESS, TC_H_CLSACT):
$ ip link add name ifb0 type ifb
$ tc qdisc add dev ifb0 parent ffff:fff1 htb
$ tc qdisc add dev ifb0 clsact
Error: Exclusivity flag on, cannot modify.
$ drgn
...
>>> ifb0 = netdev_get_by_name(prog, "ifb0")
>>> qdisc = ifb0.ingress_queue.qdisc_sleeping
>>> print(qdisc.ops.id.string_().decode())
htb
>>> qdisc.flags.value_() # TCQ_F_INGRESS
2
Only allow ingress and clsact Qdiscs under ffff:fff1. Return -EINVAL
for everything else. Make TCQ_F_INGRESS a static flag of ingress and
clsact Qdiscs.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Fixes: 1f211a1b929c ("net, sched: add clsact qdisc")
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
net/sched/sch_api.c | 7 ++++++-
net/sched/sch_ingress.c | 4 ++--
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index fdb8f429333d..383195955b7d 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1252,7 +1252,12 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
sch->parent = parent;
if (handle == TC_H_INGRESS) {
- sch->flags |= TCQ_F_INGRESS;
+ if (!(sch->flags & TCQ_F_INGRESS)) {
+ NL_SET_ERR_MSG(extack,
+ "Specified parent ID is reserved for ingress and clsact Qdiscs");
+ err = -EINVAL;
+ goto err_out3;
+ }
handle = TC_H_MAKE(TC_H_INGRESS, 0);
} else {
if (handle == 0) {
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 13218a1fe4a5..caea51e0d4e9 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -137,7 +137,7 @@ static struct Qdisc_ops ingress_qdisc_ops __read_mostly = {
.cl_ops = &ingress_class_ops,
.id = "ingress",
.priv_size = sizeof(struct ingress_sched_data),
- .static_flags = TCQ_F_CPUSTATS,
+ .static_flags = TCQ_F_INGRESS | TCQ_F_CPUSTATS,
.init = ingress_init,
.destroy = ingress_destroy,
.dump = ingress_dump,
@@ -275,7 +275,7 @@ static struct Qdisc_ops clsact_qdisc_ops __read_mostly = {
.cl_ops = &clsact_class_ops,
.id = "clsact",
.priv_size = sizeof(struct clsact_sched_data),
- .static_flags = TCQ_F_CPUSTATS,
+ .static_flags = TCQ_F_INGRESS | TCQ_F_CPUSTATS,
.init = clsact_init,
.destroy = clsact_destroy,
.dump = ingress_dump,
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 net 4/6] net/sched: Prohibit regrafting ingress or clsact Qdiscs
2023-05-22 23:51 [PATCH v2 net 0/6] net/sched: Fixes for sch_ingress and sch_clsact Peilin Ye
` (2 preceding siblings ...)
2023-05-22 23:54 ` [PATCH v2 net 3/6] net/sched: Reserve TC_H_INGRESS (TC_H_CLSACT) for ingress (clsact) Qdiscs Peilin Ye
@ 2023-05-22 23:55 ` Peilin Ye
2023-05-22 23:55 ` [PATCH v2 net 5/6] net/sched: Refactor qdisc_graft() for ingress and " Peilin Ye
2023-05-22 23:55 ` [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting Peilin Ye
5 siblings, 0 replies; 13+ messages in thread
From: Peilin Ye @ 2023-05-22 23:55 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Peilin Ye, Daniel Borkmann, John Fastabend, Vlad Buslov,
Pedro Tammela, Hillf Danton, netdev, linux-kernel, Cong Wang,
Peilin Ye
Currently, after creating an ingress (or clsact) Qdisc and grafting it
under TC_H_INGRESS (TC_H_CLSACT), it is possible to graft it again under
e.g. a TBF Qdisc:
$ ip link add ifb0 type ifb
$ tc qdisc add dev ifb0 handle 1: root tbf rate 20kbit buffer 1600 limit 3000
$ tc qdisc add dev ifb0 clsact
$ tc qdisc link dev ifb0 handle ffff: parent 1:1
$ tc qdisc show dev ifb0
qdisc tbf 1: root refcnt 2 rate 20Kbit burst 1600b lat 560.0ms
qdisc clsact ffff: parent ffff:fff1 refcnt 2
^^^^^^^^
clsact's refcount has increased: it is now grafted under both
TC_H_CLSACT and 1:1.
ingress and clsact Qdiscs should only be used under TC_H_INGRESS
(TC_H_CLSACT). Prohibit regrafting them.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Fixes: 1f211a1b929c ("net, sched: add clsact qdisc")
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
net/sched/sch_api.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 383195955b7d..49b9c1bbfdd9 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1596,6 +1596,11 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
NL_SET_ERR_MSG(extack, "Invalid qdisc name");
return -EINVAL;
}
+ if (q->flags & TCQ_F_INGRESS) {
+ NL_SET_ERR_MSG(extack,
+ "Cannot regraft ingress or clsact Qdiscs");
+ return -EINVAL;
+ }
if (q == p ||
(p && check_loop(q, p, 0))) {
NL_SET_ERR_MSG(extack, "Qdisc parent/child loop detected");
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 net 5/6] net/sched: Refactor qdisc_graft() for ingress and clsact Qdiscs
2023-05-22 23:51 [PATCH v2 net 0/6] net/sched: Fixes for sch_ingress and sch_clsact Peilin Ye
` (3 preceding siblings ...)
2023-05-22 23:55 ` [PATCH v2 net 4/6] net/sched: Prohibit regrafting ingress or clsact Qdiscs Peilin Ye
@ 2023-05-22 23:55 ` Peilin Ye
2023-05-22 23:55 ` [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting Peilin Ye
5 siblings, 0 replies; 13+ messages in thread
From: Peilin Ye @ 2023-05-22 23:55 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Peilin Ye, Daniel Borkmann, John Fastabend, Vlad Buslov,
Pedro Tammela, Hillf Danton, netdev, linux-kernel, Cong Wang,
Peilin Ye
Grafting ingress and clsact Qdiscs does not need a for-loop in
qdisc_graft(). Refactor it. No functional changes intended.
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Tested-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
net/sched/sch_api.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 49b9c1bbfdd9..f72a581666a2 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1073,12 +1073,12 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
if (parent == NULL) {
unsigned int i, num_q, ingress;
+ struct netdev_queue *dev_queue;
ingress = 0;
num_q = dev->num_tx_queues;
if ((q && q->flags & TCQ_F_INGRESS) ||
(new && new->flags & TCQ_F_INGRESS)) {
- num_q = 1;
ingress = 1;
if (!dev_ingress_queue(dev)) {
NL_SET_ERR_MSG(extack, "Device does not have an ingress queue");
@@ -1094,18 +1094,18 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
if (new && new->ops->attach && !ingress)
goto skip;
- for (i = 0; i < num_q; i++) {
- struct netdev_queue *dev_queue = dev_ingress_queue(dev);
-
- if (!ingress)
+ if (!ingress) {
+ for (i = 0; i < num_q; i++) {
dev_queue = netdev_get_tx_queue(dev, i);
+ old = dev_graft_qdisc(dev_queue, new);
- old = dev_graft_qdisc(dev_queue, new);
- if (new && i > 0)
- qdisc_refcount_inc(new);
-
- if (!ingress)
+ if (new && i > 0)
+ qdisc_refcount_inc(new);
qdisc_put(old);
+ }
+ } else {
+ dev_queue = dev_ingress_queue(dev);
+ old = dev_graft_qdisc(dev_queue, new);
}
skip:
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting
2023-05-22 23:51 [PATCH v2 net 0/6] net/sched: Fixes for sch_ingress and sch_clsact Peilin Ye
` (4 preceding siblings ...)
2023-05-22 23:55 ` [PATCH v2 net 5/6] net/sched: Refactor qdisc_graft() for ingress and " Peilin Ye
@ 2023-05-22 23:55 ` Peilin Ye
2023-05-23 3:51 ` Pedro Tammela
2023-05-23 14:04 ` Vlad Buslov
5 siblings, 2 replies; 13+ messages in thread
From: Peilin Ye @ 2023-05-22 23:55 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Peilin Ye, Daniel Borkmann, John Fastabend, Vlad Buslov,
Pedro Tammela, Hillf Danton, netdev, linux-kernel, Cong Wang,
Peilin Ye
mini_Qdisc_pair::p_miniq is a double pointer to mini_Qdisc, initialized in
ingress_init() to point to net_device::miniq_ingress. ingress Qdiscs
access this per-net_device pointer in mini_qdisc_pair_swap(). Similar for
clsact Qdiscs and miniq_egress.
Unfortunately, after introducing RTNL-unlocked RTM_{NEW,DEL,GET}TFILTER
requests (thanks Hillf Danton for the hint), when replacing ingress or
clsact Qdiscs, for example, the old Qdisc ("@old") could access the same
miniq_{in,e}gress pointer(s) concurrently with the new Qdisc ("@new"),
causing race conditions [1] including a use-after-free bug in
mini_qdisc_pair_swap() reported by syzbot:
BUG: KASAN: slab-use-after-free in mini_qdisc_pair_swap+0x1c2/0x1f0 net/sched/sch_generic.c:1573
Write of size 8 at addr ffff888045b31308 by task syz-executor690/14901
...
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd9/0x150 lib/dump_stack.c:106
print_address_description.constprop.0+0x2c/0x3c0 mm/kasan/report.c:319
print_report mm/kasan/report.c:430 [inline]
kasan_report+0x11c/0x130 mm/kasan/report.c:536
mini_qdisc_pair_swap+0x1c2/0x1f0 net/sched/sch_generic.c:1573
tcf_chain_head_change_item net/sched/cls_api.c:495 [inline]
tcf_chain0_head_change.isra.0+0xb9/0x120 net/sched/cls_api.c:509
tcf_chain_tp_insert net/sched/cls_api.c:1826 [inline]
tcf_chain_tp_insert_unique net/sched/cls_api.c:1875 [inline]
tc_new_tfilter+0x1de6/0x2290 net/sched/cls_api.c:2266
...
@old and @new should not affect each other. In other words, @old should
never modify miniq_{in,e}gress after @new, and @new should not update
@old's RCU state. Fixing without changing sch_api.c turned out to be
difficult (please refer to Closes: for discussions). Instead, make sure
@new's first call always happen after @old's last call, in
qdisc_destroy(), has finished:
In qdisc_graft(), return -EAGAIN and tell the caller to replay
(suggested by Vlad Buslov) if @old has any ongoing RTNL-unlocked filter
requests, and call qdisc_destroy() for @old before grafting @new.
Introduce qdisc_refcount_dec_if_one() as the counterpart of
qdisc_refcount_inc_nz() used for RTNL-unlocked filter requests. Introduce
a non-static version of qdisc_destroy() that does a TCQ_F_BUILTIN check,
just like qdisc_put() etc.
Depends on patch "net/sched: Refactor qdisc_graft() for ingress and clsact
Qdiscs".
[1] To illustrate, the syzkaller reproducer adds ingress Qdiscs under
TC_H_ROOT (no longer possible after patch "net/sched: sch_ingress: Only
create under TC_H_INGRESS") on eth0 that has 8 transmission queues:
Thread 1 creates ingress Qdisc A (containing mini Qdisc a1 and a2), then
adds a flower filter X to A.
Thread 2 creates another ingress Qdisc B (containing mini Qdisc b1 and
b2) to replace A, then adds a flower filter Y to B.
Thread 1 A's refcnt Thread 2
RTM_NEWQDISC (A, RTNL-locked)
qdisc_create(A) 1
qdisc_graft(A) 9
RTM_NEWTFILTER (X, RTNL-unlocked)
__tcf_qdisc_find(A) 10
tcf_chain0_head_change(A)
mini_qdisc_pair_swap(A) (1st)
|
| RTM_NEWQDISC (B, RTNL-locked)
RCU sync 2 qdisc_graft(B)
| 1 notify_and_destroy(A)
|
tcf_block_release(A) 0 RTM_NEWTFILTER (Y, RTNL-unlocked)
qdisc_destroy(A) tcf_chain0_head_change(B)
tcf_chain0_head_change_cb_del(A) mini_qdisc_pair_swap(B) (2nd)
mini_qdisc_pair_swap(A) (3rd) |
... ...
Here, B calls mini_qdisc_pair_swap(), pointing eth0->miniq_ingress to its
mini Qdisc, b1. Then, A calls mini_qdisc_pair_swap() again during
ingress_destroy(), setting eth0->miniq_ingress to NULL, so ingress packets
on eth0 will not find filter Y in sch_handle_ingress().
This is only one of the possible consequences of concurrently accessing
miniq_{in,e}gress pointers. The point is clear though: again, A should
never modify those per-net_device pointers after B, and B should not
update A's RCU state.
Fixes: 7a096d579e8e ("net: sched: ingress: set 'unlocked' flag for Qdisc ops")
Fixes: 87f373921c4e ("net: sched: ingress: set 'unlocked' flag for clsact Qdisc ops")
Reported-by: syzbot+b53a9c0d1ea4ad62da8b@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/r/0000000000006cf87705f79acf1a@google.com/
Cc: Hillf Danton <hdanton@sina.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
changes in v2:
- replay the request if the current Qdisc has any ongoing RTNL-unlocked
filter requests (Vlad)
- minor changes in code comments and commit log
include/net/sch_generic.h | 8 ++++++++
net/sched/sch_api.c | 32 ++++++++++++++++++++++++++------
net/sched/sch_generic.c | 14 +++++++++++---
3 files changed, 45 insertions(+), 9 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index fab5ba3e61b7..3e9cc43cbc90 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -137,6 +137,13 @@ static inline void qdisc_refcount_inc(struct Qdisc *qdisc)
refcount_inc(&qdisc->refcnt);
}
+static inline bool qdisc_refcount_dec_if_one(struct Qdisc *qdisc)
+{
+ if (qdisc->flags & TCQ_F_BUILTIN)
+ return true;
+ return refcount_dec_if_one(&qdisc->refcnt);
+}
+
/* Intended to be used by unlocked users, when concurrent qdisc release is
* possible.
*/
@@ -652,6 +659,7 @@ void dev_deactivate_many(struct list_head *head);
struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue,
struct Qdisc *qdisc);
void qdisc_reset(struct Qdisc *qdisc);
+void qdisc_destroy(struct Qdisc *qdisc);
void qdisc_put(struct Qdisc *qdisc);
void qdisc_put_unlocked(struct Qdisc *qdisc);
void qdisc_tree_reduce_backlog(struct Qdisc *qdisc, int n, int len);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f72a581666a2..b3bafa6c1b44 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1080,10 +1080,18 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
if ((q && q->flags & TCQ_F_INGRESS) ||
(new && new->flags & TCQ_F_INGRESS)) {
ingress = 1;
- if (!dev_ingress_queue(dev)) {
+ dev_queue = dev_ingress_queue(dev);
+ if (!dev_queue) {
NL_SET_ERR_MSG(extack, "Device does not have an ingress queue");
return -ENOENT;
}
+
+ /* Replay if the current ingress (or clsact) Qdisc has ongoing
+ * RTNL-unlocked filter request(s). This is the counterpart of that
+ * qdisc_refcount_inc_nz() call in __tcf_qdisc_find().
+ */
+ if (!qdisc_refcount_dec_if_one(dev_queue->qdisc_sleeping))
+ return -EAGAIN;
}
if (dev->flags & IFF_UP)
@@ -1104,8 +1112,16 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
qdisc_put(old);
}
} else {
- dev_queue = dev_ingress_queue(dev);
- old = dev_graft_qdisc(dev_queue, new);
+ old = dev_graft_qdisc(dev_queue, NULL);
+
+ /* {ingress,clsact}_destroy() @old before grafting @new to avoid
+ * unprotected concurrent accesses to net_device::miniq_{in,e}gress
+ * pointer(s) in mini_qdisc_pair_swap().
+ */
+ qdisc_notify(net, skb, n, classid, old, new, extack);
+ qdisc_destroy(old);
+
+ dev_graft_qdisc(dev_queue, new);
}
skip:
@@ -1119,8 +1135,6 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
if (new && new->ops->attach)
new->ops->attach(new);
- } else {
- notify_and_destroy(net, skb, n, classid, old, new, extack);
}
if (dev->flags & IFF_UP)
@@ -1458,6 +1472,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
struct Qdisc *p = NULL;
int err;
+replay:
err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
rtm_tca_policy, extack);
if (err < 0)
@@ -1515,8 +1530,11 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
return -ENOENT;
}
err = qdisc_graft(dev, p, skb, n, clid, NULL, q, extack);
- if (err != 0)
+ if (err != 0) {
+ if (err == -EAGAIN)
+ goto replay;
return err;
+ }
} else {
qdisc_notify(net, skb, n, clid, NULL, q, NULL);
}
@@ -1704,6 +1722,8 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
if (err) {
if (q)
qdisc_put(q);
+ if (err == -EAGAIN)
+ goto replay;
return err;
}
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 37e41f972f69..e14ed47f961c 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1046,7 +1046,7 @@ static void qdisc_free_cb(struct rcu_head *head)
qdisc_free(q);
}
-static void qdisc_destroy(struct Qdisc *qdisc)
+static void __qdisc_destroy(struct Qdisc *qdisc)
{
const struct Qdisc_ops *ops = qdisc->ops;
@@ -1070,6 +1070,14 @@ static void qdisc_destroy(struct Qdisc *qdisc)
call_rcu(&qdisc->rcu, qdisc_free_cb);
}
+void qdisc_destroy(struct Qdisc *qdisc)
+{
+ if (qdisc->flags & TCQ_F_BUILTIN)
+ return;
+
+ __qdisc_destroy(qdisc);
+}
+
void qdisc_put(struct Qdisc *qdisc)
{
if (!qdisc)
@@ -1079,7 +1087,7 @@ void qdisc_put(struct Qdisc *qdisc)
!refcount_dec_and_test(&qdisc->refcnt))
return;
- qdisc_destroy(qdisc);
+ __qdisc_destroy(qdisc);
}
EXPORT_SYMBOL(qdisc_put);
@@ -1094,7 +1102,7 @@ void qdisc_put_unlocked(struct Qdisc *qdisc)
!refcount_dec_and_rtnl_lock(&qdisc->refcnt))
return;
- qdisc_destroy(qdisc);
+ __qdisc_destroy(qdisc);
rtnl_unlock();
}
EXPORT_SYMBOL(qdisc_put_unlocked);
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting
2023-05-22 23:55 ` [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting Peilin Ye
@ 2023-05-23 3:51 ` Pedro Tammela
2023-05-23 4:40 ` Peilin Ye
2023-05-23 14:04 ` Vlad Buslov
1 sibling, 1 reply; 13+ messages in thread
From: Pedro Tammela @ 2023-05-23 3:51 UTC (permalink / raw)
To: Peilin Ye, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: Peilin Ye, Daniel Borkmann, John Fastabend, Hillf Danton, netdev,
linux-kernel, Cong Wang, Vlad Buslov
On 22/05/2023 20:55, Peilin Ye wrote:
> mini_Qdisc_pair::p_miniq is a double pointer to mini_Qdisc [...]
Hi Peilin!
With V2 patches 5 and 6 applied I was still able to trigger an oops.
Branch is 'net' + patches 5 & 6:
145f639b9403 (HEAD -> main) net/sched: qdisc_destroy() old ingress and
clsact Qdiscs before grafting
1aac74ef9673 net/sched: Refactor qdisc_graft() for ingress and clsact Qdiscs
18c40a1cc1d9 (origin/main, origin/HEAD) net/handshake: Fix sock->file
allocation
Kernel config is the same as in the syzbot report.
Note that this was on a _single core_ VM.
I will double check if v1 is triggering this issue (basically run the
repro for a long time). For multi-core my VM is running OOM even on a
32Gb system. I will check if we have a spare server to run the repro.
[ 695.782780][T12033]
==================================================================
[ 695.783617][T12033] BUG: KASAN: slab-use-after-free in
mini_qdisc_pair_swap+0x1c2/0x1f0
[ 695.784323][T12033] Write of size 8 at addr ffff888060cafb08 by task
repro/12033
[ 695.784996][T12033]
[ 695.785210][T12033] CPU: 0 PID: 12033 Comm: repro Not tainted
6.4.0-rc2-00187-g145f639b9403 #1
[ 695.785981][T12033] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.16.0-debian-1.16.0-5 04/01/2014
[ 695.786883][T12033] Call Trace:
[ 695.787178][T12033] <TASK>
[ 695.787444][T12033] dump_stack_lvl+0xd9/0x1b0
[ 695.787871][T12033] print_report+0xc4/0x5f0
[ 695.788283][T12033] ? __virt_addr_valid+0x5e/0x2d0
[ 695.788736][T12033] ? __phys_addr+0xc6/0x140
[ 695.789138][T12033] ? mini_qdisc_pair_swap+0x1c2/0x1f0
[ 695.789604][T12033] kasan_report+0xc0/0xf0
[ 695.789604][T12033] ? mini_qdisc_pair_swap+0x1c2/0x1f0
[ 695.789604][T12033] mini_qdisc_pair_swap+0x1c2/0x1f0
[ 695.789604][T12033] ? ingress_init+0x1c0/0x1c0
[ 695.789604][T12033] tcf_chain0_head_change.isra.0+0xb9/0x120
[ 695.789604][T12033] tc_new_tfilter+0x1ebb/0x22b0
[ 695.789604][T12033] ? tc_del_tfilter+0x1570/0x1570
[ 695.789604][T12033] ? lockdep_hardirqs_on_prepare+0x410/0x410
[ 695.789604][T12033] ? kasan_quarantine_put+0x102/0x230
[ 695.789604][T12033] ? lockdep_hardirqs_on+0x7d/0x100
[ 695.789604][T12033] ? rtnetlink_rcv_msg+0x94a/0xd30
[ 695.789604][T12033] ? reacquire_held_locks+0x4b0/0x4b0
[ 695.789604][T12033] ? bpf_lsm_capable+0x9/0x10
[ 695.789604][T12033] ? tc_del_tfilter+0x1570/0x1570
[ 695.789604][T12033] rtnetlink_rcv_msg+0x98a/0xd30
[ 695.789604][T12033] ? rtnl_getlink+0xb10/0xb10
[ 695.789604][T12033] ? reacquire_held_locks+0x4b0/0x4b0
[ 695.789604][T12033] ? netdev_core_pick_tx+0x390/0x390
[ 695.789604][T12033] netlink_rcv_skb+0x166/0x440
[ 695.789604][T12033] ? rtnl_getlink+0xb10/0xb10
[ 695.789604][T12033] ? netlink_ack+0x1370/0x1370
[ 695.789604][T12033] ? kasan_set_track+0x25/0x30
[ 695.789604][T12033] ? netlink_deliver_tap+0x1b1/0xd00
[ 695.789604][T12033] netlink_unicast+0x530/0x800
[ 695.789604][T12033] ? netlink_attachskb+0x880/0x880
[ 695.789604][T12033] ? __sanitizer_cov_trace_switch+0x54/0x90
[ 695.789604][T12033] ? __phys_addr_symbol+0x30/0x70
[ 695.789604][T12033] ? __check_object_size+0x323/0x740
[ 695.789604][T12033] netlink_sendmsg+0x90b/0xe10
[ 695.789604][T12033] ? netlink_unicast+0x800/0x800
[ 695.789604][T12033] ? bpf_lsm_socket_sendmsg+0x9/0x10
[ 695.789604][T12033] ? netlink_unicast+0x800/0x800
[ 695.789604][T12033] sock_sendmsg+0xd9/0x180
[ 695.789604][T12033] ____sys_sendmsg+0x264/0x910
[ 695.789604][T12033] ? kernel_sendmsg+0x50/0x50
[ 695.789604][T12033] ? __copy_msghdr+0x460/0x460
[ 695.789604][T12033] ___sys_sendmsg+0x11d/0x1b0
[ 695.789604][T12033] ? do_recvmmsg+0x700/0x700
[ 695.789604][T12033] ? find_held_lock+0x2d/0x110
[ 695.789604][T12033] ? __might_fault+0xe5/0x190
[ 695.789604][T12033] ? reacquire_held_locks+0x4b0/0x4b0
[ 695.789604][T12033] __sys_sendmmsg+0x18e/0x430
[ 695.789604][T12033] ? __ia32_sys_sendmsg+0xb0/0xb0
[ 695.789604][T12033] ? reacquire_held_locks+0x4b0/0x4b0
[ 695.789604][T12033] ? rcu_is_watching+0x12/0xb0
[ 695.789604][T12033] ? xfd_validate_state+0x5d/0x180
[ 695.789604][T12033] ? restore_fpregs_from_fpstate+0xc1/0x1d0
[ 695.789604][T12033] ? unlock_page_memcg+0x2d0/0x2d0
[ 695.789604][T12033] ? do_futex+0x350/0x350
[ 695.789604][T12033] __x64_sys_sendmmsg+0x9c/0x100
[ 695.789604][T12033] ? syscall_enter_from_user_mode+0x26/0x80
[ 695.789604][T12033] do_syscall_64+0x38/0xb0
[ 695.789604][T12033] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 695.789604][T12033] RIP: 0033:0x7f4aca44a89d
[ 695.789604][T12033] Code: 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3
0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c
24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 4b 05 0e 00 f7 d8 64
89 01 48
[ 695.789604][T12033] RSP: 002b:00007f4aca2eec68 EFLAGS: 00000203
ORIG_RAX: 0000000000000133
[ 695.789604][T12033] RAX: ffffffffffffffda RBX: 00007f4aca2efcdc RCX:
00007f4aca44a89d
[ 695.789604][T12033] RDX: 040000000000009f RSI: 00000000200002c0 RDI:
0000000000000007
[ 695.789604][T12033] RBP: 00007f4aca2eede0 R08: 0000000000000000 R09:
0000000000000000
[ 695.789604][T12033] R10: 0000000000000000 R11: 0000000000000203 R12:
fffffffffffffeb8
[ 695.789604][T12033] R13: 000000000000006e R14: 00007ffd1a53f720 R15:
00007f4aca2cf000
[ 695.789604][T12033] </TASK>
[ 695.789604][T12033]
[ 695.789604][T12033] Allocated by task 12031:
[ 695.789604][T12033] kasan_save_stack+0x20/0x40
[ 695.789604][T12033] kasan_set_track+0x25/0x30
[ 695.789604][T12033] __kasan_kmalloc+0xa2/0xb0
[ 695.789604][T12033] __kmalloc_node+0x60/0x100
[ 695.789604][T12033] qdisc_alloc+0xb3/0xa90
[ 695.789604][T12033] qdisc_create+0xcf/0x1020
[ 695.789604][T12033] tc_modify_qdisc+0x495/0x1ab0
[ 695.789604][T12033] rtnetlink_rcv_msg+0x439/0xd30
[ 695.789604][T12033] netlink_rcv_skb+0x166/0x440
[ 695.789604][T12033] netlink_unicast+0x530/0x800
[ 695.789604][T12033] netlink_sendmsg+0x90b/0xe10
[ 695.789604][T12033] sock_sendmsg+0xd9/0x180
[ 695.789604][T12033] ____sys_sendmsg+0x264/0x910
[ 695.789604][T12033] ___sys_sendmsg+0x11d/0x1b0
[ 695.789604][T12033] __sys_sendmmsg+0x18e/0x430
[ 695.789604][T12033] __x64_sys_sendmmsg+0x9c/0x100
[ 695.789604][T12033] do_syscall_64+0x38/0xb0
[ 695.789604][T12033] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 695.789604][T12033]
[ 695.789604][T12033] Freed by task 15:
[ 695.789604][T12033] kasan_save_stack+0x20/0x40
[ 695.789604][T12033] kasan_set_track+0x25/0x30
[ 695.789604][T12033] kasan_save_free_info+0x2e/0x40
[ 695.789604][T12033] ____kasan_slab_free+0x15e/0x1b0
[ 695.789604][T12033] slab_free_freelist_hook+0x10b/0x1e0
[ 695.789604][T12033] __kmem_cache_free+0xaf/0x2e0
[ 695.789604][T12033] rcu_core+0x7f7/0x1ac0
[ 695.789604][T12033] __do_softirq+0x1d8/0x8fd
[ 695.789604][T12033]
[ 695.789604][T12033] Last potentially related work creation:
[ 695.789604][T12033] kasan_save_stack+0x20/0x40
[ 695.789604][T12033] __kasan_record_aux_stack+0xbf/0xd0
[ 695.789604][T12033] __call_rcu_common.constprop.0+0x9a/0x790
[ 695.789604][T12033] qdisc_put_unlocked+0x74/0x90
[ 695.789604][T12033] tcf_block_release+0x90/0xa0
[ 695.789604][T12033] tc_new_tfilter+0xa5e/0x22b0
[ 695.789604][T12033] rtnetlink_rcv_msg+0x98a/0xd30
[ 695.789604][T12033] netlink_rcv_skb+0x166/0x440
[ 695.789604][T12033] netlink_unicast+0x530/0x800
[ 695.789604][T12033] netlink_sendmsg+0x90b/0xe10
[ 695.789604][T12033] sock_sendmsg+0xd9/0x180
[ 695.789604][T12033] ____sys_sendmsg+0x264/0x910
[ 695.789604][T12033] ___sys_sendmsg+0x11d/0x1b0
[ 695.789604][T12033] __sys_sendmmsg+0x18e/0x430
[ 695.789604][T12033] __x64_sys_sendmmsg+0x9c/0x100
[ 695.789604][T12033] do_syscall_64+0x38/0xb0
[ 695.789604][T12033] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 695.789604][T12033]
[ 695.789604][T12033] Second to last potentially related work creation:
[ 695.789604][T12033] kasan_save_stack+0x20/0x40
[ 695.789604][T12033] __kasan_record_aux_stack+0xbf/0xd0
[ 695.789604][T12033] __call_rcu_common.constprop.0+0x9a/0x790
[ 695.789604][T12033] rht_deferred_worker+0x10fd/0x2010
[ 695.789604][T12033] process_one_work+0x9f9/0x15f0
[ 695.789604][T12033] worker_thread+0x687/0x1110
[ 695.789604][T12033] kthread+0x334/0x430
[ 695.789604][T12033] ret_from_fork+0x1f/0x30
[ 695.789604][T12033]
[ 695.789604][T12033] The buggy address belongs to the object at
ffff888060caf800
[ 695.789604][T12033] which belongs to the cache kmalloc-1k of size 1024
[ 695.789604][T12033] The buggy address is located 776 bytes inside of
[ 695.789604][T12033] freed 1024-byte region [ffff888060caf800,
ffff888060cafc00)
[ 695.789604][T12033]
[ 695.789604][T12033] The buggy address belongs to the physical page:
[ 695.789604][T12033] page:ffffea0001832b00 refcount:1 mapcount:0
mapping:0000000000000000 index:0x0 pfn:0x60cac
[ 695.789604][T12033] head:ffffea0001832b00 order:2 entire_mapcount:0
nr_pages_mapped:0 pincount:0
[ 695.789604][T12033] flags:
0xfff00000010200(slab|head|node=0|zone=1|lastcpupid=0x7ff)
[ 695.789604][T12033] page_type: 0xffffffff()
[ 695.789604][T12033] raw: 00fff00000010200 ffff888012441dc0
ffffea0000bac600 dead000000000002
[ 695.789604][T12033] raw: 0000000000000000 0000000000080008
00000001ffffffff 0000000000000000
[ 695.789604][T12033] page dumped because: kasan: bad access detected
[ 695.789604][T12033] page_owner tracks the page as allocated
[ 695.789604][T12033] page last allocated via order 2, migratetype
Unmovable, gfp_mask
0xd20c0(__GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC),
pid 13324, tgid 13318 (repro), ts 170262603079, free_ts 0
[ 695.789604][T12033] get_page_from_freelist+0xe71/0x2e80
[ 695.789604][T12033] __alloc_pages+0x1c8/0x4a0
[ 695.789604][T12033] alloc_pages+0x1a9/0x270
[ 695.789604][T12033] allocate_slab+0x24e/0x380
[ 695.789604][T12033] ___slab_alloc+0x89a/0x1400
[ 695.789604][T12033] __slab_alloc.constprop.0+0x56/0xa0
[ 695.789604][T12033] __kmem_cache_alloc_node+0x126/0x330
[ 695.789604][T12033] kmalloc_trace+0x25/0xe0
[ 695.789604][T12033] fl_change+0x1b3/0x51e0
[ 695.789604][T12033] tc_new_tfilter+0x992/0x22b0
[ 695.789604][T12033] rtnetlink_rcv_msg+0x98a/0xd30
[ 695.789604][T12033] netlink_rcv_skb+0x166/0x440
[ 695.789604][T12033] netlink_unicast+0x530/0x800
[ 695.789604][T12033] netlink_sendmsg+0x90b/0xe10
[ 695.789604][T12033] sock_sendmsg+0xd9/0x180
[ 695.789604][T12033] ____sys_sendmsg+0x264/0x910
[ 695.789604][T12033] page_owner free stack trace missing
[ 695.789604][T12033]
[ 695.789604][T12033] Memory state around the buggy address:
[ 695.789604][T12033] ffff888060cafa00: fb fb fb fb fb fb fb fb fb fb
fb fb fb fb fb fb
[ 695.789604][T12033] ffff888060cafa80: fb fb fb fb fb fb fb fb fb fb
fb fb fb fb fb fb
[ 695.789604][T12033] >ffff888060cafb00: fb fb fb fb fb fb fb fb fb fb
fb fb fb fb fb fb
[ 695.789604][T12033] ^
[ 695.789604][T12033] ffff888060cafb80: fb fb fb fb fb fb fb fb fb fb
fb fb fb fb fb fb
[ 695.789604][T12033] ffff888060cafc00: fc fc fc fc fc fc fc fc fc fc
fc fc fc fc fc fc
[ 695.789604][T12033]
==================================================================
[ 695.996261][T12042] __nla_validate_parse: 32 callbacks suppressed
[ 695.996271][T12042] netlink: 24 bytes leftover after parsing
attributes in process `repro'.
[ 696.473670][T12046] netlink: 24 bytes leftover after parsing
attributes in process `repro'.
[ 696.660496][T12033] Kernel panic - not syncing: KASAN: panic_on_warn
set ...
[ 696.661250][T12033] CPU: 0 PID: 12033 Comm: repro Not tainted
6.4.0-rc2-00187-g145f639b9403 #1
[ 696.661947][T12033] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.16.0-debian-1.16.0-5 04/01/2014
[ 696.662768][T12033] Call Trace:
[ 696.663031][T12033] <TASK>
[ 696.663268][T12033] dump_stack_lvl+0xd9/0x1b0
[ 696.663659][T12033] panic+0x689/0x730
[ 696.663977][T12033] ? panic_smp_self_stop+0xa0/0xa0
[ 696.664396][T12033] ? preempt_schedule_thunk+0x1a/0x20
[ 696.664829][T12033] ? preempt_schedule_common+0x45/0xc0
[ 696.665263][T12033] ? mini_qdisc_pair_swap+0x1c2/0x1f0
[ 696.665698][T12033] check_panic_on_warn+0xab/0xb0
[ 696.666087][T12033] ? mini_qdisc_pair_swap+0x1c2/0x1f0
[ 696.666519][T12033] end_report+0xe9/0x120
[ 696.666861][T12033] kasan_report+0xcd/0xf0
[ 696.667209][T12033] ? mini_qdisc_pair_swap+0x1c2/0x1f0
[ 696.667639][T12033] mini_qdisc_pair_swap+0x1c2/0x1f0
[ 696.668064][T12033] ? ingress_init+0x1c0/0x1c0
[ 696.668451][T12033] tcf_chain0_head_change.isra.0+0xb9/0x120
[ 696.668964][T12033] tc_new_tfilter+0x1ebb/0x22b0
[ 696.669391][T12033] ? tc_del_tfilter+0x1570/0x1570
[ 696.669608][T12033] ? lockdep_hardirqs_on_prepare+0x410/0x410
[ 696.669608][T12033] ? kasan_quarantine_put+0x102/0x230
[ 696.669608][T12033] ? lockdep_hardirqs_on+0x7d/0x100
[ 696.669608][T12033] ? rtnetlink_rcv_msg+0x94a/0xd30
[ 696.669608][T12033] ? reacquire_held_locks+0x4b0/0x4b0
[ 696.669608][T12033] ? bpf_lsm_capable+0x9/0x10
[ 696.669608][T12033] ? tc_del_tfilter+0x1570/0x1570
[ 696.669608][T12033] rtnetlink_rcv_msg+0x98a/0xd30
[ 696.669608][T12033] ? rtnl_getlink+0xb10/0xb10
[ 696.669608][T12033] ? reacquire_held_locks+0x4b0/0x4b0
[ 696.669608][T12033] ? netdev_core_pick_tx+0x390/0x390
[ 696.669608][T12033] netlink_rcv_skb+0x166/0x440
[ 696.669608][T12033] ? rtnl_getlink+0xb10/0xb10
[ 696.669608][T12033] ? netlink_ack+0x1370/0x1370
[ 696.669608][T12033] ? kasan_set_track+0x25/0x30
[ 696.669608][T12033] ? netlink_deliver_tap+0x1b1/0xd00
[ 696.669608][T12033] netlink_unicast+0x530/0x800
[ 696.669608][T12033] ? netlink_attachskb+0x880/0x880
[ 696.669608][T12033] ? __sanitizer_cov_trace_switch+0x54/0x90
[ 696.669608][T12033] ? __phys_addr_symbol+0x30/0x70
[ 696.669608][T12033] ? __check_object_size+0x323/0x740
[ 696.669608][T12033] netlink_sendmsg+0x90b/0xe10
[ 696.669608][T12033] ? netlink_unicast+0x800/0x800
[ 696.669608][T12033] ? bpf_lsm_socket_sendmsg+0x9/0x10
[ 696.669608][T12033] ? netlink_unicast+0x800/0x800
[ 696.669608][T12033] sock_sendmsg+0xd9/0x180
[ 696.669608][T12033] ____sys_sendmsg+0x264/0x910
[ 696.669608][T12033] ? kernel_sendmsg+0x50/0x50
[ 696.669608][T12033] ? __copy_msghdr+0x460/0x460
[ 696.669608][T12033] ___sys_sendmsg+0x11d/0x1b0
[ 696.669608][T12033] ? do_recvmmsg+0x700/0x700
[ 696.669608][T12033] ? find_held_lock+0x2d/0x110
[ 696.669608][T12033] ? __might_fault+0xe5/0x190
[ 696.669608][T12033] ? reacquire_held_locks+0x4b0/0x4b0
[ 696.669608][T12033] __sys_sendmmsg+0x18e/0x430
[ 696.669608][T12033] ? __ia32_sys_sendmsg+0xb0/0xb0
[ 696.669608][T12033] ? reacquire_held_locks+0x4b0/0x4b0
[ 696.669608][T12033] ? rcu_is_watching+0x12/0xb0
[ 696.669608][T12033] ? xfd_validate_state+0x5d/0x180
[ 696.669608][T12033] ? restore_fpregs_from_fpstate+0xc1/0x1d0
[ 696.669608][T12033] ? unlock_page_memcg+0x2d0/0x2d0
[ 696.669608][T12033] ? do_futex+0x350/0x350
[ 696.669608][T12033] __x64_sys_sendmmsg+0x9c/0x100
[ 696.669608][T12033] ? syscall_enter_from_user_mode+0x26/0x80
[ 696.669608][T12033] do_syscall_64+0x38/0xb0
[ 696.669608][T12033] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 696.669608][T12033] RIP: 0033:0x7f4aca44a89d
[ 696.669608][T12033] Code: 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3
0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c
24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 4b 05 0e 00 f7 d8 64
89 01 48
[ 696.669608][T12033] RSP: 002b:00007f4aca2eec68 EFLAGS: 00000203
ORIG_RAX: 0000000000000133
[ 696.669608][T12033] RAX: ffffffffffffffda RBX: 00007f4aca2efcdc RCX:
00007f4aca44a89d
[ 696.669608][T12033] RDX: 040000000000009f RSI: 00000000200002c0 RDI:
0000000000000007
[ 696.669608][T12033] RBP: 00007f4aca2eede0 R08: 0000000000000000 R09:
0000000000000000
[ 696.669608][T12033] R10: 0000000000000000 R11: 0000000000000203 R12:
fffffffffffffeb8
[ 696.669608][T12033] R13: 000000000000006e R14: 00007ffd1a53f720 R15:
00007f4aca2cf000
[ 696.669608][T12033] </TASK>
[ 696.669608][T12033] Kernel Offset: disabled
[ 696.669608][T12033] Rebooting in 86400 seconds..
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting
2023-05-23 3:51 ` Pedro Tammela
@ 2023-05-23 4:40 ` Peilin Ye
2023-05-23 11:36 ` Pedro Tammela
0 siblings, 1 reply; 13+ messages in thread
From: Peilin Ye @ 2023-05-23 4:40 UTC (permalink / raw)
To: Pedro Tammela
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko, Peilin Ye,
Daniel Borkmann, John Fastabend, Hillf Danton, netdev,
linux-kernel, Cong Wang, Vlad Buslov
Hi Pedro,
On Tue, May 23, 2023 at 12:51:44AM -0300, Pedro Tammela wrote:
> With V2 patches 5 and 6 applied I was still able to trigger an oops.
>
> Branch is 'net' + patches 5 & 6:
> 145f639b9403 (HEAD -> main) net/sched: qdisc_destroy() old ingress and
> clsact Qdiscs before grafting
> 1aac74ef9673 net/sched: Refactor qdisc_graft() for ingress and clsact Qdiscs
> 18c40a1cc1d9 (origin/main, origin/HEAD) net/handshake: Fix sock->file
> allocation
>
> Kernel config is the same as in the syzbot report.
> Note that this was on a _single core_ VM.
> I will double check if v1 is triggering this issue (basically run the repro
> for a long time). For multi-core my VM is running OOM even on a 32Gb system.
> I will check if we have a spare server to run the repro.
Thanks for testing this, but the syzbot reproducer creates ingress Qdiscs
under TC_H_ROOT, which isn't covered by [6/6] i.e. it exercises the
"!ingress" path in qdisc_graft(). I think that's why you are still seeing
the oops. Adding sch_{ingress,clsact} to TC_H_ROOT is no longer possible
after [1,2/6], and I think we'll need a different reproducer for [5,6/6].
However I just noticed that for some reason my git-send-email in my new
setup didn't auto-generate From: tags with my work email, so Author: will
be my personal email (I have to send patches from personal email to avoid
"[External] " subject prefixes) ... I will fix it in v3 soon. Sorry in
advance for spamming.
Thanks,
Peilin Ye
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting
2023-05-23 4:40 ` Peilin Ye
@ 2023-05-23 11:36 ` Pedro Tammela
2023-05-23 20:06 ` Peilin Ye
0 siblings, 1 reply; 13+ messages in thread
From: Pedro Tammela @ 2023-05-23 11:36 UTC (permalink / raw)
To: Peilin Ye
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko, Peilin Ye,
Daniel Borkmann, John Fastabend, Hillf Danton, netdev,
linux-kernel, Cong Wang, Vlad Buslov
On 23/05/2023 01:40, Peilin Ye wrote:
> Hi Pedro,
>
> On Tue, May 23, 2023 at 12:51:44AM -0300, Pedro Tammela wrote:
>> With V2 patches 5 and 6 applied I was still able to trigger an oops.
>>
>> Branch is 'net' + patches 5 & 6:
>> 145f639b9403 (HEAD -> main) net/sched: qdisc_destroy() old ingress and
>> clsact Qdiscs before grafting
>> 1aac74ef9673 net/sched: Refactor qdisc_graft() for ingress and clsact Qdiscs
>> 18c40a1cc1d9 (origin/main, origin/HEAD) net/handshake: Fix sock->file
>> allocation
>>
>> Kernel config is the same as in the syzbot report.
>> Note that this was on a _single core_ VM.
>> I will double check if v1 is triggering this issue (basically run the repro
>> for a long time). For multi-core my VM is running OOM even on a 32Gb system.
>> I will check if we have a spare server to run the repro.
>
> Thanks for testing this, but the syzbot reproducer creates ingress Qdiscs
> under TC_H_ROOT, which isn't covered by [6/6] i.e. it exercises the
> "!ingress" path in qdisc_graft(). I think that's why you are still seeing
> the oops. Adding sch_{ingress,clsact} to TC_H_ROOT is no longer possible
> after [1,2/6], and I think we'll need a different reproducer for [5,6/6].
>
> However I just noticed that for some reason my git-send-email in my new
> setup didn't auto-generate From: tags with my work email, so Author: will
> be my personal email (I have to send patches from personal email to avoid
> "[External] " subject prefixes) ... I will fix it in v3 soon. Sorry in
> advance for spamming.
>
> Thanks,
> Peilin Ye
>
I see,
We need to make sure then, when the time comes, that all the required
patches are back ported in the same bundle so we don't have a partial
fix; given that they target different commit tags.
I was still able to trigger an oops with the full patchset:
[ 104.944353][ T6588] ------------[ cut here ]------------
[ 104.944896][ T6588] jump label: negative count!
[ 104.945780][ T6588] WARNING: CPU: 0 PID: 6588 at
kernel/jump_label.c:263 static_key_slow_try_dec+0xf2/0x110
[ 104.946795][ T6588] Modules linked in:
[ 104.947111][ T6588] CPU: 0 PID: 6588 Comm: repro Not tainted
6.4.0-rc2-00191-g4a3f9100193d #3
[ 104.947765][ T6588] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.16.0-debian-1.16.0-5 04/01/2014
[ 104.948557][ T6588] RIP: 0010:static_key_slow_try_dec+0xf2/0x110
[ 104.949064][ T6588] Code: d5 ff e8 c1 33 d5 ff 44 89 e8 5b 5d 41 5c
41 5d c3 44 89 e5 e9 66 ff ff ff e8 aa 33 d5 ff 48 c7 c7 00 9c 56 8a e8
4e ce 9c ff <0f> 0b eb ae 48 89 df e8 02 4b 28 00 e9 42 ff ff ff 66 66
2e 0f 1f
[ 104.951134][ T6588] RSP: 0018:ffffc900079cf2c0 EFLAGS: 00010286
[ 104.951646][ T6588] RAX: 0000000000000000 RBX: ffffffff9213a160 RCX:
0000000000000000
[ 104.952269][ T6588] RDX: ffff888112f83b80 RSI: ffffffff814c7747 RDI:
0000000000000001
[ 104.952901][ T6588] RBP: 00000000ffffffff R08: 0000000000000001 R09:
0000000000000000
[ 104.953523][ T6588] R10: 0000000000000001 R11: 0000000000000001 R12:
00000000ffffffff
[ 104.954133][ T6588] R13: ffff88816a514001 R14: 0000000000000001 R15:
ffffffff8e7b0680
[ 104.954746][ T6588] FS: 00007f76c65d56c0(0000)
GS:ffff8881f5a00000(0000) knlGS:0000000000000000
[ 104.955430][ T6588] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 104.955941][ T6588] CR2: 00007f9a40357a50 CR3: 000000011461e000 CR4:
0000000000350ef0
[ 104.956559][ T6588] Call Trace:
[ 104.956829][ T6588] <TASK>
[ 104.957062][ T6588] ? clsact_egress_block_get+0x40/0x40
[ 104.957507][ T6588] static_key_slow_dec+0x60/0xc0
[ 104.957906][ T6588] qdisc_create+0xa45/0x1090
[ 104.958274][ T6588] ? tc_get_qdisc+0xb70/0xb70
[ 104.958646][ T6588] tc_modify_qdisc+0x491/0x1b70
[ 104.959031][ T6588] ? qdisc_create+0x1090/0x1090
[ 104.959420][ T6588] ? bpf_lsm_capable+0x9/0x10
[ 104.959797][ T6588] ? qdisc_create+0x1090/0x1090
[ 104.960175][ T6588] rtnetlink_rcv_msg+0x439/0xd30
[ 104.960625][ T6588] ? rtnl_getlink+0xb10/0xb10
[ 104.960995][ T6588] ? __x64_sys_sendmmsg+0x9c/0x100
[ 104.961397][ T6588] ? do_syscall_64+0x38/0xb0
[ 104.961767][ T6588] ? netdev_core_pick_tx+0x390/0x390
[ 104.962195][ T6588] netlink_rcv_skb+0x166/0x440
[ 104.962584][ T6588] ? rtnl_getlink+0xb10/0xb10
[ 104.962954][ T6588] ? netlink_ack+0x1370/0x1370
[ 104.963336][ T6588] ? kasan_set_track+0x25/0x30
[ 104.963731][ T6588] ? netlink_deliver_tap+0x1b1/0xd00
[ 104.964156][ T6588] netlink_unicast+0x530/0x800
[ 104.964537][ T6588] ? netlink_attachskb+0x880/0x880
[ 104.964951][ T6588] ? __sanitizer_cov_trace_switch+0x54/0x90
[ 104.965405][ T6588] ? __phys_addr_symbol+0x30/0x70
[ 104.965793][ T6588] ? __check_object_size+0x323/0x740
[ 104.966205][ T6588] netlink_sendmsg+0x90b/0xe10
[ 104.966577][ T6588] ? netlink_unicast+0x800/0x800
[ 104.966965][ T6588] ? bpf_lsm_socket_sendmsg+0x9/0x10
[ 104.967374][ T6588] ? netlink_unicast+0x800/0x800
[ 104.967761][ T6588] sock_sendmsg+0xd9/0x180
[ 104.968109][ T6588] ____sys_sendmsg+0x264/0x910
[ 104.968490][ T6588] ? kernel_sendmsg+0x50/0x50
[ 104.968871][ T6588] ? __copy_msghdr+0x460/0x460
[ 104.969244][ T6588] ? lockdep_hardirqs_on_prepare+0x410/0x410
[ 104.969716][ T6588] ? find_held_lock+0x2d/0x110
[ 104.970088][ T6588] ___sys_sendmsg+0x11d/0x1b0
[ 104.970491][ T6588] ? do_recvmmsg+0x700/0x700
[ 104.970853][ T6588] ? __fget_files+0x260/0x420
[ 104.971221][ T6588] ? reacquire_held_locks+0x4b0/0x4b0
[ 104.971649][ T6588] ? __fget_files+0x282/0x420
[ 104.972018][ T6588] ? __fget_light+0xe6/0x270
[ 104.972384][ T6588] __sys_sendmmsg+0x18e/0x430
[ 104.972766][ T6588] ? __ia32_sys_sendmsg+0xb0/0xb0
[ 104.973167][ T6588] ? reacquire_held_locks+0x4b0/0x4b0
[ 104.973588][ T6588] ? rcu_is_watching+0x12/0xb0
[ 104.973958][ T6588] ? xfd_validate_state+0x5d/0x180
[ 104.974355][ T6588] ? restore_fpregs_from_fpstate+0xc1/0x1d0
[ 104.974796][ T6588] ? unlock_page_memcg+0x2d0/0x2d0
[ 104.975178][ T6588] ? do_futex+0x350/0x350
[ 104.975501][ T6588] __x64_sys_sendmmsg+0x9c/0x100
[ 104.975867][ T6588] ? syscall_enter_from_user_mode+0x26/0x80
[ 104.976307][ T6588] do_syscall_64+0x38/0xb0
[ 104.976649][ T6588] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 104.977088][ T6588] RIP: 0033:0x7f76c66ee89d
[ 104.977417][ T6588] Code: 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3
0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c
24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 4b 05 0e 00 f7 d8 64
89 01 48
[ 104.978835][ T6588] RSP: 002b:00007f76c65d4c68 EFLAGS: 00000203
ORIG_RAX: 0000000000000133
[ 104.979468][ T6588] RAX: ffffffffffffffda RBX: 00007f76c65d5cdc RCX:
00007f76c66ee89d
[ 104.980073][ T6588] RDX: 040000000000009f RSI: 00000000200002c0 RDI:
0000000000000007
[ 104.980739][ T6588] RBP: 00007f76c65d4de0 R08: 0000000100000001 R09:
0000000000000000
[ 104.981352][ T6588] R10: 0000000000000000 R11: 0000000000000203 R12:
fffffffffffffeb8
[ 104.981968][ T6588] R13: 0000000000000002 R14: 00007ffce5a5fd90 R15:
00007f76c65b5000
[ 104.982587][ T6588] </TASK>
[ 104.982833][ T6588] Kernel panic - not syncing: kernel: panic_on_warn
set ...
[ 104.983391][ T6588] CPU: 0 PID: 6588 Comm: repro Not tainted
6.4.0-rc2-00191-g4a3f9100193d #3
[ 104.984054][ T6588] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.16.0-debian-1.16.0-5 04/01/2014
[ 104.984854][ T6588] Call Trace:
[ 104.985121][ T6588] <TASK>
[ 104.985359][ T6588] dump_stack_lvl+0xd9/0x1b0
[ 104.985733][ T6588] panic+0x689/0x730
[ 104.986053][ T6588] ? panic_smp_self_stop+0xa0/0xa0
[ 104.986467][ T6588] ? show_trace_log_lvl+0x28e/0x400
[ 104.986888][ T6588] ? static_key_slow_try_dec+0xf2/0x110
[ 104.987329][ T6588] check_panic_on_warn+0xab/0xb0
[ 104.987729][ T6588] __warn+0xf2/0x380
[ 104.988046][ T6588] ? static_key_slow_try_dec+0xf2/0x110
[ 104.988476][ T6588] report_bug+0x3bc/0x580
[ 104.988838][ T6588] handle_bug+0x3c/0x70
[ 104.989169][ T6588] exc_invalid_op+0x17/0x40
[ 104.989529][ T6588] asm_exc_invalid_op+0x1a/0x20
[ 104.989914][ T6588] RIP: 0010:static_key_slow_try_dec+0xf2/0x110
[ 104.990402][ T6588] Code: d5 ff e8 c1 33 d5 ff 44 89 e8 5b 5d 41 5c
41 5d c3 44 89 e5 e9 66 ff ff ff e8 aa 33 d5 ff 48 c7 c7 00 9c 56 8a e8
4e ce 9c ff <0f> 0b eb ae 48 89 df e8 02 4b 28 00 e9 42 ff ff ff 66 66
2e 0f 1f
[ 104.990702][ T6588] RSP: 0018:ffffc900079cf2c0 EFLAGS: 00010286
[ 104.990702][ T6588] RAX: 0000000000000000 RBX: ffffffff9213a160 RCX:
0000000000000000
[ 104.990702][ T6588] RDX: ffff888112f83b80 RSI: ffffffff814c7747 RDI:
0000000000000001
[ 104.990702][ T6588] RBP: 00000000ffffffff R08: 0000000000000001 R09:
0000000000000000
[ 104.990702][ T6588] R10: 0000000000000001 R11: 0000000000000001 R12:
00000000ffffffff
[ 104.990702][ T6588] R13: ffff88816a514001 R14: 0000000000000001 R15:
ffffffff8e7b0680
[ 104.990702][ T6588] ? __warn_printk+0x187/0x310
[ 104.990702][ T6588] ? clsact_egress_block_get+0x40/0x40
[ 104.990702][ T6588] static_key_slow_dec+0x60/0xc0
[ 104.990702][ T6588] qdisc_create+0xa45/0x1090
[ 104.990702][ T6588] ? tc_get_qdisc+0xb70/0xb70
[ 104.990702][ T6588] tc_modify_qdisc+0x491/0x1b70
[ 104.990702][ T6588] ? qdisc_create+0x1090/0x1090
[ 104.990702][ T6588] ? bpf_lsm_capable+0x9/0x10
[ 104.990702][ T6588] ? qdisc_create+0x1090/0x1090
[ 104.990702][ T6588] rtnetlink_rcv_msg+0x439/0xd30
[ 104.990702][ T6588] ? rtnl_getlink+0xb10/0xb10
[ 104.990702][ T6588] ? __x64_sys_sendmmsg+0x9c/0x100
[ 104.990702][ T6588] ? do_syscall_64+0x38/0xb0
[ 104.990702][ T6588] ? netdev_core_pick_tx+0x390/0x390
[ 104.990702][ T6588] netlink_rcv_skb+0x166/0x440
[ 104.990702][ T6588] ? rtnl_getlink+0xb10/0xb10
[ 104.990702][ T6588] ? netlink_ack+0x1370/0x1370
[ 104.990702][ T6588] ? kasan_set_track+0x25/0x30
[ 104.990702][ T6588] ? netlink_deliver_tap+0x1b1/0xd00
[ 104.990702][ T6588] netlink_unicast+0x530/0x800
[ 104.990702][ T6588] ? netlink_attachskb+0x880/0x880
[ 104.990702][ T6588] ? __sanitizer_cov_trace_switch+0x54/0x90
[ 104.990702][ T6588] ? __phys_addr_symbol+0x30/0x70
[ 104.990702][ T6588] ? __check_object_size+0x323/0x740
[ 104.990702][ T6588] netlink_sendmsg+0x90b/0xe10
[ 104.990702][ T6588] ? netlink_unicast+0x800/0x800
[ 104.990702][ T6588] ? bpf_lsm_socket_sendmsg+0x9/0x10
[ 104.990702][ T6588] ? netlink_unicast+0x800/0x800
[ 104.990702][ T6588] sock_sendmsg+0xd9/0x180
[ 104.990702][ T6588] ____sys_sendmsg+0x264/0x910
[ 104.990702][ T6588] ? kernel_sendmsg+0x50/0x50
[ 104.990702][ T6588] ? __copy_msghdr+0x460/0x460
[ 104.990702][ T6588] ? lockdep_hardirqs_on_prepare+0x410/0x410
[ 104.990702][ T6588] ? find_held_lock+0x2d/0x110
[ 104.990702][ T6588] ___sys_sendmsg+0x11d/0x1b0
[ 104.990702][ T6588] ? do_recvmmsg+0x700/0x700
[ 104.990702][ T6588] ? __fget_files+0x260/0x420
[ 104.990702][ T6588] ? reacquire_held_locks+0x4b0/0x4b0
[ 104.990702][ T6588] ? __fget_files+0x282/0x420
[ 104.990702][ T6588] ? __fget_light+0xe6/0x270
[ 104.990702][ T6588] __sys_sendmmsg+0x18e/0x430
[ 104.990702][ T6588] ? __ia32_sys_sendmsg+0xb0/0xb0
[ 104.990702][ T6588] ? reacquire_held_locks+0x4b0/0x4b0
[ 104.990702][ T6588] ? rcu_is_watching+0x12/0xb0
[ 104.990702][ T6588] ? xfd_validate_state+0x5d/0x180
[ 104.990702][ T6588] ? restore_fpregs_from_fpstate+0xc1/0x1d0
[ 104.990702][ T6588] ? unlock_page_memcg+0x2d0/0x2d0
[ 104.990702][ T6588] ? do_futex+0x350/0x350
[ 104.990702][ T6588] __x64_sys_sendmmsg+0x9c/0x100
[ 104.990702][ T6588] ? syscall_enter_from_user_mode+0x26/0x80
[ 104.990702][ T6588] do_syscall_64+0x38/0xb0
[ 104.990702][ T6588] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 104.990702][ T6588] RIP: 0033:0x7f76c66ee89d
[ 104.990702][ T6588] Code: 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3
0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c
24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 4b 05 0e 00 f7 d8 64
89 01 48
[ 104.990702][ T6588] RSP: 002b:00007f76c65d4c68 EFLAGS: 00000203
ORIG_RAX: 0000000000000133
[ 104.990702][ T6588] RAX: ffffffffffffffda RBX: 00007f76c65d5cdc RCX:
00007f76c66ee89d
[ 104.990702][ T6588] RDX: 040000000000009f RSI: 00000000200002c0 RDI:
0000000000000007
[ 104.990702][ T6588] RBP: 00007f76c65d4de0 R08: 0000000100000001 R09:
0000000000000000
[ 104.990702][ T6588] R10: 0000000000000000 R11: 0000000000000203 R12:
fffffffffffffeb8
[ 104.990702][ T6588] R13: 0000000000000002 R14: 00007ffce5a5fd90 R15:
00007f76c65b5000
[ 104.990702][ T6588] </TASK>
[ 104.990702][ T6588] Kernel Offset: disabled
[ 104.990702][ T6588] Rebooting in 86400 seconds..
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting
2023-05-23 11:36 ` Pedro Tammela
@ 2023-05-23 20:06 ` Peilin Ye
0 siblings, 0 replies; 13+ messages in thread
From: Peilin Ye @ 2023-05-23 20:06 UTC (permalink / raw)
To: Pedro Tammela
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko, Peilin Ye,
Daniel Borkmann, John Fastabend, Hillf Danton, netdev,
linux-kernel, Cong Wang, Vlad Buslov
On Tue, May 23, 2023 at 08:36:35AM -0300, Pedro Tammela wrote:
> > Thanks for testing this, but the syzbot reproducer creates ingress Qdiscs
> > under TC_H_ROOT, which isn't covered by [6/6] i.e. it exercises the
> > "!ingress" path in qdisc_graft(). I think that's why you are still seeing
> > the oops. Adding sch_{ingress,clsact} to TC_H_ROOT is no longer possible
> > after [1,2/6], and I think we'll need a different reproducer for [5,6/6].
>
> I was still able to trigger an oops with the full patchset:
>
> [ 104.944353][ T6588] ------------[ cut here ]------------
> [ 104.944896][ T6588] jump label: negative count!
> [ 104.945780][ T6588] WARNING: CPU: 0 PID: 6588 at kernel/jump_label.c:263
> static_key_slow_try_dec+0xf2/0x110
> [ 104.946795][ T6588] Modules linked in:
> [ 104.947111][ T6588] CPU: 0 PID: 6588 Comm: repro Not tainted
> 6.4.0-rc2-00191-g4a3f9100193d #3
> [ 104.947765][ T6588] Hardware name: QEMU Standard PC (i440FX + PIIX,
> 1996), BIOS 1.16.0-debian-1.16.0-5 04/01/2014
> [ 104.948557][ T6588] RIP: 0010:static_key_slow_try_dec+0xf2/0x110
> [ 104.949064][ T6588] Code: d5 ff e8 c1 33 d5 ff 44 89 e8 5b 5d 41 5c 41 5d
> c3 44 89 e5 e9 66 ff ff ff e8 aa 33 d5 ff 48 c7 c7 00 9c 56 8a e8 4e ce 9c
> ff <0f> 0b eb ae 48 89 df e8 02 4b 28 00 e9 42 ff ff ff 66 66 2e 0f 1f
> [ 104.951134][ T6588] RSP: 0018:ffffc900079cf2c0 EFLAGS: 00010286
> [ 104.951646][ T6588] RAX: 0000000000000000 RBX: ffffffff9213a160 RCX:
> 0000000000000000
> [ 104.952269][ T6588] RDX: ffff888112f83b80 RSI: ffffffff814c7747 RDI:
> 0000000000000001
> [ 104.952901][ T6588] RBP: 00000000ffffffff R08: 0000000000000001 R09:
> 0000000000000000
> [ 104.953523][ T6588] R10: 0000000000000001 R11: 0000000000000001 R12:
> 00000000ffffffff
> [ 104.954133][ T6588] R13: ffff88816a514001 R14: 0000000000000001 R15:
> ffffffff8e7b0680
> [ 104.954746][ T6588] FS: 00007f76c65d56c0(0000) GS:ffff8881f5a00000(0000)
> knlGS:0000000000000000
> [ 104.955430][ T6588] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 104.955941][ T6588] CR2: 00007f9a40357a50 CR3: 000000011461e000 CR4:
> 0000000000350ef0
> [ 104.956559][ T6588] Call Trace:
> [ 104.956829][ T6588] <TASK>
> [ 104.957062][ T6588] ? clsact_egress_block_get+0x40/0x40
> [ 104.957507][ T6588] static_key_slow_dec+0x60/0xc0
> [ 104.957906][ T6588] qdisc_create+0xa45/0x1090
> [ 104.958274][ T6588] ? tc_get_qdisc+0xb70/0xb70
> [ 104.958646][ T6588] tc_modify_qdisc+0x491/0x1b70
> [ 104.959031][ T6588] ? qdisc_create+0x1090/0x1090
> [ 104.959420][ T6588] ? bpf_lsm_capable+0x9/0x10
> [ 104.959797][ T6588] ? qdisc_create+0x1090/0x1090
Ah, qdisc_create() calls ->destroy() even "if ops->init() failed". We
should check sch->parent in {ingress,clsact}_destroy() too. I'll update
[1,2/6] in v5.
Thanks for reporting this! Seems like I should've run the reproducer
nevertheless. I'll run it before posting v5.
Thanks,
Peilin Ye
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting
2023-05-22 23:55 ` [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting Peilin Ye
2023-05-23 3:51 ` Pedro Tammela
@ 2023-05-23 14:04 ` Vlad Buslov
2023-05-23 18:52 ` Peilin Ye
1 sibling, 1 reply; 13+ messages in thread
From: Vlad Buslov @ 2023-05-23 14:04 UTC (permalink / raw)
To: Peilin Ye
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko, Peilin Ye,
Daniel Borkmann, John Fastabend, Vlad Buslov, Pedro Tammela,
Hillf Danton, netdev, linux-kernel, Cong Wang
Hi Peilin,
Thanks again for the great analysis and investing effort into fixing
this!
On Mon 22 May 2023 at 16:55, Peilin Ye <yepeilin.cs@gmail.com> wrote:
> mini_Qdisc_pair::p_miniq is a double pointer to mini_Qdisc, initialized in
> ingress_init() to point to net_device::miniq_ingress. ingress Qdiscs
> access this per-net_device pointer in mini_qdisc_pair_swap(). Similar for
> clsact Qdiscs and miniq_egress.
>
> Unfortunately, after introducing RTNL-unlocked RTM_{NEW,DEL,GET}TFILTER
> requests (thanks Hillf Danton for the hint), when replacing ingress or
> clsact Qdiscs, for example, the old Qdisc ("@old") could access the same
> miniq_{in,e}gress pointer(s) concurrently with the new Qdisc ("@new"),
> causing race conditions [1] including a use-after-free bug in
> mini_qdisc_pair_swap() reported by syzbot:
>
> BUG: KASAN: slab-use-after-free in mini_qdisc_pair_swap+0x1c2/0x1f0 net/sched/sch_generic.c:1573
> Write of size 8 at addr ffff888045b31308 by task syz-executor690/14901
> ...
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0xd9/0x150 lib/dump_stack.c:106
> print_address_description.constprop.0+0x2c/0x3c0 mm/kasan/report.c:319
> print_report mm/kasan/report.c:430 [inline]
> kasan_report+0x11c/0x130 mm/kasan/report.c:536
> mini_qdisc_pair_swap+0x1c2/0x1f0 net/sched/sch_generic.c:1573
> tcf_chain_head_change_item net/sched/cls_api.c:495 [inline]
> tcf_chain0_head_change.isra.0+0xb9/0x120 net/sched/cls_api.c:509
> tcf_chain_tp_insert net/sched/cls_api.c:1826 [inline]
> tcf_chain_tp_insert_unique net/sched/cls_api.c:1875 [inline]
> tc_new_tfilter+0x1de6/0x2290 net/sched/cls_api.c:2266
> ...
>
> @old and @new should not affect each other. In other words, @old should
> never modify miniq_{in,e}gress after @new, and @new should not update
> @old's RCU state. Fixing without changing sch_api.c turned out to be
> difficult (please refer to Closes: for discussions). Instead, make sure
> @new's first call always happen after @old's last call, in
> qdisc_destroy(), has finished:
>
> In qdisc_graft(), return -EAGAIN and tell the caller to replay
> (suggested by Vlad Buslov) if @old has any ongoing RTNL-unlocked filter
> requests, and call qdisc_destroy() for @old before grafting @new.
>
> Introduce qdisc_refcount_dec_if_one() as the counterpart of
> qdisc_refcount_inc_nz() used for RTNL-unlocked filter requests. Introduce
> a non-static version of qdisc_destroy() that does a TCQ_F_BUILTIN check,
> just like qdisc_put() etc.
>
> Depends on patch "net/sched: Refactor qdisc_graft() for ingress and clsact
> Qdiscs".
>
> [1] To illustrate, the syzkaller reproducer adds ingress Qdiscs under
> TC_H_ROOT (no longer possible after patch "net/sched: sch_ingress: Only
> create under TC_H_INGRESS") on eth0 that has 8 transmission queues:
>
> Thread 1 creates ingress Qdisc A (containing mini Qdisc a1 and a2), then
> adds a flower filter X to A.
>
> Thread 2 creates another ingress Qdisc B (containing mini Qdisc b1 and
> b2) to replace A, then adds a flower filter Y to B.
>
> Thread 1 A's refcnt Thread 2
> RTM_NEWQDISC (A, RTNL-locked)
> qdisc_create(A) 1
> qdisc_graft(A) 9
>
> RTM_NEWTFILTER (X, RTNL-unlocked)
> __tcf_qdisc_find(A) 10
> tcf_chain0_head_change(A)
> mini_qdisc_pair_swap(A) (1st)
> |
> | RTM_NEWQDISC (B, RTNL-locked)
> RCU sync 2 qdisc_graft(B)
> | 1 notify_and_destroy(A)
> |
> tcf_block_release(A) 0 RTM_NEWTFILTER (Y, RTNL-unlocked)
> qdisc_destroy(A) tcf_chain0_head_change(B)
> tcf_chain0_head_change_cb_del(A) mini_qdisc_pair_swap(B) (2nd)
> mini_qdisc_pair_swap(A) (3rd) |
> ... ...
>
> Here, B calls mini_qdisc_pair_swap(), pointing eth0->miniq_ingress to its
> mini Qdisc, b1. Then, A calls mini_qdisc_pair_swap() again during
> ingress_destroy(), setting eth0->miniq_ingress to NULL, so ingress packets
> on eth0 will not find filter Y in sch_handle_ingress().
>
> This is only one of the possible consequences of concurrently accessing
> miniq_{in,e}gress pointers. The point is clear though: again, A should
> never modify those per-net_device pointers after B, and B should not
> update A's RCU state.
>
> Fixes: 7a096d579e8e ("net: sched: ingress: set 'unlocked' flag for Qdisc ops")
> Fixes: 87f373921c4e ("net: sched: ingress: set 'unlocked' flag for clsact Qdisc ops")
> Reported-by: syzbot+b53a9c0d1ea4ad62da8b@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/r/0000000000006cf87705f79acf1a@google.com/
> Cc: Hillf Danton <hdanton@sina.com>
> Cc: Vlad Buslov <vladbu@mellanox.com>
> Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
> ---
> changes in v2:
> - replay the request if the current Qdisc has any ongoing RTNL-unlocked
> filter requests (Vlad)
> - minor changes in code comments and commit log
>
> include/net/sch_generic.h | 8 ++++++++
> net/sched/sch_api.c | 32 ++++++++++++++++++++++++++------
> net/sched/sch_generic.c | 14 +++++++++++---
> 3 files changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index fab5ba3e61b7..3e9cc43cbc90 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -137,6 +137,13 @@ static inline void qdisc_refcount_inc(struct Qdisc *qdisc)
> refcount_inc(&qdisc->refcnt);
> }
>
> +static inline bool qdisc_refcount_dec_if_one(struct Qdisc *qdisc)
> +{
> + if (qdisc->flags & TCQ_F_BUILTIN)
> + return true;
> + return refcount_dec_if_one(&qdisc->refcnt);
> +}
> +
> /* Intended to be used by unlocked users, when concurrent qdisc release is
> * possible.
> */
> @@ -652,6 +659,7 @@ void dev_deactivate_many(struct list_head *head);
> struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue,
> struct Qdisc *qdisc);
> void qdisc_reset(struct Qdisc *qdisc);
> +void qdisc_destroy(struct Qdisc *qdisc);
> void qdisc_put(struct Qdisc *qdisc);
> void qdisc_put_unlocked(struct Qdisc *qdisc);
> void qdisc_tree_reduce_backlog(struct Qdisc *qdisc, int n, int len);
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index f72a581666a2..b3bafa6c1b44 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1080,10 +1080,18 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
> if ((q && q->flags & TCQ_F_INGRESS) ||
> (new && new->flags & TCQ_F_INGRESS)) {
> ingress = 1;
> - if (!dev_ingress_queue(dev)) {
> + dev_queue = dev_ingress_queue(dev);
> + if (!dev_queue) {
> NL_SET_ERR_MSG(extack, "Device does not have an ingress queue");
> return -ENOENT;
> }
> +
> + /* Replay if the current ingress (or clsact) Qdisc has ongoing
> + * RTNL-unlocked filter request(s). This is the counterpart of that
> + * qdisc_refcount_inc_nz() call in __tcf_qdisc_find().
> + */
> + if (!qdisc_refcount_dec_if_one(dev_queue->qdisc_sleeping))
> + return -EAGAIN;
> }
>
> if (dev->flags & IFF_UP)
> @@ -1104,8 +1112,16 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
> qdisc_put(old);
> }
> } else {
> - dev_queue = dev_ingress_queue(dev);
> - old = dev_graft_qdisc(dev_queue, new);
> + old = dev_graft_qdisc(dev_queue, NULL);
> +
> + /* {ingress,clsact}_destroy() @old before grafting @new to avoid
> + * unprotected concurrent accesses to net_device::miniq_{in,e}gress
> + * pointer(s) in mini_qdisc_pair_swap().
> + */
> + qdisc_notify(net, skb, n, classid, old, new, extack);
> + qdisc_destroy(old);
> +
> + dev_graft_qdisc(dev_queue, new);
> }
>
> skip:
> @@ -1119,8 +1135,6 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>
> if (new && new->ops->attach)
> new->ops->attach(new);
> - } else {
> - notify_and_destroy(net, skb, n, classid, old, new, extack);
> }
>
> if (dev->flags & IFF_UP)
> @@ -1458,6 +1472,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> struct Qdisc *p = NULL;
> int err;
>
> +replay:
Perhaps also set q and p to NULL here? Even though on cursory look you
should get the same lookup result since the function is called under
rtnl_lock, tc_modify_qdisc() does this on replay ("Reinit, just in case
something touches this.") and tc_new_tfilter() got some non-obvious bugs
after I introduced replay there without re-setting some of the required
variables.
> err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
> rtm_tca_policy, extack);
> if (err < 0)
> @@ -1515,8 +1530,11 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> return -ENOENT;
> }
> err = qdisc_graft(dev, p, skb, n, clid, NULL, q, extack);
> - if (err != 0)
> + if (err != 0) {
> + if (err == -EAGAIN)
> + goto replay;
> return err;
> + }
> } else {
> qdisc_notify(net, skb, n, clid, NULL, q, NULL);
> }
> @@ -1704,6 +1722,8 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> if (err) {
> if (q)
> qdisc_put(q);
> + if (err == -EAGAIN)
> + goto replay;
> return err;
> }
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 37e41f972f69..e14ed47f961c 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1046,7 +1046,7 @@ static void qdisc_free_cb(struct rcu_head *head)
> qdisc_free(q);
> }
>
> -static void qdisc_destroy(struct Qdisc *qdisc)
> +static void __qdisc_destroy(struct Qdisc *qdisc)
> {
> const struct Qdisc_ops *ops = qdisc->ops;
>
> @@ -1070,6 +1070,14 @@ static void qdisc_destroy(struct Qdisc *qdisc)
> call_rcu(&qdisc->rcu, qdisc_free_cb);
> }
>
> +void qdisc_destroy(struct Qdisc *qdisc)
> +{
> + if (qdisc->flags & TCQ_F_BUILTIN)
> + return;
> +
> + __qdisc_destroy(qdisc);
> +}
> +
> void qdisc_put(struct Qdisc *qdisc)
> {
> if (!qdisc)
> @@ -1079,7 +1087,7 @@ void qdisc_put(struct Qdisc *qdisc)
> !refcount_dec_and_test(&qdisc->refcnt))
> return;
>
> - qdisc_destroy(qdisc);
> + __qdisc_destroy(qdisc);
> }
> EXPORT_SYMBOL(qdisc_put);
>
> @@ -1094,7 +1102,7 @@ void qdisc_put_unlocked(struct Qdisc *qdisc)
> !refcount_dec_and_rtnl_lock(&qdisc->refcnt))
> return;
>
> - qdisc_destroy(qdisc);
> + __qdisc_destroy(qdisc);
> rtnl_unlock();
> }
> EXPORT_SYMBOL(qdisc_put_unlocked);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting
2023-05-23 14:04 ` Vlad Buslov
@ 2023-05-23 18:52 ` Peilin Ye
0 siblings, 0 replies; 13+ messages in thread
From: Peilin Ye @ 2023-05-23 18:52 UTC (permalink / raw)
To: Vlad Buslov
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jamal Hadi Salim, Cong Wang, Jiri Pirko, Peilin Ye,
Daniel Borkmann, John Fastabend, Vlad Buslov, Pedro Tammela,
Hillf Danton, netdev, linux-kernel, Cong Wang
Hi Vlad,
On Tue, May 23, 2023 at 05:04:40PM +0300, Vlad Buslov wrote:
> > @@ -1458,6 +1472,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> > struct Qdisc *p = NULL;
> > int err;
> >
> > +replay:
>
> Perhaps also set q and p to NULL here? Even though on cursory look you
> should get the same lookup result since the function is called under
> rtnl_lock, tc_modify_qdisc() does this on replay ("Reinit, just in case
> something touches this.") and tc_new_tfilter() got some non-obvious bugs
> after I introduced replay there without re-setting some of the required
> variables.
Sure, I'll reinitialize tcm, q and p after "replay:" in next version, just
like tc_modify_qdisc(). Thanks for the suggestion!
Peilin Ye
^ permalink raw reply [flat|nested] 13+ messages in thread