| Message ID | 20180726163101.19718-1-jiri@resnulli.us |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,RFC] net: sched: don't dump chains only held by actions | expand |
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,
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.
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, >
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.
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,