[PATCH,v2 net] net: sched: validate that class is found in qdisc_tree_decrease_qlen

[PATCH,v2 net] net: sched: validate that class is found in qdisc_tree_decrease_qlen

[Alex Gartrell]

We have an application that invokes tc to delete the root every time the
config changes. As a result we stress the cleanup code and were seeing the
following panic:

  crash> bt
  PID: 630839  TASK: ffff8823c990d280  CPU: 14  COMMAND: "tc"
   [... snip ...]
   #8 [ffff8820ceec17a0] page_fault at ffffffff8160a8c2
      [exception RIP: htb_qlen_notify+24]
      RIP: ffffffffa0841718  RSP: ffff8820ceec1858  RFLAGS: 00010282
      RAX: 0000000000000000  RBX: 0000000000000000  RCX: ffff88241747b400
      RDX: ffff88241747b408  RSI: 0000000000000000  RDI: ffff8811fb27d000
      RBP: ffff8820ceec1868   R8: ffff88120cdeff24   R9: ffff88120cdeff30
      R10: 0000000000000bd4  R11: ffffffffa0840919  R12: ffffffffa0843340
      R13: 0000000000000000  R14: 0000000000000001  R15: ffff8808dae5c2e8
      ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
   #9 [...] qdisc_tree_decrease_qlen at ffffffff81565375
  #10 [...] fq_codel_dequeue at ffffffffa084e0a0 [sch_fq_codel]
  #11 [...] fq_codel_reset at ffffffffa084e2f8 [sch_fq_codel]
  #12 [...] qdisc_destroy at ffffffff81560d2d
  #13 [...] htb_destroy_class at ffffffffa08408f8 [sch_htb]
  #14 [...] htb_put at ffffffffa084095c [sch_htb]
  #15 [...] tc_ctl_tclass at ffffffff815645a3
  #16 [...] rtnetlink_rcv_msg at ffffffff81552cb0
  [... snip ...]

To my understanding, the following situation is taking place.

  tc_ctl_tclass
   -> htb_delete
     -> class is deleted from clhash
   -> htb_put
     -> qdisc_destroy
       -> fq_codel_reset
         -> fq_codel_dequeue
           -> qdidsc_tree_decrease_qlen
             -> cl = htb_get # returns NULL, removed in htb_delete
               -> htb_qlen_notify(sch, NULL) # BOOM

This patch checks cl for 0 before invoking qlen_notify and put.  The notify
is not necessary in this case, as the parent has already been deactivated
anyway.

Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
 net/sched/sch_api.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f06aa01..46be5e5 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -762,8 +762,15 @@ void qdisc_tree_decrease_qlen(struct Qdisc *sch, unsigned int n)
 		cops = sch->ops->cl_ops;
 		if (cops->qlen_notify) {
 			cl = cops->get(sch, parentid);
-			cops->qlen_notify(sch, cl);
-			cops->put(sch, cl);
+			/* This will be 0 in the event that cl is being
+			 * destroyed, in which case we should not attempt
+			 * to invoke qlen_notify upon it (it must be
+			 * cleaned up otherwise anyway.
+			 */
+			if (cl != 0) {
+				cops->qlen_notify(sch, cl);
+				cops->put(sch, cl);
+			}
 		}
 		sch->q.qlen -= n;
 		__qdisc_qstats_drop(sch, drops);
-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH,v2 net] net: sched: validate that class is found in qdisc_tree_decrease_qlen

[Jamal Hadi Salim]

On 07/20/15 15:40, Alex Gartrell wrote:
> We have an application that invokes tc to delete the root every time the
> config changes. As a result we stress the cleanup code and were seeing the
> following panic:
>
>    crash> bt
>    PID: 630839  TASK: ffff8823c990d280  CPU: 14  COMMAND: "tc"
>     [... snip ...]
>     #8 [ffff8820ceec17a0] page_fault at ffffffff8160a8c2
>        [exception RIP: htb_qlen_notify+24]
>        RIP: ffffffffa0841718  RSP: ffff8820ceec1858  RFLAGS: 00010282
>        RAX: 0000000000000000  RBX: 0000000000000000  RCX: ffff88241747b400
>        RDX: ffff88241747b408  RSI: 0000000000000000  RDI: ffff8811fb27d000
>        RBP: ffff8820ceec1868   R8: ffff88120cdeff24   R9: ffff88120cdeff30
>        R10: 0000000000000bd4  R11: ffffffffa0840919  R12: ffffffffa0843340
>        R13: 0000000000000000  R14: 0000000000000001  R15: ffff8808dae5c2e8
>        ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
>     #9 [...] qdisc_tree_decrease_qlen at ffffffff81565375
>    #10 [...] fq_codel_dequeue at ffffffffa084e0a0 [sch_fq_codel]
>    #11 [...] fq_codel_reset at ffffffffa084e2f8 [sch_fq_codel]
>    #12 [...] qdisc_destroy at ffffffff81560d2d
>    #13 [...] htb_destroy_class at ffffffffa08408f8 [sch_htb]
>    #14 [...] htb_put at ffffffffa084095c [sch_htb]
>    #15 [...] tc_ctl_tclass at ffffffff815645a3
>    #16 [...] rtnetlink_rcv_msg at ffffffff81552cb0
>    [... snip ...]
>
> To my understanding, the following situation is taking place.
>

