netdev - Re: [RFC PATCH net-next 6/6 v2] net/sched: act_ct: Add tc recirc id set/del support

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: <bf2d8e94-ae4e-de3c-4d61-a4e9b7fb1bf1@mellanox.com>
Date:   Wed, 30 Jan 2019 08:20:00 +0000
From:   Paul Blakey <paulb@...lanox.com>
To:     Marcelo Leitner <mleitner@...hat.com>
CC:     Paul Blakey <paulb@...lanox.com>, Guy Shattah <sguy@...lanox.com>,
        Aaron Conole <aconole@...hat.com>,
        John Hurley <john.hurley@...ronome.com>,
        Simon Horman <simon.horman@...ronome.com>,
        Justin Pettit <jpettit@....org>,
        Gregory Rose <gvrose8192@...il.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        Flavio Leitner <fbl@...hat.com>,
        Florian Westphal <fwestpha@...hat.com>,
        Jiri Pirko <jiri@...nulli.us>, Rashid Khan <rkhan@...hat.com>,
        Sushil Kulkarni <sukulkar@...hat.com>,
        Andy Gospodarek <andrew.gospodarek@...adcom.com>,
        Roi Dayan <roid@...lanox.com>,
        Yossi Kuperman <yossiku@...lanox.com>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Rony Efraim <ronye@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH net-next 6/6 v2] net/sched: act_ct: Add tc recirc id
 set/del support



On 29/01/2019 20:12, Marcelo Leitner wrote:
> On Tue, Jan 29, 2019 at 10:02:06AM +0200, Paul Blakey wrote:
>> Set or clears (free) the skb tc recirc id extension.
>> If used with OVS, OVS can clear this recirc id after it reads it.
>>
>> Signed-off-by: Paul Blakey <paulb@...lanox.com>
>> ---
>>  include/net/tc_act/tc_ct.h        |  2 ++
>>  include/uapi/linux/tc_act/tc_ct.h |  2 ++
>>  net/sched/act_ct.c                | 18 ++++++++++++++++++
>>  3 files changed, 22 insertions(+)
>>
>> diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h
>> index 4a16375..6ea19d8 100644
>> --- a/include/net/tc_act/tc_ct.h
>> +++ b/include/net/tc_act/tc_ct.h
>> @@ -16,6 +16,8 @@ struct tcf_ct {
>>  	u32 mark_mask;
>>  	u16 zone;
>>  	bool commit;
>> +	uint32_t set_recirc;
>> +	bool del_recirc;
>>  };
>>  
>>  #define to_ct(a) ((struct tcf_ct *)a)
>> diff --git a/include/uapi/linux/tc_act/tc_ct.h b/include/uapi/linux/tc_act/tc_ct.h
>> index 6dbd771..2279a9b 100644
>> --- a/include/uapi/linux/tc_act/tc_ct.h
>> +++ b/include/uapi/linux/tc_act/tc_ct.h
>> @@ -14,6 +14,8 @@ struct tc_ct {
>>  	__u32 labels_mask[4];
>>  	__u32 mark;
>>  	__u32 mark_mask;
>> +	uint32_t set_recirc;
>> +	bool del_recirc;
>>  	bool commit;
>>  };
> 
> Have you considered adding a specific action for this? Asking because
> setting recirc_id can be useful outside of ct context too: decap, set
> recirc_id, reclassify.
> 

Yes I did, I didn't know if it was overkill, till a use case such as you
suggested arises beside CT.

One thing I'm not sure of is, who will delete the recirc id, it seems
it's freed only via skb_ext_put(skb) at skb_release_head_state()
But I might be missing a del from skb_scrub_packet like nf_reset does.

>>  
>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> index 61155cc..7822385 100644
>> --- a/net/sched/act_ct.c
>> +++ b/net/sched/act_ct.c
>> @@ -117,6 +117,11 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>>  	u_int8_t family;
>>  	bool cached;
>>  
>> +	if (ca->del_recirc) {
>> +		skb_ext_del(skb, SKB_EXT_TC_RECIRC_ID);
>> +		return ca->tcf_action;
>> +	}
>> +
>>  	/* The conntrack module expects to be working at L3. */
>>  	nh_ofs = skb_network_offset(skb);
>>  	skb_pull_rcsum(skb, nh_ofs);
>> @@ -243,6 +248,15 @@ static int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>>  	skb_postpush_rcsum(skb, skb->data, nh_ofs);
>>  
>>  	spin_unlock(&ca->tcf_lock);
>> +
>> +	if (ca->set_recirc) {
>> +		u32 recirc = ca->set_recirc;
>> +		uint32_t *recircp = skb_ext_add(skb, SKB_EXT_TC_RECIRC_ID);
>> +
>> +		if (recircp)
>> +			*recircp = recirc;
>> +	}
>> +
>>  	return ca->tcf_action;
>>  
>>  drop:
>> @@ -305,6 +319,8 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
>>  		ci->net = net;
>>  		ci->commit = parm->commit;
>>  		ci->zone = parm->zone;
>> +		ci->set_recirc = parm->set_recirc;
>> +		ci->del_recirc = parm->del_recirc;
>>  #if !IS_ENABLED(CONFIG_NF_CONNTRACK_MARK)
>>  		if (parm->mark_mask) {
>>  			NL_SET_ERR_MSG_MOD(extack, "Mark not supported by kernel config");
>> @@ -378,6 +394,8 @@ static inline int tcf_ct_dump(struct sk_buff *skb, struct tc_action *a,
>>  	opt.mark_mask = ci->mark_mask,
>>  	memcpy(opt.labels, ci->labels, sizeof(opt.labels));
>>  	memcpy(opt.labels_mask, ci->labels_mask, sizeof(opt.labels_mask));
>> +	opt.set_recirc = ci->set_recirc;
>> +	opt.del_recirc = ci->del_recirc;
>>  
>>  	if (nla_put(skb, TCA_CT_PARMS, sizeof(opt), &opt))
>>  		goto nla_put_failure;
>> -- 
>> 1.8.3.1
>>