[Cake] [PATCH 0/4] sched: Fix qdisc interactions exposed by using sch_cake as a leaf qdisc

[Cake] [PATCH 0/4] sched: Fix qdisc interactions exposed by using sch_cake as a leaf qdisc

[Toke Høiland-Jørgensen]

This series fixes a couple of issues exposed by running sch_cake as a
leaf qdisc in an HFSC tree, which were discovered and reported by Pete
Heist. The interaction between CAKE's GSO splitting and the parent
qdisc's notion of its own queue length could cause queue stalls. While
investigating the report, I also noticed that several qdiscs would
dereference the skb pointer after dequeue, which is potentially
problematic since the GSO splitting code also frees the original skb.

See the individual patches in the series for details.

Toke Høiland-Jørgensen (4):
  sched: Avoid dereferencing skb pointer after child enqueue
  sched: Fix detection of empty queues in child qdiscs
  sch_api: Allow reducing queue backlog by a negative value
  sch_cake: Correctly update parent qlen when splitting GSO packets

 include/net/sch_generic.h |  3 +--
 net/sched/sch_api.c       |  3 +--
 net/sched/sch_cake.c      |  5 +++--
 net/sched/sch_cbs.c       |  3 ++-
 net/sched/sch_drr.c       |  7 +++++--
 net/sched/sch_dsmark.c    |  3 ++-
 net/sched/sch_hfsc.c      |  9 +++++----
 net/sched/sch_htb.c       |  3 ++-
 net/sched/sch_prio.c      |  3 ++-
 net/sched/sch_qfq.c       | 20 ++++++++++++--------
 net/sched/sch_tbf.c       |  3 ++-
 11 files changed, 37 insertions(+), 25 deletions(-)

-- 
2.20.1

[Cake] [PATCH 1/4] sched: Avoid dereferencing skb pointer after child enqueue

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

Parent qdiscs may dereference the pointer to the enqueued skb after
enqueue. However, both CAKE and TBF call consume_skb() on the original skb
when splitting GSO packets, leading to a potential use-after-free in the
parent. Fix this by avoiding dereferencing the skb pointer after enqueueing
to the child.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/sched/sch_cbs.c    |  3 ++-
 net/sched/sch_drr.c    |  3 ++-
 net/sched/sch_dsmark.c |  3 ++-
 net/sched/sch_hfsc.c   |  5 ++---
 net/sched/sch_htb.c    |  3 ++-
 net/sched/sch_prio.c   |  3 ++-
 net/sched/sch_qfq.c    | 16 +++++++++-------
 net/sched/sch_tbf.c    |  3 ++-
 8 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
index e689e11b6d0f..c6a502933fe7 100644
--- a/net/sched/sch_cbs.c
+++ b/net/sched/sch_cbs.c
@@ -88,13 +88,14 @@ static int cbs_child_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			     struct Qdisc *child,
 			     struct sk_buff **to_free)
 {
+	unsigned int len = qdisc_pkt_len(skb);
 	int err;
 
 	err = child->ops->enqueue(skb, child, to_free);
 	if (err != NET_XMIT_SUCCESS)
 		return err;
 
-	qdisc_qstats_backlog_inc(sch, skb);
+	sch->qstats.backlog += len;
 	sch->q.qlen++;
 
 	return NET_XMIT_SUCCESS;
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index cdebaed0f8cf..feaf47178653 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -350,6 +350,7 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch,
 static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		       struct sk_buff **to_free)
 {
+	unsigned int len = qdisc_pkt_len(skb);
 	struct drr_sched *q = qdisc_priv(sch);
 	struct drr_class *cl;
 	int err = 0;
@@ -376,7 +377,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		cl->deficit = cl->quantum;
 	}
 
-	qdisc_qstats_backlog_inc(sch, skb);
+	sch->qstats.backlog += len;
 	sch->q.qlen++;
 	return err;
 }
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index f6f480784bc6..42471464ded3 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -199,6 +199,7 @@ static struct tcf_block *dsmark_tcf_block(struct Qdisc *sch, unsigned long cl,
 static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			  struct sk_buff **to_free)
 {
+	unsigned int len = qdisc_pkt_len(skb);
 	struct dsmark_qdisc_data *p = qdisc_priv(sch);
 	int err;
 
@@ -271,7 +272,7 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return err;
 	}
 
