Skip to content

Commit

Permalink
Merge branch 'sch_cake-leaf-qdisc-fixes'
Browse files Browse the repository at this point in the history
Toke Høiland-Jørgensen says:

====================
sched: Fix qdisc interactions exposed by using sch_cake as a leaf qdisc

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

Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
David S. Miller committed Jan 16, 2019
2 parents 80b3671 + 8c6c37f commit d62f38c
Show file tree
Hide file tree
Showing 9 changed files with 35 additions and 21 deletions.
5 changes: 3 additions & 2 deletions net/sched/sch_cake.c
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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++;
Expand All @@ -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 */
Expand Down
3 changes: 2 additions & 1 deletion net/sched/sch_cbs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 5 additions & 2 deletions net/sched/sch_drr.c
Original file line number Diff line number Diff line change
Expand Up @@ -350,9 +350,11 @@ 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;
bool first;

cl = drr_classify(skb, sch, &err);
if (cl == NULL) {
Expand All @@ -362,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)) {
Expand All @@ -371,12 +374,12 @@ 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;
}

qdisc_qstats_backlog_inc(sch, skb);
sch->qstats.backlog += len;
sch->q.qlen++;
return err;
}
Expand Down
3 changes: 2 additions & 1 deletion net/sched/sch_dsmark.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down
9 changes: 5 additions & 4 deletions net/sched/sch_hfsc.c
Original file line number Diff line number Diff line change
Expand Up @@ -1539,8 +1539,10 @@ 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);
bool first;

cl = hfsc_classify(skb, sch, &err);
if (cl == NULL) {
Expand All @@ -1550,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)) {
Expand All @@ -1559,9 +1562,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
return err;
}

if (cl->qdisc->q.qlen == 1) {
unsigned int len = qdisc_pkt_len(skb);

if (first) {
if (cl->cl_flags & HFSC_RSC)
init_ed(cl, len);
if (cl->cl_flags & HFSC_FSC)
Expand All @@ -1576,7 +1577,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;
Expand Down
3 changes: 2 additions & 1 deletion net/sched/sch_htb.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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;
}
Expand Down
3 changes: 2 additions & 1 deletion net/sched/sch_prio.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
}
Expand Down
20 changes: 12 additions & 8 deletions net/sched/sch_qfq.c
Original file line number Diff line number Diff line change
Expand Up @@ -1210,10 +1210,12 @@ 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;
int err = 0;
bool first;

cl = qfq_classify(skb, sch, &err);
if (cl == NULL) {
Expand All @@ -1224,17 +1226,18 @@ 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;
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);
Expand All @@ -1245,16 +1248,17 @@ 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;
/* 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 < qdisc_pkt_len(skb))
== cl && cl->deficit < len)
list_move_tail(&cl->alist, &agg->active);

return err;
Expand Down
3 changes: 2 additions & 1 deletion net/sched/sch_tbf.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down

0 comments on commit d62f38c

Please sign in to comment.