[patch net-next 1/2] net/sched: properly assign RCU pointer in tcf_chain_tp_insert/remove

[patch net-next 1/2] net/sched: properly assign RCU pointer in tcf_chain_tp_insert/remove

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

*p_filter_chain is rcu-dereferenced on reader path. So here in writer,
property assign the pointer.

Fixes: 2190d1d0944f ("net: sched: introduce helpers to work with filter chains")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_api.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 4020b8d..85088ed 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -351,7 +351,7 @@ static void tcf_chain_tp_insert(struct tcf_chain *chain,
 {
 	if (chain->p_filter_chain &&
 	    *chain_info->pprev == chain->filter_chain)
-		*chain->p_filter_chain = tp;
+		rcu_assign_pointer(*chain->p_filter_chain, tp);
 	RCU_INIT_POINTER(tp->next, tcf_chain_tp_prev(chain_info));
 	rcu_assign_pointer(*chain_info->pprev, tp);
 }
@@ -363,7 +363,7 @@ static void tcf_chain_tp_remove(struct tcf_chain *chain,
 	struct tcf_proto *next = rtnl_dereference(chain_info->next);
 
 	if (chain->p_filter_chain && tp == chain->filter_chain)
-		*chain->p_filter_chain = next;
+		RCU_INIT_POINTER(*chain->p_filter_chain, next);
 	RCU_INIT_POINTER(*chain_info->pprev, next);
 }
 
-- 
2.9.3

[patch net-next 2/2] net/sched: fix filter flushing

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

When user instructs to remove all filters from chain, we cannot destroy
the chain as other actions may hold a reference. Also the put in errout
would try to destroy it again. So instead, just walk the chain and remove
all existing filters.

Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_api.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 85088ed..01a8b8b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -201,15 +201,22 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
 	return chain;
 }
 
-static void tcf_chain_destroy(struct tcf_chain *chain)
+static void tcf_chain_flush(struct tcf_chain *chain)
 {
 	struct tcf_proto *tp;
 
-	list_del(&chain->list);
+	if (*chain->p_filter_chain)
+		RCU_INIT_POINTER(*chain->p_filter_chain, NULL);
 	while ((tp = rtnl_dereference(chain->filter_chain)) != NULL) {
 		RCU_INIT_POINTER(chain->filter_chain, tp->next);
 		tcf_proto_destroy(tp);
 	}
+}
+
+static void tcf_chain_destroy(struct tcf_chain *chain)
+{
+	list_del(&chain->list);
+	tcf_chain_flush(chain);
 	kfree(chain);
 }
 
@@ -510,7 +517,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
 
 	if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
 		tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
-		tcf_chain_destroy(chain);
+		tcf_chain_flush(chain);
 		err = 0;
 		goto errout;
 	}
-- 
2.9.3

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Cong Wang]

On Sat, May 20, 2017 at 6:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> +static void tcf_chain_destroy(struct tcf_chain *chain)
> +{
> +       list_del(&chain->list);
> +       tcf_chain_flush(chain);
>         kfree(chain);
>  }
>
> @@ -510,7 +517,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>
>         if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
>                 tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
> -               tcf_chain_destroy(chain);
> +               tcf_chain_flush(chain);


I wonder if we should return EBUSY and do nothing in case of busy?
The chain is no longer visual to new actions after your list_del(), but
the old one could still use and see it.

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Jiri Pirko]

Sun, May 21, 2017 at 02:16:45AM CEST, xiyou.wangcong@gmail.com wrote:
>On Sat, May 20, 2017 at 6:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> +static void tcf_chain_destroy(struct tcf_chain *chain)
>> +{
>> +       list_del(&chain->list);
>> +       tcf_chain_flush(chain);
>>         kfree(chain);
>>  }
>>
>> @@ -510,7 +517,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>
>>         if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
>>                 tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
>> -               tcf_chain_destroy(chain);
>> +               tcf_chain_flush(chain);
>
>
>I wonder if we should return EBUSY and do nothing in case of busy?
>The chain is no longer visual to new actions after your list_del(), but
>the old one could still use and see it.

