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: <zs0zNqlNRnnijrll33yl0m4VGbDv-dTgQecTErwr97GnuwynGtuLRmz5tG63EnH41B3tmZrYOTs7FSgVbYBusapPk7CkmHOLKoBST_ITufg=@willsroot.io>
Date: Tue, 24 Jun 2025 04:28:31 +0000
From: William Liu <will@...lsroot.io>
To: Jamal Hadi Salim <jhs@...atatu.com>
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

On Monday, June 23rd, 2025 at 2:33 PM, Jamal Hadi Salim <jhs@...atatu.com> wrote:

> 
> 
> 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..
> 

tdc has been fully run for v2 - I was under the assumption that only the netem category mattered. Is there any reason tc-tests/qdiscs/teql.json does not run under a new namepsace?

> 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

Ok, thank you to everyone for the helpful feedback. I have addressed everything and just posted v2.