Re: [PATCH net] sch_htb: Fix prematurely activating netdev during htb_destroy_class_offload
From: Maxim Mikityanskiy <hidden>
Date: 2023-01-04 20:12:45
On Wed, 4 Jan 2023 at 19:53, Rahul Rameshbabu [off-list ref] wrote:
When the netdev qdisc is updated correctly with the new qdisc before destroying the old qdisc, the netdev should not be activated till cleanup is completed. When htb_destroy_class_offload called htb_graft_helper, the netdev may be activated before cleanup is completed.
Oh, so that's what was happening! Now I get the full picture: 1. The user does RTM_DELQDISC. 2. qdisc_graft calls dev_deactivate, which sets dev_queue->qdisc to NULL, but keeps dev_queue->qdisc_sleeping. 3. The loop in qdisc_graft calls dev_graft_qdisc(dev_queue, new), where new is NULL, for each queue. 4. Then we get into htb_destroy_class_offload, and it's important whether dev->qdisc is still HTB (before Eric's patch) or noop_qdisc (after Eric's patch). 5. If dev->qdisc is noop_qdisc, and htb_graft_helper accidentally activates the netdev, attach_default_qdiscs will be called, and dev_queue->qdisc will no longer be NULL for the rest of the queues, hence the WARN_ON triggering. Nice catch indeed, premature activation of the netdev wasn't intended.
quoted hunk ↗ jump to hunk
The new netdev qdisc may be used prematurely by queues before cleanup is done. Call dev_graft_qdisc in place of htb_graft_helper when destroying the htb to prevent premature netdev activation. Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload") Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> Acked-by: Saeed Mahameed <saeedm@nvidia.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Maxim Mikityanskiy <redacted> --- net/sched/sch_htb.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index 2238edece1a4..f62334ef016a 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c@@ -1557,14 +1557,16 @@ static int htb_destroy_class_offload(struct Qdisc *sch, struct htb_class *cl, WARN_ON(!q); dev_queue = htb_offload_get_queue(cl); - old = htb_graft_helper(dev_queue, NULL); - if (destroying) + if (destroying) { + old = dev_graft_qdisc(dev_queue, NULL); /* Before HTB is destroyed, the kernel grafts noop_qdisc to * all queues. */ WARN_ON(!(old->flags & TCQ_F_BUILTIN));
Now regarding this WARN_ON, I have concerns about its correctness. Can the user replace the root qdisc from HTB to something else with a single command? I.e. instead of `tc qdisc del dev eth2 root handle 1:` do `tc qdisc replace ...` or whatever that causes qdisc_graft to be called with new != NULL? If that is possible, then: 1. `old` won't be noop_qdisc, but rather the new qdisc (if it doesn't support the attach callback) or the old one left from HTB (old == q, if the new qdisc supports the attach callback). WARN_ON should trigger. 2. We shouldn't even call dev_graft_qdisc in this case (if destroying is true). Likewise, we shouldn't try to revert it on errors or call qdisc_put on it. Could you please try to reproduce this scenario of triggering WARN_ON? I remember testing it, and something actually prevented me from doing a replacement, but maybe I just missed something back then.
- else
+ } else {
+ old = htb_graft_helper(dev_queue, NULL);
WARN_ON(old != q);
+ }
if (cl->parent) {
_bstats_update(&cl->parent->bstats_bias,
--
2.36.2