Re: [PATCH] net/sched: reset block pointer in tcf_block_put()

[Konstantin Khlebnikov]

On 15.08.2017 00:15, Cong Wang wrote:

On Mon, Aug 14, 2017 at 5:59 AM, Konstantin Khlebnikov
 wrote:


This should work, I suppose.

But this approach requires careful review for all qdisc, mine is completely
mechanical.


Well, we don't have many classful qdisc's. Your patch actually
touches more qdisc's than mine, because you change an API, so
it is slightly harder to backport. ;)



Ok, I've fixed this right in qdiscs: [PATCH] net_sched: reset pointers to tcf 
blocks in classful qdiscs' destructors

Re: [PATCH] net/sched: reset block pointer in tcf_block_put()

[Cong Wang]

On Mon, Aug 14, 2017 at 5:59 AM, Konstantin Khlebnikov
 wrote:
>
> This should work, I suppose.
>
> But this approach requires careful review for all qdisc, mine is completely
> mechanical.

Well, we don't have many classful qdisc's. Your patch actually
touches more qdisc's than mine, because you change an API, so
it is slightly harder to backport. ;)

Re: [PATCH] net/sched: reset block pointer in tcf_block_put()

[Konstantin Khlebnikov]

On 12.08.2017 00:38, Cong Wang wrote:

On Fri, Aug 11, 2017 at 1:36 PM, Konstantin Khlebnikov
 wrote:



On 11.08.2017 23:18, Cong Wang wrote:


On Thu, Aug 10, 2017 at 2:31 AM, Konstantin Khlebnikov
 wrote:


In previous API tcf_destroy_chain() could be called several times and
some schedulers like hfsc and atm use that. In new API tcf_block_put()
frees block but leaves stale pointer, second call will free it once
again.



Which call path do we call tcf_block_put() for multiple times on
the same block? Please be specific, it is not obvious.



For example in hfsc_destroy_qdisc() since
a4aebb83cf0da0363684f1c339f7e6149a3e74c1
second time in hfsc_destroy_class() called from it.

Actually, I see the same pattern in all classy qdiscs.


Good find! But that means we can just move it up??
Something like (PoC only, not even compile):


This should work, I suppose.

But this approach requires careful review for all qdisc, mine is completely 
mechanical.



diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index b52f74610dc7..c7db8060e8ef 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1099,7 +1099,6 @@ hfsc_destroy_class(struct Qdisc *sch, struct
hfsc_class *cl)
  {
 struct hfsc_sched *q = qdisc_priv(sch);

-   tcf_block_put(cl->block);
 qdisc_destroy(cl->qdisc);
 gen_kill_estimator(>rate_est);
 if (cl != >root)
@@ -1243,8 +1242,10 @@ hfsc_put_class(struct Qdisc *sch, unsigned long arg)
  {
 struct hfsc_class *cl = (struct hfsc_class *)arg;

-   if (--cl->refcnt == 0)
+   if (--cl->refcnt == 0) {
+   tcf_block_put(cl->block);
 hfsc_destroy_class(sch, cl);
+   }
  }

  static unsigned long

Re: [PATCH] net/sched: reset block pointer in tcf_block_put()

[Cong Wang]

On Fri, Aug 11, 2017 at 1:36 PM, Konstantin Khlebnikov
 wrote:
>
>
> On 11.08.2017 23:18, Cong Wang wrote:
>>
>> On Thu, Aug 10, 2017 at 2:31 AM, Konstantin Khlebnikov
>>  wrote:
>>>
>>> In previous API tcf_destroy_chain() could be called several times and
>>> some schedulers like hfsc and atm use that. In new API tcf_block_put()
>>> frees block but leaves stale pointer, second call will free it once
>>> again.
>>
>>
>> Which call path do we call tcf_block_put() for multiple times on
>> the same block? Please be specific, it is not obvious.
>>
>
> For example in hfsc_destroy_qdisc() since
> a4aebb83cf0da0363684f1c339f7e6149a3e74c1
> second time in hfsc_destroy_class() called from it.
>
> Actually, I see the same pattern in all classy qdiscs.

Good find! But that means we can just move it up??
Something like (PoC only, not even compile):

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index b52f74610dc7..c7db8060e8ef 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1099,7 +1099,6 @@ hfsc_destroy_class(struct Qdisc *sch, struct
hfsc_class *cl)
 {
struct hfsc_sched *q = qdisc_priv(sch);

-   tcf_block_put(cl->block);
qdisc_destroy(cl->qdisc);
gen_kill_estimator(>rate_est);
if (cl != >root)
@@ -1243,8 +1242,10 @@ hfsc_put_class(struct Qdisc *sch, unsigned long arg)
 {
struct hfsc_class *cl = (struct hfsc_class *)arg;

-   if (--cl->refcnt == 0)
+   if (--cl->refcnt == 0) {
+   tcf_block_put(cl->block);
hfsc_destroy_class(sch, cl);
+   }
 }

 static unsigned long

Re: [PATCH] net/sched: reset block pointer in tcf_block_put()

[David Miller]

From: Cong Wang 
Date: Fri, 11 Aug 2017 14:06:31 -0700

> On Fri, Aug 11, 2017 at 1:32 PM, Florian Westphal  wrote:
>> Cong Wang  wrote:
>>> On Thu, Aug 10, 2017 at 2:31 AM, Konstantin Khlebnikov
>>>  wrote:
>>> > In previous API tcf_destroy_chain() could be called several times and
>>> > some schedulers like hfsc and atm use that. In new API tcf_block_put()
>>> > frees block but leaves stale pointer, second call will free it once again.
>>>
>>> Which call path do we call tcf_block_put() for multiple times on
>>> the same block? Please be specific, it is not obvious.
>>
>> you can use tools/testing/selftests/net/rtnetlink.sh to reproduce this
>> (kernel panics on delete of root qdisc).
> 
> I am sure this is not how changelog works. We have enough space
> in changelog to describe a bug, don't have to leave details in a
> following-up email which will almost surely be lost in history.

Yeah, the more information in the commit log message the better.

Re: [PATCH] net/sched: reset block pointer in tcf_block_put()

[Cong Wang]

On Fri, Aug 11, 2017 at 1:32 PM, Florian Westphal  wrote:
> Cong Wang  wrote:
>> On Thu, Aug 10, 2017 at 2:31 AM, Konstantin Khlebnikov
>>  wrote:
>> > In previous API tcf_destroy_chain() could be called several times and
>> > some schedulers like hfsc and atm use that. In new API tcf_block_put()
>> > frees block but leaves stale pointer, second call will free it once again.
>>
>> Which call path do we call tcf_block_put() for multiple times on
>> the same block? Please be specific, it is not obvious.
>
> you can use tools/testing/selftests/net/rtnetlink.sh to reproduce this
> (kernel panics on delete of root qdisc).

I am sure this is not how changelog works. We have enough space
in changelog to describe a bug, don't have to leave details in a
following-up email which will almost surely be lost in history.

Re: [PATCH] net/sched: reset block pointer in tcf_block_put()

[Konstantin Khlebnikov]

On 11.08.2017 23:18, Cong Wang wrote:

On Thu, Aug 10, 2017 at 2:31 AM, Konstantin Khlebnikov
 wrote:

In previous API tcf_destroy_chain() could be called several times and
some schedulers like hfsc and atm use that. In new API tcf_block_put()
frees block but leaves stale pointer, second call will free it once again.


Which call path do we call tcf_block_put() for multiple times on
the same block? Please be specific, it is not obvious.



For example in hfsc_destroy_qdisc() since 
a4aebb83cf0da0363684f1c339f7e6149a3e74c1
second time in hfsc_destroy_class() called from it.

Actually, I see the same pattern in all classy qdiscs.

Re: [PATCH] net/sched: reset block pointer in tcf_block_put()

[Florian Westphal]

Cong Wang  wrote:
> On Thu, Aug 10, 2017 at 2:31 AM, Konstantin Khlebnikov
>  wrote:
> > In previous API tcf_destroy_chain() could be called several times and
> > some schedulers like hfsc and atm use that. In new API tcf_block_put()
> > frees block but leaves stale pointer, second call will free it once again.
> 
> Which call path do we call tcf_block_put() for multiple times on
> the same block? Please be specific, it is not obvious.

you can use tools/testing/selftests/net/rtnetlink.sh to reproduce this
(kernel panics on delete of root qdisc).

Re: [PATCH] net/sched: reset block pointer in tcf_block_put()

[Cong Wang]

On Thu, Aug 10, 2017 at 2:31 AM, Konstantin Khlebnikov
 wrote:
> In previous API tcf_destroy_chain() could be called several times and
> some schedulers like hfsc and atm use that. In new API tcf_block_put()
> frees block but leaves stale pointer, second call will free it once again.

Which call path do we call tcf_block_put() for multiple times on
the same block? Please be specific, it is not obvious.

[PATCH] net/sched: reset block pointer in tcf_block_put()

[Konstantin Khlebnikov]

In previous API tcf_destroy_chain() could be called several times and
some schedulers like hfsc and atm use that. In new API tcf_block_put()
frees block but leaves stale pointer, second call will free it once again.

This patch fixes such double-frees.

Signed-off-by: Konstantin Khlebnikov 
Fixes: 6529eaba33f0 ("net: sched: introduce tcf block infractructure")
---
 include/net/pkt_cls.h|4 ++--
 net/sched/cls_api.c  |4 +++-
 net/sched/sch_atm.c  |4 ++--
 net/sched/sch_cbq.c  |6 +++---
 net/sched/sch_drr.c  |2 +-
 net/sched/sch_dsmark.c   |2 +-
 net/sched/sch_fq_codel.c |2 +-
 net/sched/sch_hfsc.c |6 +++---
 net/sched/sch_htb.c  |8 
 net/sched/sch_ingress.c  |6 +++---
 net/sched/sch_multiq.c   |2 +-
 net/sched/sch_prio.c |2 +-
 net/sched/sch_qfq.c  |2 +-
 net/sched/sch_sfb.c  |2 +-
 net/sched/sch_sfq.c  |2 +-
 15 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 537d0a0ad4c4..96aef16e5d56 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -23,7 +23,7 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 
chain_index,
 void tcf_chain_put(struct tcf_chain *chain);
 int tcf_block_get(struct tcf_block **p_block,
  struct tcf_proto __rcu **p_filter_chain);
-void tcf_block_put(struct tcf_block *block);
+void tcf_block_put(struct tcf_block **p_block);
 int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 struct tcf_result *res, bool compat_mode);
 
@@ -35,7 +35,7 @@ int tcf_block_get(struct tcf_block **p_block,
return 0;
 }
 
-static inline void tcf_block_put(struct tcf_block *block)
+static inline void tcf_block_put(struct tcf_block **p_block)
 {
 }
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 39da0c5801c9..72147650b179 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -281,8 +281,9 @@ int tcf_block_get(struct tcf_block **p_block,
 }
 EXPORT_SYMBOL(tcf_block_get);
 
-void tcf_block_put(struct tcf_block *block)
+void tcf_block_put(struct tcf_block **p_block)
 {
+   struct tcf_block *block = *p_block;
struct tcf_chain *chain, *tmp;
 
if (!block)
@@ -291,6 +292,7 @@ void tcf_block_put(struct tcf_block *block)
list_for_each_entry_safe(chain, tmp, >chain_list, list)
tcf_chain_destroy(chain);
kfree(block);
+   *p_block = NULL;
 }
 EXPORT_SYMBOL(tcf_block_put);
 
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index 572fe2584e48..b6b24a9834b0 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -144,7 +144,7 @@ static void atm_tc_put(struct Qdisc *sch, unsigned long cl)
list_del_init(>list);
pr_debug("atm_tc_put: qdisc %p\n", flow->q);
qdisc_destroy(flow->q);
-   tcf_block_put(flow->block);
+   tcf_block_put(>block);
if (flow->sock) {
pr_debug("atm_tc_put: f_count %ld\n",
file_count(flow->sock->file));
@@ -573,7 +573,7 @@ static void atm_tc_destroy(struct Qdisc *sch)
 
pr_debug("atm_tc_destroy(sch %p,[qdisc %p])\n", sch, p);
list_for_each_entry(flow, >flows, list)
-   tcf_block_put(flow->block);
+   tcf_block_put(>block);
 
list_for_each_entry_safe(flow, tmp, >flows, list) {
if (flow->ref > 1)
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 481036f6b54e..89bff8d4e113 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1407,7 +1407,7 @@ static void cbq_destroy_class(struct Qdisc *sch, struct 
cbq_class *cl)
 
WARN_ON(cl->filters);
 
-   tcf_block_put(cl->block);
+   tcf_block_put(>block);
qdisc_destroy(cl->q);
qdisc_put_rtab(cl->R_tab);
gen_kill_estimator(>rate_est);
@@ -1432,7 +1432,7 @@ static void cbq_destroy(struct Qdisc *sch)
 */
for (h = 0; h < q->clhash.hashsize; h++) {
hlist_for_each_entry(cl, >clhash.hash[h], common.hnode)
-   tcf_block_put(cl->block);
+   tcf_block_put(>block);
}
for (h = 0; h < q->clhash.hashsize; h++) {
hlist_for_each_entry_safe(cl, next, >clhash.hash[h],
@@ -1599,7 +1599,7 @@ cbq_change_class(struct Qdisc *sch, u32 classid, u32 
parentid, struct nlattr **t
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
if (err) {
-   tcf_block_put(cl->block);
+   tcf_block_put(>block);
kfree(cl);
goto failure;
}
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index a413dc1c2098..fb73a903ab74 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -466,7 +466,7 @@ static void drr_destroy_qdisc(struct Qdisc *sch)