No. User request to flush the chain, that is what happens in the past
and that is what should happen now.
If there is still a reference, the chain_put will keep the empty chain.
If there is no longer a reference, chain_put will destroy the chain. All
good.

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Cong Wang]

On Sat, May 20, 2017 at 10:54 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Sun, May 21, 2017 at 02:16:45AM CEST, xiyou.wangcong@gmail.com wrote:
>>On Sat, May 20, 2017 at 6:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> +static void tcf_chain_destroy(struct tcf_chain *chain)
>>> +{
>>> +       list_del(&chain->list);
>>> +       tcf_chain_flush(chain);
>>>         kfree(chain);
>>>  }
>>>
>>> @@ -510,7 +517,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>>
>>>         if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
>>>                 tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
>>> -               tcf_chain_destroy(chain);
>>> +               tcf_chain_flush(chain);
>>
>>
>>I wonder if we should return EBUSY and do nothing in case of busy?
>>The chain is no longer visual to new actions after your list_del(), but
>>the old one could still use and see it.
>
> No. User request to flush the chain, that is what happens in the past
> and that is what should happen now.
> If there is still a reference, the chain_put will keep the empty chain.

But if you dump the actions, this chain is still shown "goto chain"?
You can't claim you really delete it as long as actions can still
see it and dump it.

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Jiri Pirko]

Sun, May 21, 2017 at 08:27:21PM CEST, xiyou.wangcong@gmail.com wrote:
>On Sat, May 20, 2017 at 10:54 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Sun, May 21, 2017 at 02:16:45AM CEST, xiyou.wangcong@gmail.com wrote:
>>>On Sat, May 20, 2017 at 6:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> +static void tcf_chain_destroy(struct tcf_chain *chain)
>>>> +{
>>>> +       list_del(&chain->list);
>>>> +       tcf_chain_flush(chain);
>>>>         kfree(chain);
>>>>  }
>>>>
>>>> @@ -510,7 +517,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>>>
>>>>         if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
>>>>                 tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
>>>> -               tcf_chain_destroy(chain);
>>>> +               tcf_chain_flush(chain);
>>>
>>>
>>>I wonder if we should return EBUSY and do nothing in case of busy?
>>>The chain is no longer visual to new actions after your list_del(), but
>>>the old one could still use and see it.
>>
>> No. User request to flush the chain, that is what happens in the past
>> and that is what should happen now.
>> If there is still a reference, the chain_put will keep the empty chain.
>
>But if you dump the actions, this chain is still shown "goto chain"?

Yes, it will be shown there.


>You can't claim you really delete it as long as actions can still
>see it and dump it.

No, user just wants to delete all the filters. That is done. User does
not care if the actual chain structure is there or not.

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Jamal Hadi Salim]

On 17-05-21 03:19 PM, Jiri Pirko wrote:
> Sun, May 21, 2017 at 08:27:21PM CEST, xiyou.wangcong@gmail.com wrote:
>> On Sat, May 20, 2017 at 10:54 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Sun, May 21, 2017 at 02:16:45AM CEST, xiyou.wangcong@gmail.com wrote:
>>>> On Sat, May 20, 2017 at 6:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>> +static void tcf_chain_destroy(struct tcf_chain *chain)
>>>>> +{
>>>>> +       list_del(&chain->list);
>>>>> +       tcf_chain_flush(chain);
>>>>>          kfree(chain);
>>>>>   }
>>>>>
>>>>> @@ -510,7 +517,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>>>>
>>>>>          if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
>>>>>                  tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
>>>>> -               tcf_chain_destroy(chain);
>>>>> +               tcf_chain_flush(chain);
>>>>
>>>>
>>>> I wonder if we should return EBUSY and do nothing in case of busy?
>>>> The chain is no longer visual to new actions after your list_del(), but
>>>> the old one could still use and see it.
>>>
>>> No. User request to flush the chain, that is what happens in the past
>>> and that is what should happen now.
>>> If there is still a reference, the chain_put will keep the empty chain.
>>
>> But if you dump the actions, this chain is still shown "goto chain"?
> 
> Yes, it will be shown there.
> 
> 
>> You can't claim you really delete it as long as actions can still
>> see it and dump it.
> 
> No, user just wants to delete all the filters. That is done. User does
> not care if the actual chain structure is there or not.
> 