>    tc_ctl_tclass

>     -> htb_delete
>       -> class is deleted from clhash
>     -> htb_put
>       -> qdisc_destroy
>         -> fq_codel_reset

=========> this part looks suspicious. Why is reset invoking
a dequeue? Shouldnt a destroy just purge the queue?

>           -> fq_codel_dequeue
>             -> qdidsc_tree_decrease_qlen
>               -> cl = htb_get # returns NULL, removed in htb_delete
>                 -> htb_qlen_notify(sch, NULL) # BOOM
>

It is worrisome to fix the core code for this. The root cause seems to
be codel. Dont have time but in general, reset would be something like:

struct fq_codel_sched_data *q = qdisc_priv(sch);
qdisc_reset(q)

or something along those lines...
But certainly dequeue semantics dont seem right there..

cheers,
jamal



cheers,
jamal

Re: [PATCH,v2 net] net: sched: validate that class is found in qdisc_tree_decrease_qlen

[Eric Dumazet]

On Tue, 2015-07-21 at 06:04 -0400, Jamal Hadi Salim wrote:

> It is worrisome to fix the core code for this. The root cause seems to
> be codel. Dont have time but in general, reset would be something like:
> 
> struct fq_codel_sched_data *q = qdisc_priv(sch);
> qdisc_reset(q)

This only works for very simple qdisc with one queue.

> 
> or something along those lines...
> But certainly dequeue semantics dont seem right there..

Well, reset() is trivial to implement like this

while (skb = local_dequeue(sch)) {
	kfree_skb(skb);
}

And I guess I copy/pasted sfq code here, because I was lazy.

But yes, qdisc_tree_decrease_qlen() would have to be not called.

It seems I coded fq_reset() differently.

Alex, please try instead :

diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 21ca33c9f036..3f0320ab6029 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -288,10 +288,21 @@ begin:
 
 static void fq_codel_reset(struct Qdisc *sch)
 {
-	struct sk_buff *skb;
+	struct fq_codel_sched_data *q = qdisc_priv(sch);
+	int i;
 
-	while ((skb = fq_codel_dequeue(sch)) != NULL)
-		kfree_skb(skb);
+	INIT_LIST_HEAD(&q->new_flows);
+	INIT_LIST_HEAD(&q->old_flows);
+	for (i = 0; i < q->flows_cnt; i++) {
+		struct fq_codel_flow *flow = q->flows + i;
+
+		while (flow->head)
+			kfree_skb(dequeue_head(flow));
+
+		INIT_LIST_HEAD(&flow->flowchain);
+	}
+	memset(q->backlogs, 0, q->flows_cnt * sizeof(u32));
+	sch->q.qlen = 0;
 }
 
 static const struct nla_policy fq_codel_policy[TCA_FQ_CODEL_MAX + 1] = {

Re: [PATCH,v2 net] net: sched: validate that class is found in qdisc_tree_decrease_qlen

[Cong Wang]

On Tue, Jul 21, 2015 at 3:52 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2015-07-21 at 06:04 -0400, Jamal Hadi Salim wrote:
>
>> It is worrisome to fix the core code for this. The root cause seems to
>> be codel. Dont have time but in general, reset would be something like:
>>
>> struct fq_codel_sched_data *q = qdisc_priv(sch);
>> qdisc_reset(q)
>
> This only works for very simple qdisc with one queue.
>
>>
>> or something along those lines...
>> But certainly dequeue semantics dont seem right there..
>
> Well, reset() is trivial to implement like this
>
> while (skb = local_dequeue(sch)) {
>         kfree_skb(skb);
> }
>
> And I guess I copy/pasted sfq code here, because I was lazy.
>
> But yes, qdisc_tree_decrease_qlen() would have to be not called.


Hmm, so the semantic is each qdisc resets qlen for its own
and calls qdisc_reset() to reset its leaf qdisc's, that makes sense
for me.

>
> It seems I coded fq_reset() differently.
>
> Alex, please try instead :
>
> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> index 21ca33c9f036..3f0320ab6029 100644
> --- a/net/sched/sch_fq_codel.c
> +++ b/net/sched/sch_fq_codel.c
> @@ -288,10 +288,21 @@ begin:
>
>  static void fq_codel_reset(struct Qdisc *sch)
>  {
> -       struct sk_buff *skb;
> +       struct fq_codel_sched_data *q = qdisc_priv(sch);
> +       int i;
>
> -       while ((skb = fq_codel_dequeue(sch)) != NULL)
> -               kfree_skb(skb);
> +       INIT_LIST_HEAD(&q->new_flows);
> +       INIT_LIST_HEAD(&q->old_flows);
> +       for (i = 0; i < q->flows_cnt; i++) {
> +               struct fq_codel_flow *flow = q->flows + i;
> +
> +               while (flow->head)
> +                       kfree_skb(dequeue_head(flow));
> +
> +               INIT_LIST_HEAD(&flow->flowchain);


You probably need to call codel_vars_init(&flow->cvars) as well.

> +       }
> +       memset(q->backlogs, 0, q->flows_cnt * sizeof(u32));
> +       sch->q.qlen = 0;
>  }
>
>  static const struct nla_policy fq_codel_policy[TCA_FQ_CODEL_MAX + 1] = {
>
>
>

Thanks.

Re: [PATCH,v2 net] net: sched: validate that class is found in qdisc_tree_decrease_qlen

[Eric Dumazet]

On Tue, 2015-07-21 at 11:12 -0700, Cong Wang wrote:

> > -               kfree_skb(skb);
> > +       INIT_LIST_HEAD(&q->new_flows);
> > +       INIT_LIST_HEAD(&q->old_flows);
> > +       for (i = 0; i < q->flows_cnt; i++) {
> > +               struct fq_codel_flow *flow = q->flows + i;
> > +
> > +               while (flow->head)
> > +                       kfree_skb(dequeue_head(flow));
> > +
> > +               INIT_LIST_HEAD(&flow->flowchain);
> 
> 
> You probably need to call codel_vars_init(&flow->cvars) as well.

It is not necessary : flow->cvars only matter in the event of a dequeue,
but whole qdisc is dismantled and no packet will be dequeued.

Re: [PATCH,v2 net] net: sched: validate that class is found in qdisc_tree_decrease_qlen

[Cong Wang]

On Tue, Jul 21, 2015 at 1:57 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2015-07-21 at 11:12 -0700, Cong Wang wrote:
>
>> > -               kfree_skb(skb);
>> > +       INIT_LIST_HEAD(&q->new_flows);
>> > +       INIT_LIST_HEAD(&q->old_flows);
>> > +       for (i = 0; i < q->flows_cnt; i++) {
>> > +               struct fq_codel_flow *flow = q->flows + i;
>> > +
>> > +               while (flow->head)
>> > +                       kfree_skb(dequeue_head(flow));
>> > +
>> > +               INIT_LIST_HEAD(&flow->flowchain);
>>
>>
>> You probably need to call codel_vars_init(&flow->cvars) as well.
>
> It is not necessary : flow->cvars only matter in the event of a dequeue,
> but whole qdisc is dismantled and no packet will be dequeued.
>

But it will affect the next dequeue _after_ reset? which is not supposed
to happen as we expect a fresh start after reset?

Re: [PATCH,v2 net] net: sched: validate that class is found in qdisc_tree_decrease_qlen

[Eric Dumazet]

On Tue, 2015-07-21 at 19:03 -0700, Cong Wang wrote:
> On Tue, Jul 21, 2015 at 1:57 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Tue, 2015-07-21 at 11:12 -0700, Cong Wang wrote:
> >
> >> > -               kfree_skb(skb);
> >> > +       INIT_LIST_HEAD(&q->new_flows);
> >> > +       INIT_LIST_HEAD(&q->old_flows);
> >> > +       for (i = 0; i < q->flows_cnt; i++) {
> >> > +               struct fq_codel_flow *flow = q->flows + i;
> >> > +
> >> > +               while (flow->head)
> >> > +                       kfree_skb(dequeue_head(flow));
> >> > +
> >> > +               INIT_LIST_HEAD(&flow->flowchain);
> >>
> >>
> >> You probably need to call codel_vars_init(&flow->cvars) as well.
> >
> > It is not necessary : flow->cvars only matter in the event of a dequeue,
> > but whole qdisc is dismantled and no packet will be dequeued.
> >
> 
> But it will affect the next dequeue _after_ reset? which is not supposed
> to happen as we expect a fresh start after reset?

Hmm... I thought reset() was only done at queue dismantle, so no new
packet should be added later, and since no packet should be left after
reset, no dequeue should happen.

For completeness, we still can add the codel_vars_init(), no problem.

Thanks.