netdev - Re: [PATCH net-next] sched: act_ct: switch to per-action label counting

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]

Message-ID: <6bd93499-97d1-4a43-ac8e-1432f2799b3b@mojatatu.com>
Date: Fri, 20 Oct 2023 11:28:04 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Cc: jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us
Subject: Re: [PATCH net-next] sched: act_ct: switch to per-action label
 counting

On 20/10/2023 09:45, Florian Westphal wrote:
> net->ct.labels_used was meant to convey 'number of ip/nftables rules
> that need the label extension allocated'.
> 
> act_ct enables this for each net namespace, which voids all attempts
> to avoid ct->ext allocation when possible.
> 
> Move this increment to the control plane to request label extension
> space allocation only when its needed.
> 
> Signed-off-by: Florian Westphal <fw@...len.de>
> ---
>   "tdc.py -c ct" still passes with this applied.
> 
>   include/net/tc_act/tc_ct.h |  1 +
>   net/sched/act_ct.c         | 41 +++++++++++++++++---------------------
>   2 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h
> index b24ea2d9400b..8a6dbfb23336 100644
> --- a/include/net/tc_act/tc_ct.h
> +++ b/include/net/tc_act/tc_ct.h
> @@ -22,6 +22,7 @@ struct tcf_ct_params {
>   
>   	struct nf_nat_range2 range;
>   	bool ipv4_range;
> +	bool put_labels;
>   
>   	u16 ct_action;
>   
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 7c652d14528b..9422686f73b0 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -690,7 +690,6 @@ static struct tc_action_ops act_ct_ops;
>   
>   struct tc_ct_action_net {
>   	struct tc_action_net tn; /* Must be first */
> -	bool labels;
>   };
>   
>   /* Determine whether skb->_nfct is equal to the result of conntrack lookup. */
> @@ -829,8 +828,13 @@ static void tcf_ct_params_free(struct tcf_ct_params *params)
>   	}
>   	if (params->ct_ft)
>   		tcf_ct_flow_table_put(params->ct_ft);
> -	if (params->tmpl)
> +	if (params->tmpl) {
> +		if (params->put_labels)
> +			nf_connlabels_put(nf_ct_net(params->tmpl));
> +
>   		nf_ct_put(params->tmpl);
> +	}
> +
>   	kfree(params);
>   }
>   
> @@ -1154,10 +1158,10 @@ static int tcf_ct_fill_params(struct net *net,
>   			      struct nlattr **tb,
>   			      struct netlink_ext_ack *extack)
>   {
> -	struct tc_ct_action_net *tn = net_generic(net, act_ct_ops.net_id);
>   	struct nf_conntrack_zone zone;
>   	int err, family, proto, len;
>   	struct nf_conn *tmpl;
> +	bool put_labels = false;

Shouldn't this be after `struct nf_conntrack_zone zone` to conform to 
rev-xmas-tree?

Other than that,
Reviewed-by: Pedro Tammela <pctammela@...atatu.com>

>   	char *name;
>   
>   	p->zone = NF_CT_DEFAULT_ZONE_ID;
> @@ -1186,15 +1190,20 @@ static int tcf_ct_fill_params(struct net *net,
>   	}
>   
>   	if (tb[TCA_CT_LABELS]) {
> +		unsigned int n_bits = sizeof_field(struct tcf_ct_params, labels) * 8;
> +
>   		if (!IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS)) {
>   			NL_SET_ERR_MSG_MOD(extack, "Conntrack labels isn't enabled.");
>   			return -EOPNOTSUPP;
>   		}
>   
> -		if (!tn->labels) {
> +		if (nf_connlabels_get(net, n_bits - 1)) {
>   			NL_SET_ERR_MSG_MOD(extack, "Failed to set connlabel length");
>   			return -EOPNOTSUPP;
> +		} else {
> +			put_labels = true;
>   		}
> +
>   		tcf_ct_set_key_val(tb,
>   				   p->labels, TCA_CT_LABELS,
>   				   p->labels_mask, TCA_CT_LABELS_MASK,
> @@ -1238,10 +1247,15 @@ static int tcf_ct_fill_params(struct net *net,
>   		}
>   	}
>   
> +	p->put_labels = put_labels;
> +
>   	if (p->ct_action & TCA_CT_ACT_COMMIT)
>   		__set_bit(IPS_CONFIRMED_BIT, &tmpl->status);
>   	return 0;
>   err:
> +	if (put_labels)
> +		nf_connlabels_put(net);
> +
>   	nf_ct_put(p->tmpl);
>   	p->tmpl = NULL;
>   	return err;
> @@ -1542,32 +1556,13 @@ static struct tc_action_ops act_ct_ops = {
>   
>   static __net_init int ct_init_net(struct net *net)
>   {
> -	unsigned int n_bits = sizeof_field(struct tcf_ct_params, labels) * 8;
>   	struct tc_ct_action_net *tn = net_generic(net, act_ct_ops.net_id);
>   
> -	if (nf_connlabels_get(net, n_bits - 1)) {
> -		tn->labels = false;
> -		pr_err("act_ct: Failed to set connlabels length");
> -	} else {
> -		tn->labels = true;
> -	}
> -
>   	return tc_action_net_init(net, &tn->tn, &act_ct_ops);
>   }
>   
>   static void __net_exit ct_exit_net(struct list_head *net_list)
>   {
> -	struct net *net;
> -
> -	rtnl_lock();
> -	list_for_each_entry(net, net_list, exit_list) {
> -		struct tc_ct_action_net *tn = net_generic(net, act_ct_ops.net_id);
> -
> -		if (tn->labels)
> -			nf_connlabels_put(net);
> -	}
> -	rtnl_unlock();
> -
>   	tc_action_net_exit(net_list, act_ct_ops.net_id);
>   }
>