Re: [PATCH net-next v2 1/7] net: flow_offload: provision conntrack info in ct_metadata

[Vlad Buslov <vladbu@nvidia.com>]

On Tue 17 Jan 2023 at 15:42, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Vlad,
>
> On Fri, Jan 13, 2023 at 05:55:42PM +0100, Vlad Buslov wrote:
>> In order to offload connections in other states besides "established" the
>> driver offload callbacks need to have access to connection conntrack info.
>> Extend flow offload intermediate representation data structure
>> flow_action_entry->ct_metadata with new enum ip_conntrack_info field and
>> fill it in tcf_ct_flow_table_add_action_meta() callback.
>> 
>> Reject offloading IP_CT_NEW connections for now by returning an error in
>> relevant driver callbacks based on value of ctinfo. Support for offloading
>> such connections will need to be added to the drivers afterwards.
>> 
>> Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
>> ---
>> 
>> Notes:
>>     Changes V1 -> V2:
>>     
>>     - Add missing include that caused compilation errors on certain configs.
>>     
>>     - Change naming in nfp driver as suggested by Simon and Baowen.
>> 
>>  .../ethernet/mellanox/mlx5/core/en/tc_ct.c    |  2 +-
>>  .../ethernet/netronome/nfp/flower/conntrack.c | 20 +++++++++++++++++++
>>  include/net/flow_offload.h                    |  2 ++
>>  net/sched/act_ct.c                            |  1 +
>>  4 files changed, 24 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
>> index 313df8232db7..8cad5cf3305d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
>> @@ -1077,7 +1077,7 @@ mlx5_tc_ct_block_flow_offload_add(struct mlx5_ct_ft *ft,
>>  	int err;
>>  
>>  	meta_action = mlx5_tc_ct_get_ct_metadata_action(flow_rule);
>> -	if (!meta_action)
>> +	if (!meta_action || meta_action->ct_metadata.ctinfo == IP_CT_NEW)
>>  		return -EOPNOTSUPP;
>>  
>>  	spin_lock_bh(&ct_priv->ht_lock);
>> diff --git a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
>> index f693119541d5..f7569584b9d8 100644
>> --- a/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
>> +++ b/drivers/net/ethernet/netronome/nfp/flower/conntrack.c
>> @@ -1964,6 +1964,23 @@ int nfp_fl_ct_stats(struct flow_cls_offload *flow,
>>  	return 0;
>>  }
>>  
>> +static bool
>> +nfp_fl_ct_offload_nft_supported(struct flow_cls_offload *flow)
>> +{
>> +	struct flow_rule *flow_rule = flow->rule;
>> +	struct flow_action *flow_action =
>> +		&flow_rule->action;
>> +	struct flow_action_entry *act;
>> +	int i;
>> +
>> +	flow_action_for_each(i, act, flow_action) {
>> +		if (act->id == FLOW_ACTION_CT_METADATA)
>> +			return act->ct_metadata.ctinfo != IP_CT_NEW;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>  static int
>>  nfp_fl_ct_offload_nft_flow(struct nfp_fl_ct_zone_entry *zt, struct flow_cls_offload *flow)
>>  {
>> @@ -1976,6 +1993,9 @@ nfp_fl_ct_offload_nft_flow(struct nfp_fl_ct_zone_entry *zt, struct flow_cls_offl
>>  	extack = flow->common.extack;
>>  	switch (flow->command) {
>>  	case FLOW_CLS_REPLACE:
>> +		if (!nfp_fl_ct_offload_nft_supported(flow))
>> +			return -EOPNOTSUPP;
>> +
>>  		/* Netfilter can request offload multiple times for the same
>>  		 * flow - protect against adding duplicates.
>>  		 */
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>> index 0400a0ac8a29..a6adaffb68fb 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -4,6 +4,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/list.h>
>>  #include <linux/netlink.h>
>> +#include <linux/netfilter/nf_conntrack_common.h>
>>  #include <net/flow_dissector.h>
>>  
>>  struct flow_match {
>> @@ -288,6 +289,7 @@ struct flow_action_entry {
>>  		} ct;
>>  		struct {
>>  			unsigned long cookie;
>> +			enum ip_conntrack_info ctinfo;
>
> Maybe you can use a bool here, only possible states that make sense
> are new and established.

As Marcelo suggested we can just obtain same info from the cookie, so
there is no need for adding either ctinfo or a boolean here.

>
>>  			u32 mark;
>>  			u32 labels[4];
>>  			bool orig_dir;
>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> index 0ca2bb8ed026..515577f913a3 100644
>> --- a/net/sched/act_ct.c
>> +++ b/net/sched/act_ct.c
>> @@ -187,6 +187,7 @@ static void tcf_ct_flow_table_add_action_meta(struct nf_conn *ct,
>>  	/* aligns with the CT reference on the SKB nf_ct_set */
>>  	entry->ct_metadata.cookie = (unsigned long)ct | ctinfo;
>>  	entry->ct_metadata.orig_dir = dir == IP_CT_DIR_ORIGINAL;
>> +	entry->ct_metadata.ctinfo = ctinfo;
>>  
>>  	act_ct_labels = entry->ct_metadata.labels;
>>  	ct_labels = nf_ct_labels_find(ct);
>> -- 
>> 2.38.1
>>