On Thu, 26 Jul 2018 18:31:01 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <j...@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 <j...@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,
