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: <aFopK5iWHa0wrEIk@pop-os.localdomain>
Date: Mon, 23 Jun 2025 21:27:23 -0700
From: Cong Wang <xiyou.wangcong@...il.com>
To: William Liu <will@...lsroot.io>
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 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.

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