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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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.

>  			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
>