netdev - Re: [patch net-next RFC] net: sched: don't dump chains only held by actions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 26 Jul 2018 12:59:30 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jhs@...atatu.com,
        xiyou.wangcong@...il.com, mlxsw@...lanox.com
Subject: Re: [patch net-next RFC] net: sched: don't dump chains only held by
 actions

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