-	qdisc_qstats_backlog_inc(sch, skb);
+	sch->qstats.backlog += len;
 	sch->q.qlen++;
 
 	return NET_XMIT_SUCCESS;
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index b18ec1f6de60..6bb8f73a8473 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1539,6 +1539,7 @@ hfsc_dump_qdisc(struct Qdisc *sch, struct sk_buff *skb)
 static int
 hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 {
+	unsigned int len = qdisc_pkt_len(skb);
 	struct hfsc_class *cl;
 	int uninitialized_var(err);
 
@@ -1560,8 +1561,6 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 	}
 
 	if (cl->qdisc->q.qlen == 1) {
-		unsigned int len = qdisc_pkt_len(skb);
-
 		if (cl->cl_flags & HFSC_RSC)
 			init_ed(cl, len);
 		if (cl->cl_flags & HFSC_FSC)
@@ -1576,7 +1575,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 
 	}
 
-	qdisc_qstats_backlog_inc(sch, skb);
+	sch->qstats.backlog += len;
 	sch->q.qlen++;
 
 	return NET_XMIT_SUCCESS;
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 58b449490757..30f9da7e1076 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -581,6 +581,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		       struct sk_buff **to_free)
 {
 	int uninitialized_var(ret);
+	unsigned int len = qdisc_pkt_len(skb);
 	struct htb_sched *q = qdisc_priv(sch);
 	struct htb_class *cl = htb_classify(skb, sch, &ret);
 
@@ -610,7 +611,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		htb_activate(q, cl);
 	}
 
-	qdisc_qstats_backlog_inc(sch, skb);
+	sch->qstats.backlog += len;
 	sch->q.qlen++;
 	return NET_XMIT_SUCCESS;
 }
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index cdf68706e40f..847141cd900f 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -72,6 +72,7 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
 static int
 prio_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 {
+	unsigned int len = qdisc_pkt_len(skb);
 	struct Qdisc *qdisc;
 	int ret;
 
@@ -88,7 +89,7 @@ prio_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 
 	ret = qdisc_enqueue(skb, qdisc, to_free);
 	if (ret == NET_XMIT_SUCCESS) {
-		qdisc_qstats_backlog_inc(sch, skb);
+		sch->qstats.backlog += len;
 		sch->q.qlen++;
 		return NET_XMIT_SUCCESS;
 	}
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index dc37c4ead439..8d5e55d5bed2 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -1210,6 +1210,7 @@ static struct qfq_aggregate *qfq_choose_next_agg(struct qfq_sched *q)
 static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		       struct sk_buff **to_free)
 {
+	unsigned int len = qdisc_pkt_len(skb), gso_segs;
 	struct qfq_sched *q = qdisc_priv(sch);
 	struct qfq_class *cl;
 	struct qfq_aggregate *agg;
@@ -1224,17 +1225,17 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	}
 	pr_debug("qfq_enqueue: cl = %x\n", cl->common.classid);
 
-	if (unlikely(cl->agg->lmax < qdisc_pkt_len(skb))) {
+	if (unlikely(cl->agg->lmax < len)) {
 		pr_debug("qfq: increasing maxpkt from %u to %u for class %u",
-			 cl->agg->lmax, qdisc_pkt_len(skb), cl->common.classid);
-		err = qfq_change_agg(sch, cl, cl->agg->class_weight,
-				     qdisc_pkt_len(skb));
+			 cl->agg->lmax, len, cl->common.classid);
+		err = qfq_change_agg(sch, cl, cl->agg->class_weight, len);
 		if (err) {
 			cl->qstats.drops++;
 			return qdisc_drop(skb, sch, to_free);
 		}
 	}
 
+	gso_segs = skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1;
 	err = qdisc_enqueue(skb, cl->qdisc, to_free);
 	if (unlikely(err != NET_XMIT_SUCCESS)) {
 		pr_debug("qfq_enqueue: enqueue failed %d\n", err);
@@ -1245,8 +1246,9 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return err;
 	}
 
-	bstats_update(&cl->bstats, skb);
-	qdisc_qstats_backlog_inc(sch, skb);
+	cl->bstats.bytes += len;
+	cl->bstats.packets += gso_segs;
+	sch->qstats.backlog += len;
 	++sch->q.qlen;
 
 	agg = cl->agg;
