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

2015-07-21 Thread Cong Wang 
On Tue, Jul 21, 2015 at 3:52 AM, Eric Dumazet eric.duma...@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.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-07-21 Thread Cong Wang 
On Tue, Jul 21, 2015 at 1:57 PM, Eric Dumazet eric.duma...@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?
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-07-21 Thread 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.duma...@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.



--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-07-21 Thread 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.


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-07-21 Thread 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: 8823c990d280  CPU: 14  COMMAND: tc
[... snip ...]
#8 [8820ceec17a0] page_fault at 8160a8c2
   [exception RIP: htb_qlen_notify+24]
   RIP: a0841718  RSP: 8820ceec1858  RFLAGS: 00010282
   RAX:   RBX:   RCX: 88241747b400
   RDX: 88241747b408  RSI:   RDI: 8811fb27d000
   RBP: 8820ceec1868   R8: 88120cdeff24   R9: 88120cdeff30
   R10: 0bd4  R11: a0840919  R12: a0843340
   R13:   R14: 0001  R15: 8808dae5c2e8
   ORIG_RAX:   CS: 0010  SS: 0018
#9 [...] qdisc_tree_decrease_qlen at 81565375
   #10 [...] fq_codel_dequeue at a084e0a0 [sch_fq_codel]
   #11 [...] fq_codel_reset at a084e2f8 [sch_fq_codel]
   #12 [...] qdisc_destroy at 81560d2d
   #13 [...] htb_destroy_class at a08408f8 [sch_htb]
   #14 [...] htb_put at a084095c [sch_htb]
   #15 [...] tc_ctl_tclass at 815645a3
   #16 [...] rtnetlink_rcv_msg at 81552cb0
   [... 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
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-07-21 Thread 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] = {




--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-07-20 Thread 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: 8823c990d280  CPU: 14  COMMAND: tc
   [... snip ...]
   #8 [8820ceec17a0] page_fault at 8160a8c2
  [exception RIP: htb_qlen_notify+24]
  RIP: a0841718  RSP: 8820ceec1858  RFLAGS: 00010282
  RAX:   RBX:   RCX: 88241747b400
  RDX: 88241747b408  RSI:   RDI: 8811fb27d000
  RBP: 8820ceec1868   R8: 88120cdeff24   R9: 88120cdeff30
  R10: 0bd4  R11: a0840919  R12: a0843340
  R13:   R14: 0001  R15: 8808dae5c2e8
  ORIG_RAX:   CS: 0010  SS: 0018
   #9 [...] qdisc_tree_decrease_qlen at 81565375
  #10 [...] fq_codel_dequeue at a084e0a0 [sch_fq_codel]
  #11 [...] fq_codel_reset at a084e2f8 [sch_fq_codel]
  #12 [...] qdisc_destroy at 81560d2d
  #13 [...] htb_destroy_class at a08408f8 [sch_htb]
  #14 [...] htb_put at a084095c [sch_htb]
  #15 [...] tc_ctl_tclass at 815645a3
  #16 [...] rtnetlink_rcv_msg at 81552cb0
  [... 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 agartr...@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 agartr...@fb.com

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html