netdev - Re: [PATCH net] sch_htb: Fix prematurely activating netdev during htb_destroy_class

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 Jan 2023 22:12:11 +0200
From:   Maxim Mikityanskiy <maxtram95@...il.com>
To:     Rahul Rameshbabu <rrameshbabu@...dia.com>
Cc:     netdev@...r.kernel.org, Tariq Toukan <tariqt@...dia.com>,
        Gal Pressman <gal@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net] sch_htb: Fix prematurely activating netdev during htb_destroy_class_offload

On Wed, 4 Jan 2023 at 19:53, Rahul Rameshbabu <rrameshbabu@...dia.com> 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.

> 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@...dia.com>
> Acked-by: Saeed Mahameed <saeedm@...dia.com>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Maxim Mikityanskiy <maxtram95@...il.com>
> ---
>  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
>