I am trying to visualize a scenario where this is a problem.
Using gact action it may be  possible to cause issues (requires
validating - when i get time I will test).
Steps are something like:

1. create filter on chain 11 (refcnt = 1)
2. create gact action index 5 goto chain 11 (refcnt =2)
3'. create new filter on chain 0 ... action gact index 5
3''. create new filter on chain 0 ... action gact index 5


None of the #3 steps will increment the refcnt.
Delete the filter from #1 (refcnt becomes 1)
Delete the filter from #3'1 (refcnt = 0, destroy happens)
Filter #3'' is still hanging there. Dump that and strange things
happen.

cheers,
jamal

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Jiri Pirko]

Mon, May 22, 2017 at 12:42:44PM CEST, jhs@mojatatu.com wrote:
>On 17-05-21 03:19 PM, Jiri Pirko wrote:
>> Sun, May 21, 2017 at 08:27:21PM CEST, xiyou.wangcong@gmail.com wrote:
>> > On Sat, May 20, 2017 at 10:54 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > > Sun, May 21, 2017 at 02:16:45AM CEST, xiyou.wangcong@gmail.com wrote:
>> > > > On Sat, May 20, 2017 at 6:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > > > > +static void tcf_chain_destroy(struct tcf_chain *chain)
>> > > > > +{
>> > > > > +       list_del(&chain->list);
>> > > > > +       tcf_chain_flush(chain);
>> > > > >          kfree(chain);
>> > > > >   }
>> > > > > 
>> > > > > @@ -510,7 +517,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>> > > > > 
>> > > > >          if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
>> > > > >                  tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
>> > > > > -               tcf_chain_destroy(chain);
>> > > > > +               tcf_chain_flush(chain);
>> > > > 
>> > > > 
>> > > > I wonder if we should return EBUSY and do nothing in case of busy?
>> > > > The chain is no longer visual to new actions after your list_del(), but
>> > > > the old one could still use and see it.
>> > > 
>> > > No. User request to flush the chain, that is what happens in the past
>> > > and that is what should happen now.
>> > > If there is still a reference, the chain_put will keep the empty chain.
>> > 
>> > But if you dump the actions, this chain is still shown "goto chain"?
>> 
>> Yes, it will be shown there.
>> 
>> 
>> > You can't claim you really delete it as long as actions can still
>> > see it and dump it.
>> 
>> No, user just wants to delete all the filters. That is done. User does
>> not care if the actual chain structure is there or not.
>> 
>
>I am trying to visualize a scenario where this is a problem.
>Using gact action it may be  possible to cause issues (requires
>validating - when i get time I will test).
>Steps are something like:
>
>1. create filter on chain 11 (refcnt = 1)

refcnt will be 0, chain->filter_chain will be non-NULL.
Please see the code before you assume anything. Namely tcf_chain_get and
tcf_chain_put.


>2. create gact action index 5 goto chain 11 (refcnt =2)

refcnt will be 1 after this


>3'. create new filter on chain 0 ... action gact index 5
>3''. create new filter on chain 0 ... action gact index 5
>
>
>None of the #3 steps will increment the refcnt.

Right


>Delete the filter from #1 (refcnt becomes 1)

Right, refcnt was 1, after delete will still be 1


>Delete the filter from #3'1 (refcnt = 0, destroy happens)

