Re: [PATCH net v2 0/5] net_sched: Adapt qdiscs for reentrant enqueue cases

[Cong Wang <xiyou.wangcong@gmail.com>]

On Tue, Apr 22, 2025 at 01:21:22PM +0200, Paolo Abeni wrote:
> On 4/17/25 9:23 PM, Cong Wang wrote:
> > On Wed, Apr 16, 2025 at 07:24:22AM -0300, Victor Nogueira wrote:
> >> As described in Gerrard's report [1], there are cases where netem can
> >> make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets,
> >> qfq) break whenever the enqueue callback has reentrant behaviour.
> >> This series addresses these issues by adding extra checks that cater for
> >> these reentrant corner cases. This series has passed all relevant test
> >> cases in the TDC suite.
> >>
> >> [1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/
> >>
> > 
> > I am wondering why we need to enqueue the duplicate skb before enqueuing
> > the original skb in netem? IOW, why not just swap them?
> 
> It's not clear to me what you are suggesting, could you please rephrase
> and/or expand the above?

Sure, below is the change on my mind:

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index fdd79d3ccd8c..000f8138f561 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -531,21 +531,6 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return NET_XMIT_DROP;
 	}
 
-	/*
-	 * If doing duplication then re-insert at top of the
-	 * qdisc tree, since parent queuer expects that only one
-	 * skb will be queued.
-	 */
-	if (skb2) {
-		struct Qdisc *rootq = qdisc_root_bh(sch);
-		u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
-
-		q->duplicate = 0;
-		rootq->enqueue(skb2, rootq, to_free);
-		q->duplicate = dupsave;
-		skb2 = NULL;
-	}
-
 	qdisc_qstats_backlog_inc(sch, skb);
 
 	cb = netem_skb_cb(skb);
@@ -613,6 +598,21 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		sch->qstats.requeues++;
 	}
 
+	/*
+	 * If doing duplication then re-insert at top of the
+	 * qdisc tree, since parent queuer expects that only one
+	 * skb will be queued.
+	 */
+	if (skb2) {
+		struct Qdisc *rootq = qdisc_root_bh(sch);
+		u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
+
+		q->duplicate = 0;
+		rootq->enqueue(skb2, rootq, to_free);
+		q->duplicate = dupsave;
+		skb2 = NULL;
+	}
+
 finish_segs:
 	if (skb2)
 		__qdisc_drop(skb2, to_free);

> 
> When duplication packets, I think we will need to call root->enqueue()
> no matter what, to ensure proper accounting, and that would cause the
> re-entrancy issue. What I'm missing?

The problem here is the ordering, if we enqueue the skb2 (aka the
duplication packet) first (as what it is), the qlen is not yet increased
at this point so the qdisc is technically still empty (as we test qlen).

If we reverse that order, that is, enqueuing skb2 _after_ the original
packet, qlen should be increased by tfifo_enqueue() _before_ skb2 is
enqueue, so the qdisc is not empty for skb2 any more.

This is why I think (meaning I never test it) it could solve the problem
here and is a much simpler fix (0 line of code in delta).

Thanks!