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] [day] [month] [year] [list]

Message-ID: <Mg8Y4BPLchTQ9KEXuuZ4-nTKQf2s0SFeWM8X23wXeUJaJ0bbM7eGSnivxX3wmh8dob2WHtI17KBSXXEywmk5-Dqqg6RmctdK9HHn0lBtTN0=@willsroot.io>
Date: Tue, 24 Jun 2025 04:46:22 +0000
From: William Liu <will@...lsroot.io>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: netdev@...r.kernel.org, jhs@...atatu.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 Tuesday, June 24th, 2025 at 4:27 AM, Cong Wang <xiyou.wangcong@...il.com> wrote:

> 
> 
> On Sun, Jun 22, 2025 at 07:05:18PM +0000, William Liu 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.
> 
> 
> Thanks for the patch. But singling out this specific case in netem does
> not look like an elegant solution to me, it looks hacky.
> 
> I know you probably discussed this with Jamal before posting this, could
> you please summarize why you decided to pick up this solution in the
> changelog? It is very important for code review.
> 

Oh oops, I saw this email right after my reply and posting v2. I will update this in v3.

I originally suggested adding a single bit to the skb to track duplication for netem - Jamal mentioned a similar issue with a loopy DOS due to the removal of some TTL bits in the skb structure. I assumed it would be ok since it didn't change the struct size (and even if it did, it's backed by a slab allocator), but was informed that such a change would be very hard to push through.

The alternative approach is to ensure that multiple netems do not remain on the same qdisc tree path when duplication is involved. But Jamal pointed out that the issue will come up once again when filters and actions redirect packets from one path of the qdisc tree to another. 

> One additional comment below.
> 
> > [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;
> > +}
> 
> 
> This is not worth a helper, because it only has one line and it is
> actually more readable without using a helper, IMHO.
> 
> Regards,
> Cong