Re: [PATCH net-next v2 6/7] net/sched: act_ct: offload UDP NEW connections

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Fri, Jan 13, 2023 at 05:55:47PM +0100, Vlad Buslov wrote:
> When processing connections allow offloading of UDP connections that don't
> have IPS_ASSURED_BIT set as unidirectional. When performing table lookup

Hmm. Considering that this is now offloading one direction only
already, what about skipping this grace period:

In nf_conntrack_udp_packet(), it does:

        /* If we've seen traffic both ways, this is some kind of UDP
         * stream. Set Assured.
         */
        if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
		...
                /* Still active after two seconds? Extend timeout. */
                if (time_after(jiffies, ct->proto.udp.stream_ts)) {
                        extra = timeouts[UDP_CT_REPLIED];
                        stream = true;
                }
		...
                /* Also, more likely to be important, and not a probe */
                if (stream && !test_and_set_bit(IPS_ASSURED_BIT, &ct->status))
                        nf_conntrack_event_cache(IPCT_ASSURED, ct);

Maybe the patch should be relying on IPS_SEEN_REPLY_BIT instead of
ASSURED for UDP? Just a thought here, but I'm not seeing why not.

> for reply packets check the current connection status: If UDP
> unidirectional connection became assured also promote the corresponding
> flow table entry to bidirectional and set the 'update' bit, else just set
> the 'update' bit since reply directional traffic will most likely cause
> connection status to become 'established' which requires updating the
> offload state.
> 
> Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
> ---
>  net/sched/act_ct.c | 48 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index bfddb462d2bc..563cbdd8341c 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -369,7 +369,7 @@ static void tcf_ct_flow_tc_ifidx(struct flow_offload *entry,
>  
>  static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,
>  				  struct nf_conn *ct,
> -				  bool tcp)
> +				  bool tcp, bool bidirectional)
>  {
>  	struct nf_conn_act_ct_ext *act_ct_ext;
>  	struct flow_offload *entry;
> @@ -388,6 +388,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,
>  		ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
>  		ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
>  	}
> +	if (bidirectional)
> +		__set_bit(NF_FLOW_HW_BIDIRECTIONAL, &entry->flags);
>  
>  	act_ct_ext = nf_conn_act_ct_ext_find(ct);
>  	if (act_ct_ext) {
> @@ -411,26 +413,34 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
>  					   struct nf_conn *ct,
>  					   enum ip_conntrack_info ctinfo)
>  {
> -	bool tcp = false;
> -
> -	if ((ctinfo != IP_CT_ESTABLISHED && ctinfo != IP_CT_ESTABLISHED_REPLY) ||
> -	    !test_bit(IPS_ASSURED_BIT, &ct->status))
> -		return;
> +	bool tcp = false, bidirectional = true;
>  
>  	switch (nf_ct_protonum(ct)) {
>  	case IPPROTO_TCP:
> -		tcp = true;
> -		if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED)
> +		if ((ctinfo != IP_CT_ESTABLISHED &&
> +		     ctinfo != IP_CT_ESTABLISHED_REPLY) ||
> +		    !test_bit(IPS_ASSURED_BIT, &ct->status) ||
> +		    ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED)
>  			return;
> +
> +		tcp = true;
>  		break;
>  	case IPPROTO_UDP:
> +		if (!nf_ct_is_confirmed(ct))
> +			return;
> +		if (!test_bit(IPS_ASSURED_BIT, &ct->status))
> +			bidirectional = false;
>  		break;
>  #ifdef CONFIG_NF_CT_PROTO_GRE
>  	case IPPROTO_GRE: {
>  		struct nf_conntrack_tuple *tuple;
>  
> -		if (ct->status & IPS_NAT_MASK)
> +		if ((ctinfo != IP_CT_ESTABLISHED &&
> +		     ctinfo != IP_CT_ESTABLISHED_REPLY) ||
> +		    !test_bit(IPS_ASSURED_BIT, &ct->status) ||
> +		    ct->status & IPS_NAT_MASK)
>  			return;
> +
>  		tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
>  		/* No support for GRE v1 */
>  		if (tuple->src.u.gre.key || tuple->dst.u.gre.key)
> @@ -446,7 +456,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
>  	    ct->status & IPS_SEQ_ADJUST)
>  		return;
>  
> -	tcf_ct_flow_table_add(ct_ft, ct, tcp);
> +	tcf_ct_flow_table_add(ct_ft, ct, tcp, bidirectional);
>  }
>  
>  static bool
> @@ -625,13 +635,27 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p,
>  	flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
>  	ct = flow->ct;
>  
> +	if (dir == FLOW_OFFLOAD_DIR_REPLY &&
> +	    !test_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags)) {
> +		/* Only offload reply direction after connection became
> +		 * assured.
> +		 */
> +		if (test_bit(IPS_ASSURED_BIT, &ct->status))
> +			set_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags);
> +		set_bit(NF_FLOW_HW_UPDATE, &flow->flags);
> +		return false;
> +	}
> +
>  	if (tcph && (unlikely(tcph->fin || tcph->rst))) {
>  		flow_offload_teardown(flow);
>  		return false;
>  	}
>  
> -	ctinfo = dir == FLOW_OFFLOAD_DIR_ORIGINAL ? IP_CT_ESTABLISHED :
> -						    IP_CT_ESTABLISHED_REPLY;
> +	if (dir == FLOW_OFFLOAD_DIR_ORIGINAL)
> +		ctinfo = test_bit(IPS_SEEN_REPLY_BIT, &ct->status) ?
> +			IP_CT_ESTABLISHED : IP_CT_NEW;
> +	else
> +		ctinfo = IP_CT_ESTABLISHED_REPLY;
>  
>  	flow_offload_refresh(nf_ft, flow);
>  	nf_conntrack_get(&ct->ct_general);
> -- 
> 2.38.1
>