netdev - Re: [PATCH net v3 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] [day] [month] [year] [list]

Message-ID: <CAM0EoM=rkxR0OuDm8E8NMbHwxp2CknxGH=Mwz=iaPfn6nHRnsA@mail.gmail.com>
Date: Wed, 25 Jun 2025 10:02:57 -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 v3 1/2] net/sched: Restrict conditions for adding
 duplicating netems to qdisc tree

Please, just wait maybe a day or two next time if you get feedback
before sending an updated version.

On Tue, Jun 24, 2025 at 1:57 AM 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].
> Since we do not want to add bits to sk_buff for tracking
> duplication metadata for this one specific case, we have to ensure
> that a duplicating netem cannot exist in a tree with other netems.

I think what is useful is to describe the discussions we had, off top
of my head:
1) you had the skb bits initially - to which i pointed out that it is
politically incorrect solution to add more metadata bits to the skb. I
posted some url, include that url from lwn
2) you then added skb cb state, to which i pointed that it can be
overwritten defeating the purpose.
3) we then went to using a similar approach as mirred loop-avoidance
and found out that it wont work for netem dequeu (and i cant remember
what else) with stopping the loop
4) is this solution which makes it illegal to have more than one netem
in a hierarchy; i.e we are identifying that as an "illegal" hierarchy.

cheers,
jamal

> Technically, this check is only required on the ancestral path,
> but filters and actions can change a packet's path in the
> qdisc tree and require us to apply the restriction over the tree.
>
> [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>
> ---
> v2 -> v3:
>   - Clarify reasoning for approach in changelog
>   - Removed has_duplication
> v1 -> v2: https://lore.kernel.org/all/20250622190344.446090-1-will@willsroot.io/
>   - Renamed only_duplicating to duplicates and invert logic for clarity
> ---
>  net/sched/sch_netem.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index fdd79d3ccd8c..eafc316ae319 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -973,6 +973,41 @@ static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
>         return 0;
>  }
>
> +static const struct Qdisc_class_ops netem_class_ops;
> +
> +static int check_netem_in_tree(struct Qdisc *sch, bool duplicates,
> +                              struct netlink_ext_ack *extack)
> +{
> +       struct Qdisc *root, *q;
> +       unsigned int i;
> +
> +       root = qdisc_root_sleeping(sch);
> +
> +       if (sch != root && root->ops->cl_ops == &netem_class_ops) {
> +               if (duplicates ||
> +                   ((struct netem_sched_data *)qdisc_priv(root))->duplicate)
> +                       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 (duplicates ||
> +                           ((struct netem_sched_data *)qdisc_priv(q))->duplicate)
> +                               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 +1066,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, extack);
> +       if (ret)
> +               goto unlock;
> +
>         q->duplicate = qopt->duplicate;
>
>         /* for compatibility with earlier versions.
> --
> 2.43.0
>
>