| Message ID | 20230113165548.2692720-2-vladbu@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Allow offloading of UDP NEW connections via act_ct | expand |
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 >
On Fri, Jan 13, 2023 at 05:55:42PM +0100, Vlad Buslov wrote: ... > struct flow_match { > @@ -288,6 +289,7 @@ struct flow_action_entry { > } ct; > struct { > unsigned long cookie; > + enum ip_conntrack_info ctinfo; > 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; tcf_ct_flow_table_restore_skb() is doing: enum ip_conntrack_info ctinfo = cookie & NFCT_INFOMASK; Not sure if it really needs this duplication then. > > act_ct_labels = entry->ct_metadata.labels; > ct_labels = nf_ct_labels_find(ct); > -- > 2.38.1 >
On Tue, Jan 17, 2023 at 12:04:32PM -0300, Marcelo Ricardo Leitner wrote: > On Fri, Jan 13, 2023 at 05:55:42PM +0100, Vlad Buslov wrote: > ... > > struct flow_match { > > @@ -288,6 +289,7 @@ struct flow_action_entry { > > } ct; > > struct { > > unsigned long cookie; > > + enum ip_conntrack_info ctinfo; > > 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; > ^^^^^^^^^^^ Hmm. Thought that just came up and still need to dig into, but wanted to share/ask already. Would it be a problem to update the cookie later on then, to reflect the new ctinfo? > > > entry->ct_metadata.orig_dir = dir == IP_CT_DIR_ORIGINAL; > > + entry->ct_metadata.ctinfo = ctinfo; > > tcf_ct_flow_table_restore_skb() is doing: > enum ip_conntrack_info ctinfo = cookie & NFCT_INFOMASK; > > Not sure if it really needs this duplication then. > > > > > act_ct_labels = entry->ct_metadata.labels; > > ct_labels = nf_ct_labels_find(ct); > > -- > > 2.38.1 > >
Hi Marcelo, Thanks for reviewing! On Tue 17 Jan 2023 at 12:04, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > On Fri, Jan 13, 2023 at 05:55:42PM +0100, Vlad Buslov wrote: > ... >> struct flow_match { >> @@ -288,6 +289,7 @@ struct flow_action_entry { >> } ct; >> struct { >> unsigned long cookie; >> + enum ip_conntrack_info ctinfo; >> 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; > > tcf_ct_flow_table_restore_skb() is doing: > enum ip_conntrack_info ctinfo = cookie & NFCT_INFOMASK; > > Not sure if it really needs this duplication then. Indeed! I wanted to preserve the cookie as opaque integer (similar to TC filter cookie), but since drivers are already aware about its contents we can just reuse it for my case and prevent duplication. > >> >> act_ct_labels = entry->ct_metadata.labels; >> ct_labels = nf_ct_labels_find(ct); >> -- >> 2.38.1 >>
On Tue 17 Jan 2023 at 12:09, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > On Tue, Jan 17, 2023 at 12:04:32PM -0300, Marcelo Ricardo Leitner wrote: >> On Fri, Jan 13, 2023 at 05:55:42PM +0100, Vlad Buslov wrote: >> ... >> > struct flow_match { >> > @@ -288,6 +289,7 @@ struct flow_action_entry { >> > } ct; >> > struct { >> > unsigned long cookie; >> > + enum ip_conntrack_info ctinfo; >> > 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; >> ^^^^^^^^^^^ > > Hmm. Thought that just came up and still need to dig into, but wanted > to share/ask already. Would it be a problem to update the cookie later > on then, to reflect the new ctinfo? Not sure I'm following, but every time the flow changes state it is updated in the driver since new metadata is generated by calling tcf_ct_flow_table_fill_actions() from nf_flow_offload_rule_alloc(). > >> >> > entry->ct_metadata.orig_dir = dir == IP_CT_DIR_ORIGINAL; >> > + entry->ct_metadata.ctinfo = ctinfo; >> >> tcf_ct_flow_table_restore_skb() is doing: >> enum ip_conntrack_info ctinfo = cookie & NFCT_INFOMASK; >> >> Not sure if it really needs this duplication then. >> >> > >> > act_ct_labels = entry->ct_metadata.labels; >> > ct_labels = nf_ct_labels_find(ct); >> > -- >> > 2.38.1 >> >
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 >>
On Tue, Jan 17, 2023 at 07:28:09PM +0200, Vlad Buslov wrote: > On Tue 17 Jan 2023 at 12:09, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Tue, Jan 17, 2023 at 12:04:32PM -0300, Marcelo Ricardo Leitner wrote: > >> On Fri, Jan 13, 2023 at 05:55:42PM +0100, Vlad Buslov wrote: > >> ... > >> > struct flow_match { > >> > @@ -288,6 +289,7 @@ struct flow_action_entry { > >> > } ct; > >> > struct { > >> > unsigned long cookie; > >> > + enum ip_conntrack_info ctinfo; > >> > 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; > >> ^^^^^^^^^^^ > > > > Hmm. Thought that just came up and still need to dig into, but wanted > > to share/ask already. Would it be a problem to update the cookie later > > on then, to reflect the new ctinfo? > > Not sure I'm following, but every time the flow changes state it is > updated in the driver since new metadata is generated by calling > tcf_ct_flow_table_fill_actions() from nf_flow_offload_rule_alloc(). Whoops.. missed to reply this one. I worried that the cookie perhaps was used a hash index or so, and with such update on it, then maybe the key would be changing under the carpet. Checked now, I don't see such issue. I guess that's it from my side then. :) > > > > >> > >> > entry->ct_metadata.orig_dir = dir == IP_CT_DIR_ORIGINAL; > >> > + entry->ct_metadata.ctinfo = ctinfo; > >> > >> tcf_ct_flow_table_restore_skb() is doing: > >> enum ip_conntrack_info ctinfo = cookie & NFCT_INFOMASK; > >> > >> Not sure if it really needs this duplication then. > >> > >> > > >> > act_ct_labels = entry->ct_metadata.labels; > >> > ct_labels = nf_ct_labels_find(ct); > >> > -- > >> > 2.38.1 > >> > >
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; 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);
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(-)