No. refcnt will still be 1.


>Filter #3'' is still hanging there. Dump that and strange things
>happen.

No. I see nothing strange.


>
>cheers,
>jamal

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Cong Wang]

On Sun, May 21, 2017 at 12:19 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>You can't claim you really delete it as long as actions can still
>>see it and dump it.
>
> No, user just wants to delete all the filters. That is done. User does
> not care if the actual chain structure is there or not.
>

Hmm, so users see a chain with no filters... Fair enough.

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Cong Wang]

On Mon, May 22, 2017 at 1:54 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sun, May 21, 2017 at 12:19 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>You can't claim you really delete it as long as actions can still
>>>see it and dump it.
>>
>> No, user just wants to delete all the filters. That is done. User does
>> not care if the actual chain structure is there or not.
>>
>
> Hmm, so users see a chain with no filters... Fair enough.

But since you remove the chain from the chain_list, it means
users could not add new filters to this chain after flushing? And
users could create a new chain with the same index??

If so, you should instead keep it in the chain_list, although empty.

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Jiri Pirko]

Mon, May 22, 2017 at 11:04:58PM CEST, xiyou.wangcong@gmail.com wrote:
>On Mon, May 22, 2017 at 1:54 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Sun, May 21, 2017 at 12:19 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>You can't claim you really delete it as long as actions can still
>>>>see it and dump it.
>>>
>>> No, user just wants to delete all the filters. That is done. User does
>>> not care if the actual chain structure is there or not.
>>>
>>
>> Hmm, so users see a chain with no filters... Fair enough.
>
>But since you remove the chain from the chain_list, it means
>users could not add new filters to this chain after flushing? And

No, in flush, I don't remove it from the list. That is not in the
patch. Why would you think so?


>users could create a new chain with the same index??
>
>If so, you should instead keep it in the chain_list, although empty.

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Cong Wang]

On Mon, May 22, 2017 at 10:17 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, May 22, 2017 at 11:04:58PM CEST, xiyou.wangcong@gmail.com wrote:
>>On Mon, May 22, 2017 at 1:54 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Sun, May 21, 2017 at 12:19 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>You can't claim you really delete it as long as actions can still
>>>>>see it and dump it.
>>>>
>>>> No, user just wants to delete all the filters. That is done. User does
>>>> not care if the actual chain structure is there or not.
>>>>
>>>
>>> Hmm, so users see a chain with no filters... Fair enough.
>>
>>But since you remove the chain from the chain_list, it means
>>users could not add new filters to this chain after flushing? And
>
> No, in flush, I don't remove it from the list. That is not in the
> patch. Why would you think so?

Oh, I missed the minus before the first list_del()... I blame my
Web browser font.

Your patch looks good.

Re: [patch net-next 2/2] net/sched: fix filter flushing

[Cong Wang]

On Sat, May 20, 2017 at 6:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> When user instructs to remove all filters from chain, we cannot destroy
> the chain as other actions may hold a reference. Also the put in errout
> would try to destroy it again. So instead, just walk the chain and remove
> all existing filters.
>
> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Acked-by: Cong Wang <xiyou.wangcong@gmail.com>

Re: [patch net-next 1/2] net/sched: properly assign RCU pointer in tcf_chain_tp_insert/remove

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Sat, 20 May 2017 15:01:31 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> *p_filter_chain is rcu-dereferenced on reader path. So here in writer,
> property assign the pointer.
> 
> Fixes: 2190d1d0944f ("net: sched: introduce helpers to work with filter chains")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Applied.

Re: [patch net-next 2/2] net/sched: fix filter flushing

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Sat, 20 May 2017 15:01:32 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> When user instructs to remove all filters from chain, we cannot destroy
> the chain as other actions may hold a reference. Also the put in errout
> would try to destroy it again. So instead, just walk the chain and remove
> all existing filters.
> 
> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Applied.