@@ -1254,7 +1256,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	if (cl->qdisc->q.qlen != 1) {
 		if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&
 		    list_first_entry(&agg->active, struct qfq_class, alist)
-		    == cl && cl->deficit < qdisc_pkt_len(skb))
+		    == cl && cl->deficit < len)
 			list_move_tail(&cl->alist, &agg->active);
 
 		return err;
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index 942dcca09cf2..7f272a9070c5 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -185,6 +185,7 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		       struct sk_buff **to_free)
 {
 	struct tbf_sched_data *q = qdisc_priv(sch);
+	unsigned int len = qdisc_pkt_len(skb);
 	int ret;
 
 	if (qdisc_pkt_len(skb) > q->max_size) {
@@ -200,7 +201,7 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return ret;
 	}
 
-	qdisc_qstats_backlog_inc(sch, skb);
+	sch->qstats.backlog += len;
 	sch->q.qlen++;
 	return NET_XMIT_SUCCESS;
 }
-- 
2.20.1

Re: [Cake] [PATCH 1/4] sched: Avoid dereferencing skb pointer after child enqueue

[Cong Wang]

On Mon, Jan 7, 2019 at 11:50 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> @@ -1254,7 +1256,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>         if (cl->qdisc->q.qlen != 1) {
>                 if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&


Isn't this comparison problematic too? While you are on it...

Re: [Cake] [PATCH 1/4] sched: Avoid dereferencing skb pointer after child enqueue

[Toke Høiland-Jørgensen]

Cong Wang <xiyou.wangcong@gmail.com> writes:

> On Mon, Jan 7, 2019 at 11:50 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> @@ -1254,7 +1256,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>>         if (cl->qdisc->q.qlen != 1) {
>>                 if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&
>
>
> Isn't this comparison problematic too? While you are on it...

Well, I was only looking at safety issues, and since it's not
dereferencing the pointer, that's not really an issue here. The check is
just going to always fail if GSO splitting is enabled. Which I'm not
actually sure is an error in this case?

-Toke

Re: [Cake] [PATCH 1/4] sched: Avoid dereferencing skb pointer after child enqueue

[Cong Wang]

On Wed, Jan 9, 2019 at 12:14 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> Cong Wang <xiyou.wangcong@gmail.com> writes:
>
> > On Mon, Jan 7, 2019 at 11:50 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> >> @@ -1254,7 +1256,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >>         if (cl->qdisc->q.qlen != 1) {
> >>                 if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&
> >
> >
> > Isn't this comparison problematic too? While you are on it...
>
> Well, I was only looking at safety issues, and since it's not
> dereferencing the pointer, that's not really an issue here. The check is
> just going to always fail if GSO splitting is enabled. Which I'm not
> actually sure is an error in this case?

Yeah, I knew you only fix defereferences. The comparison is used
to check if the enqueued packet is the head of the queue, which
is incorrect to me for GSO splitting case. Don't worry, we can fix it later.

[Cake] [PATCH 2/4] sched: Fix detection of empty queues in child qdiscs

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

Several qdiscs check on enqueue whether the packet was enqueued to a class
with an empty queue, in which case the class is activated. This is done by
checking if the qlen is exactly 1 after enqueue. However, if GSO splitting
is enabled in the child qdisc, a single packet can result in a qlen longer
than 1. This means the activation check fails, leading to a stalled queue.

Fix this by checking if the queue is empty *before* enqueue, and running
the activation logic if this was the case.

Reported-by: Pete Heist <pete@heistp.net>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/sched/sch_drr.c  | 4 +++-
 net/sched/sch_hfsc.c | 4 +++-
 net/sched/sch_qfq.c  | 4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index feaf47178653..09b800991065 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -354,6 +354,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	struct drr_sched *q = qdisc_priv(sch);
 	struct drr_class *cl;
 	int err = 0;
+	bool first;
 
 	cl = drr_classify(skb, sch, &err);
 	if (cl == NULL) {
@@ -363,6 +364,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return err;
 	}
 
+	first = !cl->qdisc->q.qlen;
 	err = qdisc_enqueue(skb, cl->qdisc, to_free);
 	if (unlikely(err != NET_XMIT_SUCCESS)) {
 		if (net_xmit_drop_count(err)) {
@@ -372,7 +374,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return err;
 	}
 
-	if (cl->qdisc->q.qlen == 1) {
+	if (first) {
 		list_add_tail(&cl->alist, &q->active);
 		cl->deficit = cl->quantum;
 	}
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 6bb8f73a8473..24cc220a3218 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1542,6 +1542,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 	unsigned int len = qdisc_pkt_len(skb);
 	struct hfsc_class *cl;
 	int uninitialized_var(err);
+	bool first;
 
 	cl = hfsc_classify(skb, sch, &err);
 	if (cl == NULL) {
@@ -1551,6 +1552,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 		return err;
 	}
 
+	first = !cl->qdisc->q.qlen;
 	err = qdisc_enqueue(skb, cl->qdisc, to_free);
 	if (unlikely(err != NET_XMIT_SUCCESS)) {
 		if (net_xmit_drop_count(err)) {
@@ -1560,7 +1562,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
 		return err;
 	}
 
-	if (cl->qdisc->q.qlen == 1) {
+	if (first) {
 		if (cl->cl_flags & HFSC_RSC)
 			init_ed(cl, len);
 		if (cl->cl_flags & HFSC_FSC)
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 8d5e55d5bed2..29f5c4a24688 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -1215,6 +1215,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	struct qfq_class *cl;
 	struct qfq_aggregate *agg;
 	int err = 0;
+	bool first;
 
 	cl = qfq_classify(skb, sch, &err);
 	if (cl == NULL) {
@@ -1236,6 +1237,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	}
 
 	gso_segs = skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1;
+	first = !cl->qdisc->q.qlen;
 	err = qdisc_enqueue(skb, cl->qdisc, to_free);
 	if (unlikely(err != NET_XMIT_SUCCESS)) {
 		pr_debug("qfq_enqueue: enqueue failed %d\n", err);
@@ -1253,7 +1255,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 
 	agg = cl->agg;
 	/* if the queue was not empty, then done here */
-	if (cl->qdisc->q.qlen != 1) {
+	if (!first) {
 		if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&
 		    list_first_entry(&agg->active, struct qfq_class, alist)
 		    == cl && cl->deficit < len)
-- 
2.20.1

Re: [Cake] [PATCH 2/4] sched: Fix detection of empty queues in child qdiscs

[Dave Taht]

awesome. I'd (rarely) seen this bug in drr and qfq and never solved
it, thus going for the all-in-one fq_codel....

On Mon, Jan 7, 2019 at 11:48 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Several qdiscs check on enqueue whether the packet was enqueued to a class
> with an empty queue, in which case the class is activated. This is done by
> checking if the qlen is exactly 1 after enqueue. However, if GSO splitting
> is enabled in the child qdisc, a single packet can result in a qlen longer
> than 1. This means the activation check fails, leading to a stalled queue.
>
> Fix this by checking if the queue is empty *before* enqueue, and running
> the activation logic if this was the case.
>
> Reported-by: Pete Heist <pete@heistp.net>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  net/sched/sch_drr.c  | 4 +++-
>  net/sched/sch_hfsc.c | 4 +++-
>  net/sched/sch_qfq.c  | 4 +++-
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> index feaf47178653..09b800991065 100644
> --- a/net/sched/sch_drr.c
> +++ b/net/sched/sch_drr.c
> @@ -354,6 +354,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>         struct drr_sched *q = qdisc_priv(sch);
>         struct drr_class *cl;
>         int err = 0;
> +       bool first;
>
>         cl = drr_classify(skb, sch, &err);
>         if (cl == NULL) {
> @@ -363,6 +364,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>                 return err;
>         }
>
> +       first = !cl->qdisc->q.qlen;
>         err = qdisc_enqueue(skb, cl->qdisc, to_free);
>         if (unlikely(err != NET_XMIT_SUCCESS)) {
>                 if (net_xmit_drop_count(err)) {
> @@ -372,7 +374,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>                 return err;
>         }
>
> -       if (cl->qdisc->q.qlen == 1) {
> +       if (first) {
>                 list_add_tail(&cl->alist, &q->active);
>                 cl->deficit = cl->quantum;
>         }
> diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> index 6bb8f73a8473..24cc220a3218 100644
> --- a/net/sched/sch_hfsc.c
> +++ b/net/sched/sch_hfsc.c
> @@ -1542,6 +1542,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
>         unsigned int len = qdisc_pkt_len(skb);
>         struct hfsc_class *cl;
>         int uninitialized_var(err);
> +       bool first;
>
>         cl = hfsc_classify(skb, sch, &err);
>         if (cl == NULL) {
> @@ -1551,6 +1552,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
>                 return err;
>         }
>
> +       first = !cl->qdisc->q.qlen;
>         err = qdisc_enqueue(skb, cl->qdisc, to_free);
>         if (unlikely(err != NET_XMIT_SUCCESS)) {
>                 if (net_xmit_drop_count(err)) {
> @@ -1560,7 +1562,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
>                 return err;
>         }
>
> -       if (cl->qdisc->q.qlen == 1) {
> +       if (first) {
>                 if (cl->cl_flags & HFSC_RSC)
>                         init_ed(cl, len);
>                 if (cl->cl_flags & HFSC_FSC)
> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
> index 8d5e55d5bed2..29f5c4a24688 100644
> --- a/net/sched/sch_qfq.c
> +++ b/net/sched/sch_qfq.c
> @@ -1215,6 +1215,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>         struct qfq_class *cl;
>         struct qfq_aggregate *agg;
>         int err = 0;
> +       bool first;
>
>         cl = qfq_classify(skb, sch, &err);
>         if (cl == NULL) {
> @@ -1236,6 +1237,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>         }
>
>         gso_segs = skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1;
> +       first = !cl->qdisc->q.qlen;
>         err = qdisc_enqueue(skb, cl->qdisc, to_free);
>         if (unlikely(err != NET_XMIT_SUCCESS)) {
>                 pr_debug("qfq_enqueue: enqueue failed %d\n", err);
> @@ -1253,7 +1255,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>
>         agg = cl->agg;
>         /* if the queue was not empty, then done here */
> -       if (cl->qdisc->q.qlen != 1) {
> +       if (!first) {
>                 if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&
>                     list_first_entry(&agg->active, struct qfq_class, alist)
>                     == cl && cl->deficit < len)
> --
> 2.20.1
>
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake



-- 

Dave Täht
CTO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-831-205-9740

[Cake] [PATCH 3/4] sch_api: Allow reducing queue backlog by a negative value

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

With GSO splitting in sch_cake, we can decrease as well as increase the
qlen. To make it possible to signal this up the qdisc tree, change
qdisc_tree_reduce_backlog() to accept signed integer values as the number
of packets and bytes to reduce the backlog by.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/sch_generic.h | 3 +--
 net/sched/sch_api.c       | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 9481f2c142e2..7a4957599874 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -580,8 +580,7 @@ struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue,
 void qdisc_reset(struct Qdisc *qdisc);
 void qdisc_put(struct Qdisc *qdisc);
 void qdisc_put_unlocked(struct Qdisc *qdisc);
-void qdisc_tree_reduce_backlog(struct Qdisc *qdisc, unsigned int n,
-			       unsigned int len);
+void qdisc_tree_reduce_backlog(struct Qdisc *qdisc, int n, int len);
 #ifdef CONFIG_NET_SCHED
 int qdisc_offload_dump_helper(struct Qdisc *q, enum tc_setup_type type,
 			      void *type_data);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 7e4d1ccf4c87..03e26e8d0ec9 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -758,8 +758,7 @@ static u32 qdisc_alloc_handle(struct net_device *dev)
 	return 0;
 }
 
-void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
-			       unsigned int len)
+void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
 {
 	bool qdisc_is_offloaded = sch->flags & TCQ_F_OFFLOADED;
 	const struct Qdisc_class_ops *cops;
-- 
2.20.1

Re: [Cake] [PATCH 3/4] sch_api: Allow reducing queue backlog by a negative value

[Eric Dumazet]

On 01/07/2019 11:47 AM, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> With GSO splitting in sch_cake, we can decrease as well as increase the
> qlen. To make it possible to signal this up the qdisc tree, change
> qdisc_tree_reduce_backlog() to accept signed integer values as the number
> of packets and bytes to reduce the backlog by.


Not sure why this patch is needed, given we use u32 integers
for q.qlen & qstats.backlog

0xFFFFFFFF for n is really -1 once folded...

sch->q.qlen -= n;
sch->qstats.backlog -= len;

Re: [Cake] [PATCH 3/4] sch_api: Allow reducing queue backlog by a negative value

[Toke Høiland-Jørgensen]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On 01/07/2019 11:47 AM, Toke Høiland-Jørgensen wrote:
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> With GSO splitting in sch_cake, we can decrease as well as increase the
>> qlen. To make it possible to signal this up the qdisc tree, change
>> qdisc_tree_reduce_backlog() to accept signed integer values as the number
>> of packets and bytes to reduce the backlog by.
>
>
> Not sure why this patch is needed, given we use u32 integers
> for q.qlen & qstats.backlog
>
> 0xFFFFFFFF for n is really -1 once folded...
>
> sch->q.qlen -= n;
> sch->qstats.backlog -= len;

Sure, it works without the patch, but changing to int properly
communicates the intent (that negative values are allowed) rather than
rely on overflow behaviour...

-Toke

[Cake] [PATCH 4/4] sch_cake: Correctly update parent qlen when splitting GSO packets

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

To ensure parent qdiscs have the same notion of the number of enqueued
packets even after splitting a GSO packet, update the qdisc tree with the
number of packets that was added due to the split.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/sched/sch_cake.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index b910cd5c56f7..73940293700d 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1667,7 +1667,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	if (skb_is_gso(skb) && q->rate_flags & CAKE_FLAG_SPLIT_GSO) {
 		struct sk_buff *segs, *nskb;
 		netdev_features_t features = netif_skb_features(skb);
-		unsigned int slen = 0;
+		unsigned int slen = 0, numsegs = 0;
 
 		segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
 		if (IS_ERR_OR_NULL(segs))
@@ -1683,6 +1683,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			flow_queue_add(flow, segs);
 
 			sch->q.qlen++;
+			numsegs++;
 			slen += segs->len;
 			q->buffer_used += segs->truesize;
 			b->packets++;
@@ -1696,7 +1697,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		sch->qstats.backlog += slen;
 		q->avg_window_bytes += slen;
 
-		qdisc_tree_reduce_backlog(sch, 1, len);
+		qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen);
 		consume_skb(skb);
 	} else {
 		/* not splitting */
-- 
2.20.1

Re: [Cake] [PATCH 0/4] sched: Fix qdisc interactions exposed by using sch_cake as a leaf qdisc

[David Miller]

From: Toke Høiland-Jørgensen <toke@toke.dk>
Date: Mon,  7 Jan 2019 20:47:29 +0100

> This series fixes a couple of issues exposed by running sch_cake as a
> leaf qdisc in an HFSC tree, which were discovered and reported by Pete
> Heist. The interaction between CAKE's GSO splitting and the parent
> qdisc's notion of its own queue length could cause queue stalls. While
> investigating the report, I also noticed that several qdiscs would
> dereference the skb pointer after dequeue, which is potentially
> problematic since the GSO splitting code also frees the original skb.
> 
> See the individual patches in the series for details.

Toke, can you please resubmit this without patch #3.

If you want to push for patch #3 still, it is much easier to submit
it separately.

Re: [Cake] [PATCH 0/4] sched: Fix qdisc interactions exposed by using sch_cake as a leaf qdisc

[Toke Høiland-Jørgensen]

David Miller <davem@davemloft.net> writes:

> From: Toke Høiland-Jørgensen <toke@toke.dk>
> Date: Mon,  7 Jan 2019 20:47:29 +0100
>
>> This series fixes a couple of issues exposed by running sch_cake as a
>> leaf qdisc in an HFSC tree, which were discovered and reported by Pete
>> Heist. The interaction between CAKE's GSO splitting and the parent
>> qdisc's notion of its own queue length could cause queue stalls. While
>> investigating the report, I also noticed that several qdiscs would
>> dereference the skb pointer after dequeue, which is potentially
>> problematic since the GSO splitting code also frees the original skb.
>> 
>> See the individual patches in the series for details.
>
> Toke, can you please resubmit this without patch #3.
>
> If you want to push for patch #3 still, it is much easier to submit
> it separately.

Can do :)

-Toke