netdev - Re: [PATCH net 1/2] net/sched: Restrict conditions for adding duplicating netems to qdisc tree

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]

Message-ID: <CAM0EoMmbcudme6=ogcUdQ1qt9MThChqy=37Ck1vhnw-4VuKmNw@mail.gmail.com>
Date: Mon, 23 Jun 2025 10:33:29 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: William Liu <will@...lsroot.io>
Cc: netdev@...r.kernel.org, xiyou.wangcong@...il.com, victor@...atatu.com, 
	pctammela@...atatu.com, pabeni@...hat.com, kuba@...nel.org, 
	stephen@...workplumber.org, dcaratti@...hat.com, savy@...t3mfailure.io, 
	jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com, horms@...nel.org
Subject: Re: [PATCH net 1/2] net/sched: Restrict conditions for adding
 duplicating netems to qdisc tree

BTW, you did fail to test tdc like i asked you to do. It was a trap
question - if you did run it you would have caught the issue Jakub
just pointed out. Maybe i shouldnt have been so coy/evil..
Please run tdc fully..

On Sun, Jun 22, 2025 at 3:05 PM William Liu <will@...lsroot.io> wrote:
>
> netem_enqueue's duplication prevention logic breaks when a netem
> resides in a qdisc tree with other netems - this can lead to a
> soft lockup and OOM loop in netem_dequeue as seen in [1].
> Ensure that a duplicating netem cannot exist in a tree with other
> netems.
>
> [1] https://lore.kernel.org/netdev/8DuRWwfqjoRDLDmBMlIfbrsZg9Gx50DHJc1ilxsEBNe2D6NMoigR_eIRIG0LOjMc3r10nUUZtArXx4oZBIdUfZQrwjcQhdinnMis_0G7VEk=@willsroot.io/
>
> Fixes: 0afb51e72855 ("[PKT_SCHED]: netem: reinsert for duplication")
> Reported-by: William Liu <will@...lsroot.io>
> Reported-by: Savino Dicanosa <savy@...t3mfailure.io>
> Signed-off-by: William Liu <will@...lsroot.io>
> Signed-off-by: Savino Dicanosa <savy@...t3mfailure.io>
> ---
>  net/sched/sch_netem.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index fdd79d3ccd8c..308ce6629d7e 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -973,6 +973,46 @@ static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
>         return 0;
>  }
>
> +static const struct Qdisc_class_ops netem_class_ops;
> +
> +static inline bool has_duplication(struct Qdisc *sch)
> +{
> +       struct netem_sched_data *q = qdisc_priv(sch);
> +
> +       return q->duplicate != 0;

return q->duplicate not good enough?

> +}
> +
> +static int check_netem_in_tree(struct Qdisc *sch, bool only_duplicating,
> +                              struct netlink_ext_ack *extack)
> +{
> +       struct Qdisc *root, *q;
> +       unsigned int i;
> +

"only_duplicating" is very confusing. Why not "duplicates"?

> +       root = qdisc_root_sleeping(sch);
> +
> +       if (sch != root && root->ops->cl_ops == &netem_class_ops) {
> +               if (!only_duplicating || has_duplication(root))
> +                       goto err;
> +       }
> +
> +       if (!qdisc_dev(root))
> +               return 0;
> +
> +       hash_for_each(qdisc_dev(root)->qdisc_hash, i, q, hash) {
> +               if (sch != q && q->ops->cl_ops == &netem_class_ops) {
> +                       if (!only_duplicating || has_duplication(q))

if (duplicates || has_duplication)

> +                               goto err;
> +               }
> +       }
> +
> +       return 0;
> +
> +err:
> +       NL_SET_ERR_MSG(extack,
> +                      "netem: cannot mix duplicating netems with other netems in tree");
> +       return -EINVAL;
> +}
> +
>  /* Parse netlink message to set options */
>  static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>                         struct netlink_ext_ack *extack)
> @@ -1031,6 +1071,11 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
>         q->gap = qopt->gap;
>         q->counter = 0;
>         q->loss = qopt->loss;
> +
> +       ret = check_netem_in_tree(sch, qopt->duplicate == 0, extack);

check_netem_in_tree(sch, qopt->duplicate, extack) ?


cheers,
jamal

> +       if (ret)
> +               goto unlock;
> +
>         q->duplicate = qopt->duplicate;
>
>         /* for compatibility with earlier versions.
> --
> 2.43.0
>
>