* [patch net-next RFC] net: sched: don't dump chains only held by actions
@ 2018-07-26 16:31 Jiri Pirko
2018-07-26 19:59 ` Jakub Kicinski
2018-07-27 4:18 ` Cong Wang
0 siblings, 2 replies; 5+ messages in thread
From: Jiri Pirko @ 2018-07-26 16:31 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, xiyou.wangcong, mlxsw
From: Jiri Pirko <jiri@mellanox.com>
In case a chain is empty and not explicitly created by a user,
such chain should not exist. The only exception is if there is
an action "goto chain" pointing to it. In that case, don't show the
chain in the dump. Track the chain references held by actions and
use them to find out if a chain should or should not be shown
in chain dump.
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/pkt_cls.h | 3 +++
include/net/sch_generic.h | 1 +
net/sched/act_api.c | 4 +--
net/sched/cls_api.c | 68 +++++++++++++++++++++++++++++++++++++++--------
4 files changed, 63 insertions(+), 13 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index a3101582f642..6d02f31abba8 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -39,7 +39,10 @@ bool tcf_queue_work(struct rcu_work *rwork, work_func_t func);
#ifdef CONFIG_NET_CLS
struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
bool create);
+struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block,
+ u32 chain_index);
void tcf_chain_put(struct tcf_chain *chain);
+void tcf_chain_put_by_act(struct tcf_chain *chain);
void tcf_block_netif_keep_dst(struct tcf_block *block);
int tcf_block_get(struct tcf_block **p_block,
struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q,
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 085c509c8674..c5432362dc26 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -314,6 +314,7 @@ struct tcf_chain {
struct tcf_block *block;
u32 index; /* chain index */
unsigned int refcnt;
+ unsigned int action_refcnt;
bool explicitly_created;
const struct tcf_proto_ops *tmplt_ops;
void *tmplt_priv;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 148a89ab789b..b43df1e25c6d 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -36,7 +36,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
if (!tp)
return -EINVAL;
- a->goto_chain = tcf_chain_get(tp->chain->block, chain_index, true);
+ a->goto_chain = tcf_chain_get_by_act(tp->chain->block, chain_index);
if (!a->goto_chain)
return -ENOMEM;
return 0;
@@ -44,7 +44,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
static void tcf_action_goto_chain_fini(struct tc_action *a)
{
- tcf_chain_put(a->goto_chain);
+ tcf_chain_put_by_act(a->goto_chain);
}
static void tcf_action_goto_chain_exec(const struct tc_action *a,
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 75cce2819de9..76035cd6e3bf 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -262,6 +262,25 @@ static void tcf_chain_hold(struct tcf_chain *chain)
++chain->refcnt;
}
+static void tcf_chain_hold_by_act(struct tcf_chain *chain)
+{
+ ++chain->action_refcnt;
+}
+
+static void tcf_chain_release_by_act(struct tcf_chain *chain)
+{
+ --chain->action_refcnt;
+}
+
+static bool tcf_chain_is_zombie(struct tcf_chain *chain)
+{
+ /* In case all the references are action references, this
+ * chain is a zombie and should not be listed in the chain
+ * dump list.
+ */
+ return chain->refcnt == chain->action_refcnt;
+}
+
static struct tcf_chain *tcf_chain_lookup(struct tcf_block *block,
u32 chain_index)
{
@@ -298,6 +317,15 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
}
EXPORT_SYMBOL(tcf_chain_get);
+struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block, u32 chain_index)
+{
+ struct tcf_chain *chain = tcf_chain_get(block, chain_index, true);
+
+ tcf_chain_hold_by_act(chain);
+ return chain;
+}
+EXPORT_SYMBOL(tcf_chain_get_by_act);
+
static void tc_chain_tmplt_del(struct tcf_chain *chain);
void tcf_chain_put(struct tcf_chain *chain)
@@ -310,6 +338,13 @@ void tcf_chain_put(struct tcf_chain *chain)
}
EXPORT_SYMBOL(tcf_chain_put);
+void tcf_chain_put_by_act(struct tcf_chain *chain)
+{
+ tcf_chain_release_by_act(chain);
+ tcf_chain_put(chain);
+}
+EXPORT_SYMBOL(tcf_chain_put_by_act);
+
static void tcf_chain_put_explicitly_created(struct tcf_chain *chain)
{
if (chain->explicitly_created)
@@ -1803,17 +1838,26 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
chain = tcf_chain_lookup(block, chain_index);
if (n->nlmsg_type == RTM_NEWCHAIN) {
if (chain) {
- NL_SET_ERR_MSG(extack, "Filter chain already exists");
- return -EEXIST;
- }
- if (!(n->nlmsg_flags & NLM_F_CREATE)) {
- NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain");
- return -ENOENT;
- }
- chain = tcf_chain_create(block, chain_index);
- if (!chain) {
- NL_SET_ERR_MSG(extack, "Failed to create filter chain");
- return -ENOMEM;
+ if (tcf_chain_is_zombie(chain)) {
+ /* The chain exists only because there is
+ * some action referencing it, meaning it
+ * is a zombie.
+ */
+ tcf_chain_hold(chain);
+ } else {
+ NL_SET_ERR_MSG(extack, "Filter chain already exists");
+ return -EEXIST;
+ }
+ } else {
+ if (!(n->nlmsg_flags & NLM_F_CREATE)) {
+ NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain");
+ return -ENOENT;
+ }
+ chain = tcf_chain_create(block, chain_index);
+ if (!chain) {
+ NL_SET_ERR_MSG(extack, "Failed to create filter chain");
+ return -ENOMEM;
+ }
}
} else {
if (!chain) {
@@ -1944,6 +1988,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
index++;
continue;
}
+ if (tcf_chain_is_zombie(chain))
+ continue;
err = tc_chain_fill_node(chain, net, skb, block,
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
--
2.14.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [patch net-next RFC] net: sched: don't dump chains only held by actions
2018-07-26 16:31 [patch net-next RFC] net: sched: don't dump chains only held by actions Jiri Pirko
@ 2018-07-26 19:59 ` Jakub Kicinski
2018-07-27 6:13 ` Jiri Pirko
2018-07-27 4:18 ` Cong Wang
1 sibling, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2018-07-26 19:59 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, jhs, xiyou.wangcong, mlxsw
On Thu, 26 Jul 2018 18:31:01 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> In case a chain is empty and not explicitly created by a user,
> such chain should not exist. The only exception is if there is
> an action "goto chain" pointing to it. In that case, don't show the
> chain in the dump. Track the chain references held by actions and
> use them to find out if a chain should or should not be shown
> in chain dump.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
I don't have any better ideas :)
One question below.
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 75cce2819de9..76035cd6e3bf 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -262,6 +262,25 @@ static void tcf_chain_hold(struct tcf_chain *chain)
> ++chain->refcnt;
> }
>
> +static void tcf_chain_hold_by_act(struct tcf_chain *chain)
> +{
> + ++chain->action_refcnt;
> +}
> +
> +static void tcf_chain_release_by_act(struct tcf_chain *chain)
> +{
> + --chain->action_refcnt;
> +}
> +
> +static bool tcf_chain_is_zombie(struct tcf_chain *chain)
> +{
> + /* In case all the references are action references, this
> + * chain is a zombie and should not be listed in the chain
> + * dump list.
> + */
> + return chain->refcnt == chain->action_refcnt;
> +}
> +
> static struct tcf_chain *tcf_chain_lookup(struct tcf_block *block,
> u32 chain_index)
> {
> @@ -298,6 +317,15 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
> }
> EXPORT_SYMBOL(tcf_chain_get);
>
> +struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block, u32 chain_index)
> +{
> + struct tcf_chain *chain = tcf_chain_get(block, chain_index, true);
> +
> + tcf_chain_hold_by_act(chain);
> + return chain;
> +}
> +EXPORT_SYMBOL(tcf_chain_get_by_act);
> +
> static void tc_chain_tmplt_del(struct tcf_chain *chain);
>
> void tcf_chain_put(struct tcf_chain *chain)
> @@ -310,6 +338,13 @@ void tcf_chain_put(struct tcf_chain *chain)
> }
> EXPORT_SYMBOL(tcf_chain_put);
>
> +void tcf_chain_put_by_act(struct tcf_chain *chain)
> +{
> + tcf_chain_release_by_act(chain);
> + tcf_chain_put(chain);
> +}
> +EXPORT_SYMBOL(tcf_chain_put_by_act);
> +
> static void tcf_chain_put_explicitly_created(struct tcf_chain *chain)
> {
> if (chain->explicitly_created)
> @@ -1803,17 +1838,26 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
> chain = tcf_chain_lookup(block, chain_index);
> if (n->nlmsg_type == RTM_NEWCHAIN) {
> if (chain) {
> - NL_SET_ERR_MSG(extack, "Filter chain already exists");
> - return -EEXIST;
> - }
> - if (!(n->nlmsg_flags & NLM_F_CREATE)) {
> - NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain");
> - return -ENOENT;
> - }
> - chain = tcf_chain_create(block, chain_index);
> - if (!chain) {
> - NL_SET_ERR_MSG(extack, "Failed to create filter chain");
> - return -ENOMEM;
> + if (tcf_chain_is_zombie(chain)) {
> + /* The chain exists only because there is
> + * some action referencing it, meaning it
> + * is a zombie.
> + */
> + tcf_chain_hold(chain);
I'm not 100% sure why this is needed? In my tree below I see:
switch (n->nlmsg_type) {
case RTM_NEWCHAIN:
err = tc_chain_tmplt_add(chain, net, tca, extack);
if (err)
goto errout;
/* In case the chain was successfully added, take a reference
* to the chain. This ensures that an empty chain
* does not disappear at the end of this function.
*/
tcf_chain_hold(chain);
chain->explicitly_created = true;
so one reference will be taken.. do we need two?
> + } else {
> + NL_SET_ERR_MSG(extack, "Filter chain already exists");
> + return -EEXIST;
> + }
> + } else {
> + if (!(n->nlmsg_flags & NLM_F_CREATE)) {
> + NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain");
> + return -ENOENT;
> + }
> + chain = tcf_chain_create(block, chain_index);
> + if (!chain) {
> + NL_SET_ERR_MSG(extack, "Failed to create filter chain");
> + return -ENOMEM;
> + }
> }
> } else {
> if (!chain) {
> @@ -1944,6 +1988,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> index++;
> continue;
> }
> + if (tcf_chain_is_zombie(chain))
> + continue;
> err = tc_chain_fill_node(chain, net, skb, block,
> NETLINK_CB(cb->skb).portid,
> cb->nlh->nlmsg_seq, NLM_F_MULTI,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch net-next RFC] net: sched: don't dump chains only held by actions
2018-07-26 16:31 [patch net-next RFC] net: sched: don't dump chains only held by actions Jiri Pirko
2018-07-26 19:59 ` Jakub Kicinski
@ 2018-07-27 4:18 ` Cong Wang
2018-07-27 6:13 ` Jiri Pirko
1 sibling, 1 reply; 5+ messages in thread
From: Cong Wang @ 2018-07-27 4:18 UTC (permalink / raw)
To: Jiri Pirko
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim, mlxsw
On Thu, Jul 26, 2018 at 9:33 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> From: Jiri Pirko <jiri@mellanox.com>
>
> In case a chain is empty and not explicitly created by a user,
> such chain should not exist. The only exception is if there is
> an action "goto chain" pointing to it. In that case, don't show the
> chain in the dump. Track the chain references held by actions and
> use them to find out if a chain should or should not be shown
> in chain dump.
Hiding it makes sense. But you still need to make sure
user can't find it by ID, hiding it merely means user can't
see it.
Also, I don't understand why you increase the refcnt
for a hiding chain either, like Jakub mentioned.
If user can't see it, it must not be found by ID either.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch net-next RFC] net: sched: don't dump chains only held by actions
2018-07-26 19:59 ` Jakub Kicinski
@ 2018-07-27 6:13 ` Jiri Pirko
0 siblings, 0 replies; 5+ messages in thread
From: Jiri Pirko @ 2018-07-27 6:13 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, davem, jhs, xiyou.wangcong, mlxsw
Thu, Jul 26, 2018 at 09:59:30PM CEST, jakub.kicinski@netronome.com wrote:
>On Thu, 26 Jul 2018 18:31:01 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> In case a chain is empty and not explicitly created by a user,
>> such chain should not exist. The only exception is if there is
>> an action "goto chain" pointing to it. In that case, don't show the
>> chain in the dump. Track the chain references held by actions and
>> use them to find out if a chain should or should not be shown
>> in chain dump.
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>I don't have any better ideas :)
>
>One question below.
>
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 75cce2819de9..76035cd6e3bf 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -262,6 +262,25 @@ static void tcf_chain_hold(struct tcf_chain *chain)
>> ++chain->refcnt;
>> }
>>
>> +static void tcf_chain_hold_by_act(struct tcf_chain *chain)
>> +{
>> + ++chain->action_refcnt;
>> +}
>> +
>> +static void tcf_chain_release_by_act(struct tcf_chain *chain)
>> +{
>> + --chain->action_refcnt;
>> +}
>> +
>> +static bool tcf_chain_is_zombie(struct tcf_chain *chain)
>> +{
>> + /* In case all the references are action references, this
>> + * chain is a zombie and should not be listed in the chain
>> + * dump list.
>> + */
>> + return chain->refcnt == chain->action_refcnt;
>> +}
>> +
>> static struct tcf_chain *tcf_chain_lookup(struct tcf_block *block,
>> u32 chain_index)
>> {
>> @@ -298,6 +317,15 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>> }
>> EXPORT_SYMBOL(tcf_chain_get);
>>
>> +struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block, u32 chain_index)
>> +{
>> + struct tcf_chain *chain = tcf_chain_get(block, chain_index, true);
>> +
>> + tcf_chain_hold_by_act(chain);
>> + return chain;
>> +}
>> +EXPORT_SYMBOL(tcf_chain_get_by_act);
>> +
>> static void tc_chain_tmplt_del(struct tcf_chain *chain);
>>
>> void tcf_chain_put(struct tcf_chain *chain)
>> @@ -310,6 +338,13 @@ void tcf_chain_put(struct tcf_chain *chain)
>> }
>> EXPORT_SYMBOL(tcf_chain_put);
>>
>> +void tcf_chain_put_by_act(struct tcf_chain *chain)
>> +{
>> + tcf_chain_release_by_act(chain);
>> + tcf_chain_put(chain);
>> +}
>> +EXPORT_SYMBOL(tcf_chain_put_by_act);
>> +
>> static void tcf_chain_put_explicitly_created(struct tcf_chain *chain)
>> {
>> if (chain->explicitly_created)
>> @@ -1803,17 +1838,26 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
>> chain = tcf_chain_lookup(block, chain_index);
>> if (n->nlmsg_type == RTM_NEWCHAIN) {
>> if (chain) {
>> - NL_SET_ERR_MSG(extack, "Filter chain already exists");
>> - return -EEXIST;
>> - }
>> - if (!(n->nlmsg_flags & NLM_F_CREATE)) {
>> - NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain");
>> - return -ENOENT;
>> - }
>> - chain = tcf_chain_create(block, chain_index);
>> - if (!chain) {
>> - NL_SET_ERR_MSG(extack, "Failed to create filter chain");
>> - return -ENOMEM;
>> + if (tcf_chain_is_zombie(chain)) {
>> + /* The chain exists only because there is
>> + * some action referencing it, meaning it
>> + * is a zombie.
>> + */
>> + tcf_chain_hold(chain);
>
>I'm not 100% sure why this is needed? In my tree below I see:
>
> switch (n->nlmsg_type) {
> case RTM_NEWCHAIN:
> err = tc_chain_tmplt_add(chain, net, tca, extack);
> if (err)
> goto errout;
> /* In case the chain was successfully added, take a reference
> * to the chain. This ensures that an empty chain
> * does not disappear at the end of this function.
> */
> tcf_chain_hold(chain);
> chain->explicitly_created = true;
>
>so one reference will be taken.. do we need two?
There is a put at the end of this function.
>
>> + } else {
>> + NL_SET_ERR_MSG(extack, "Filter chain already exists");
>> + return -EEXIST;
>> + }
>> + } else {
>> + if (!(n->nlmsg_flags & NLM_F_CREATE)) {
>> + NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain");
>> + return -ENOENT;
>> + }
>> + chain = tcf_chain_create(block, chain_index);
>> + if (!chain) {
>> + NL_SET_ERR_MSG(extack, "Failed to create filter chain");
>> + return -ENOMEM;
>> + }
>> }
>> } else {
>> if (!chain) {
>> @@ -1944,6 +1988,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
>> index++;
>> continue;
>> }
>> + if (tcf_chain_is_zombie(chain))
>> + continue;
>> err = tc_chain_fill_node(chain, net, skb, block,
>> NETLINK_CB(cb->skb).portid,
>> cb->nlh->nlmsg_seq, NLM_F_MULTI,
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch net-next RFC] net: sched: don't dump chains only held by actions
2018-07-27 4:18 ` Cong Wang
@ 2018-07-27 6:13 ` Jiri Pirko
0 siblings, 0 replies; 5+ messages in thread
From: Jiri Pirko @ 2018-07-27 6:13 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim, mlxsw
Fri, Jul 27, 2018 at 06:18:14AM CEST, xiyou.wangcong@gmail.com wrote:
>On Thu, Jul 26, 2018 at 9:33 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> In case a chain is empty and not explicitly created by a user,
>> such chain should not exist. The only exception is if there is
>> an action "goto chain" pointing to it. In that case, don't show the
>> chain in the dump. Track the chain references held by actions and
>> use them to find out if a chain should or should not be shown
>> in chain dump.
>
>Hiding it makes sense. But you still need to make sure
>user can't find it by ID, hiding it merely means user can't
>see it.
>
>Also, I don't understand why you increase the refcnt
>for a hiding chain either, like Jakub mentioned.
>
>If user can't see it, it must not be found by ID either.
Ack. Will do that.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-27 7:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-26 16:31 [patch net-next RFC] net: sched: don't dump chains only held by actions Jiri Pirko
2018-07-26 19:59 ` Jakub Kicinski
2018-07-27 6:13 ` Jiri Pirko
2018-07-27 4:18 ` Cong Wang
2018-07-27 6:13 ` Jiri Pirko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.