On Tue 17 Jan 2023 at 15:42, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>
>> 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
>>
