From: Saeed Mahameed <saeedm@nvidia.com>
Hi Dave, Jakub
This series provides misc updates to mlx5 plus the +trk+new TC
connection tracking rules support for UDP.
For more information please see tag log below.
Please pull and let me know if there is any problem.
Thanks,
Saeed.
---
The following changes since commit 58334e7537278793c86baa88b70c48b0d50b00ae:
Merge branch 'generic-zcopy_-functions' (2021-01-07 16:08:38 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git tags/mlx5-updates-2021-01-07
for you to fetch changes up to 29c9b5c4ed42ca9d7c8f8f6c34a7ef7ab787ad6e:
net/mlx5e: IPsec, Remove unnecessary config flag usage (2021-01-07 21:27:08 -0800)
----------------------------------------------------------------
mlx5-updates-2021-01-07
Misc updates series for mlx5 driver:
1) From Eli and Jianbo, E-Switch cleanups and usage of new
FW capability for mpls over udp
2) Paul Blakey, Adds support for mirroring with Connection tracking
by splitting rules to pre and post Connection tracking to perform the
mirroring.
3) Roi Dayan, Adds support for +trk+new connection tracking rules
3.1) cleanups to connection tracking
3.2) to support +trk+new feature of connection tracking, Roi adds another
flow table that catches all missed packets from the original CT table
(The table that handles stateful established flows)
3.3) Add support to offload +trk+new rules for terminating flows for udp
protocols using source port entropy.
We support only the default registered vxlan, RoCE and Geneve ports.
Using the registered ports assume the traffic is that of the
registered protocol.
4) From Tariq, Cleanups and improvements to IPSec
----------------------------------------------------------------
Eli Cohen (2):
net/mlx5e: Simplify condition on esw_vport_enable_qos()
net/mlx5: E-Switch, use new cap as condition for mpls over udp
Jianbo Liu (1):
net/mlx5e: E-Switch, Offload all chain 0 priorities when modify header and forward action is not supported
Paul Blakey (2):
net/mlx5: Add HW definition of reg_c_preserve
net/mlx5e: CT: Add support for mirroring
Roi Dayan (6):
net/mlx5e: CT: Pass null instead of zero spec
net/mlx5e: Remove redundant initialization to null
net/mlx5e: CT: Remove redundant usage of zone mask
net/mlx5e: CT: Preparation for offloading +trk+new ct rules
net/mlx5e: CT: Support offload of +trk+new ct rules
net/mlx5e: CT, Avoid false lock depenency warning
Tariq Toukan (4):
net/mlx5e: IPsec, Enclose csum logic under ipsec config
net/mlx5e: IPsec, Avoid unreachable return
net/mlx5e: IPsec, Inline feature_check fast-path function
net/mlx5e: IPsec, Remove unnecessary config flag usage
drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 353 +++++++++++++++++++--
drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.h | 6 +
.../mellanox/mlx5/core/en/tc_tun_mplsoudp.c | 4 +-
.../mellanox/mlx5/core/en_accel/en_accel.h | 4 +-
.../mellanox/mlx5/core/en_accel/ipsec_rxtx.c | 14 -
.../mellanox/mlx5/core/en_accel/ipsec_rxtx.h | 29 +-
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 4 -
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 2 -
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 47 +--
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 3 +-
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 3 +-
.../ethernet/mellanox/mlx5/core/lib/fs_chains.c | 7 +-
include/linux/mlx5/mlx5_ifc.h | 4 +-
13 files changed, 399 insertions(+), 81 deletions(-)
From: Eli Cohen <redacted>
esw->qos.enabled will only be true if both MLX5_CAP_GEN(dev, qos) and
MLX5_CAP_QOS(dev, esw_scheduling) are true. Therefore, remove them from
the condition in and rely only on esw->qos.enabled.
Fixes: 1bd27b11c1df ("net/mlx5: Introduce E-switch QoS management")
Signed-off-by: Eli Cohen <redacted>
Reviewed-by: Roi Dayan <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: Jianbo Liu <redacted>
Miss path handling of tc multi chain filters (i.e. filters that are
defined on chain > 0) requires the hardware to communicate to the
driver the last chain that was processed. This is possible only when
the hardware is capable of performing the combination of modify header
and forward to table actions. Currently, if the hardware is missing
this capability then the driver only offloads rules that are defined
on tc chain 0 prio 1. However, this restriction can be relaxed because
packets that miss from chain 0 are processed through all the
priorities by tc software.
Allow the offload of all the supported priorities for chain 0 even
when the hardware is not capable to perform modify header and goto
table actions.
Fixes: 0b3a8b6b5340 ("net/mlx5: E-Switch: Fix using fwd and modify when firmware doesn't support it")
Signed-off-by: Jianbo Liu <redacted>
Reviewed-by: Oz Shlomo <redacted>
Reviewed-by: Roi Dayan <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 6 ------
drivers/net/ethernet/mellanox/mlx5/core/lib/fs_chains.c | 3 ---
2 files changed, 9 deletions(-)
@@ -1317,12 +1317,6 @@ mlx5e_tc_add_fdb_flow(struct mlx5e_priv *priv,interr=0;intout_index;-if(!mlx5_chains_prios_supported(esw_chains(esw))&&attr->prio!=1){-NL_SET_ERR_MSG_MOD(extack,-"E-switch priorities unsupported, upgrade FW");-return-EOPNOTSUPP;-}-/* We check chain range only for tc flows.*Forftflows,wecheckedattr->chainwasoriginally0andsetitto*FDB_FT_CHAINwhichisoutsidetcrange.
From: Paul Blakey <redacted>
Add capability bit to test whether reg_c value is preserved on
recirculation.
Signed-off-by: Paul Blakey <redacted>
Signed-off-by: Maor Dickman <redacted>
Reviewed-by: Roi Dayan <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
include/linux/mlx5/mlx5_ifc.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Eli Cohen <redacted>
Use tunnel_stateless_mpls_over_udp instead of
MLX5_FLEX_PROTO_CW_MPLS_UDP since new devices have native support for
mpls over udp and do not rely on flex parser.
Signed-off-by: Eli Cohen <redacted>
Reviewed-by: Roi Dayan <redacted>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_mplsoudp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Roi Dayan <redacted>
The zone member is of type u16 so there is no reason to apply
the zone mask on it. This is also matching the call to set a
match in other places which don't need and don't apply the mask.
Signed-off-by: Roi Dayan <redacted>
Reviewed-by: Paul Blakey <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Tariq Toukan <tariqt@nvidia.com>
All IPsec logic should be wrapped under the compile flag,
including its checksum logic.
Introduce an inline function in ipsec datapath header,
with a corresponding stub.
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Raed Salem <redacted>
Reviewed-by: Huy Nguyen <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
.../ethernet/mellanox/mlx5/core/en_accel/ipsec_rxtx.h | 10 ++++++++++
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 3 +--
2 files changed, 11 insertions(+), 2 deletions(-)
From: Tariq Toukan <tariqt@nvidia.com>
Feature check functions are in the TX fast-path of all SKBs, not only
IPsec traffic.
Move the IPsec feature check function into a header and turn it inline.
Use a stub and clean the config flag condition in Eth main driver file.
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Raed Salem <redacted>
Reviewed-by: Huy Nguyen <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
.../mellanox/mlx5/core/en_accel/ipsec_rxtx.c | 14 --------------
.../mellanox/mlx5/core/en_accel/ipsec_rxtx.h | 19 +++++++++++++++++--
.../net/ethernet/mellanox/mlx5/core/en_main.c | 2 --
3 files changed, 17 insertions(+), 18 deletions(-)
@@ -4375,10 +4375,8 @@ netdev_features_t mlx5e_features_check(struct sk_buff *skb,features=vlan_features_check(skb,features);features=vxlan_features_check(skb,features);-#ifdef CONFIG_MLX5_EN_IPSECif(mlx5e_ipsec_feature_check(skb,netdev,features))returnfeatures;-#endif/* Validate if the tunneled packet is being offloaded by HW */if(skb->encapsulation&&
From: Roi Dayan <redacted>
Add support to offload +trk+new rules for terminating flows for udp
protocols using source port entropy.
This kind of traffic will never be considered connect in conntrack
and thus never set as established so no need to keep
track of them in SW conntrack and offload this traffic based on dst
port.
In this commit we support only the default registered vxlan port,
RoCE and Geneve ports. Using the registered ports assume the traffic
is that of the registered protocol.
Signed-off-by: Roi Dayan <redacted>
Reviewed-by: Paul Blakey <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
.../ethernet/mellanox/mlx5/core/en/tc_ct.c | 228 +++++++++++++++++-
.../ethernet/mellanox/mlx5/core/en/tc_ct.h | 6 +
.../net/ethernet/mellanox/mlx5/core/en_tc.c | 16 +-
3 files changed, 236 insertions(+), 14 deletions(-)
@@ -1390,10 +1409,157 @@ mlx5_tc_ct_free_pre_ct_tables(struct mlx5_ct_ft *ft)mlx5_tc_ct_free_pre_ct(ft,&ft->pre_ct);}+staticvoidmlx5_tc_ct_set_match_dst_udp_port(structmlx5_flow_spec*spec,u16dst_port)+{+void*headers_c=MLX5_ADDR_OF(fte_match_param,spec->match_criteria,+outer_headers);+void*headers_v=MLX5_ADDR_OF(fte_match_param,spec->match_value,+outer_headers);++MLX5_SET_TO_ONES(fte_match_set_lyr_2_4,headers_c,udp_dport);+MLX5_SET(fte_match_set_lyr_2_4,headers_v,udp_dport,dst_port);++spec->match_criteria_enable|=MLX5_MATCH_OUTER_HEADERS;+}++staticstructmlx5_tc_ct_trk_new_rule*+tc_ct_add_trk_new_rule(structmlx5_ct_ft*ft,intport)+{+structmlx5_tc_ct_priv*ct_priv=ft->ct_priv;+structmlx5_tc_ct_trk_new_rule*trk_new_rule;+structmlx5_flow_destinationdest={};+structmlx5_flow_actflow_act={};+structmlx5_flow_handle*rule;+structmlx5_flow_spec*spec;+interr;++trk_new_rule=kzalloc(sizeof(*trk_new_rule),GFP_KERNEL);+if(!trk_new_rule)+returnERR_PTR(-ENOMEM);++spec=kzalloc(sizeof(*spec),GFP_KERNEL);+if(!spec){+kfree(trk_new_rule);+returnERR_PTR(-ENOMEM);+}++flow_act.action=MLX5_FLOW_CONTEXT_ACTION_FWD_DEST|+MLX5_FLOW_CONTEXT_ACTION_MOD_HDR;+flow_act.flags|=FLOW_ACT_IGNORE_FLOW_LEVEL;+flow_act.modify_hdr=ft->trk_new_rules.modify_hdr;+dest.type=MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE;+dest.ft=ct_priv->post_ct;++mlx5e_tc_match_to_reg_match(spec,ZONE_TO_REG,ft->zone,MLX5_CT_ZONE_MASK);+mlx5_tc_ct_set_match_dst_udp_port(spec,port);++rule=mlx5_add_flow_rules(ct_priv->trk_new_ct,spec,&flow_act,&dest,1);+if(IS_ERR(rule)){+err=PTR_ERR(rule);+ct_dbg("Failed to add trk_new rule for udp port %d, err %d",port,err);+gotoerr_insert;+}++kfree(spec);+trk_new_rule->flow_rule=rule;+list_add_tail(&trk_new_rule->list,&ft->trk_new_rules.rules);+returntrk_new_rule;++err_insert:+kfree(spec);+kfree(trk_new_rule);+returnERR_PTR(err);+}++staticvoid+tc_ct_del_trk_new_rule(structmlx5_tc_ct_trk_new_rule*rule)+{+list_del(&rule->list);+mlx5_del_flow_rules(rule->flow_rule);+kfree(rule);+}++staticint+tc_ct_init_trk_new_rules(structmlx5_ct_ft*ft)+{+structmlx5_tc_ct_priv*ct_priv=ft->ct_priv;+structmlx5_tc_ct_trk_new_rule*rule,*tmp;+structmlx5e_tc_mod_hdr_actsmod_acts={};+structmlx5_modify_hdr*mod_hdr;+structmlx5e_priv*priv;+u32ct_state;+inti,err;++priv=netdev_priv(ct_priv->netdev);++ct_state=MLX5_CT_STATE_TRK_BIT|MLX5_CT_STATE_NEW_BIT;+err=mlx5e_tc_match_to_reg_set(priv->mdev,&mod_acts,ct_priv->ns_type,+CTSTATE_TO_REG,ct_state);+if(err){+ct_dbg("Failed to set register for ct trk_new");+gotoerr_set_registers;+}++err=mlx5e_tc_match_to_reg_set(priv->mdev,&mod_acts,ct_priv->ns_type,+ZONE_RESTORE_TO_REG,ft->zone_restore_id);+if(err){+ct_dbg("Failed to set register for ct trk_new zone restore");+gotoerr_set_registers;+}++mod_hdr=mlx5_modify_header_alloc(priv->mdev,+ct_priv->ns_type,+mod_acts.num_actions,+mod_acts.actions);+if(IS_ERR(mod_hdr)){+err=PTR_ERR(mod_hdr);+ct_dbg("Failed to create ct trk_new mod hdr");+gotoerr_set_registers;+}++ft->trk_new_rules.modify_hdr=mod_hdr;+dealloc_mod_hdr_actions(&mod_acts);++for(i=0;i<DEFAULT_UDP_PORTS;i++){+intport=default_udp_ports[i];++rule=tc_ct_add_trk_new_rule(ft,port);+if(IS_ERR(rule))+gotoerr_insert;+}++return0;++err_insert:+list_for_each_entry_safe(rule,tmp,&ft->trk_new_rules.rules,list)+tc_ct_del_trk_new_rule(rule);+mlx5_modify_header_dealloc(priv->mdev,mod_hdr);+err_set_registers:+dealloc_mod_hdr_actions(&mod_acts);+netdev_warn(priv->netdev,+"Failed to offload ct trk_new flow, err %d\n",err);+returnerr;+}++staticvoid+tc_ct_cleanup_trk_new_rules(structmlx5_ct_ft*ft)+{+structmlx5_tc_ct_priv*ct_priv=ft->ct_priv;+structmlx5_tc_ct_trk_new_rule*rule,*tmp;+structmlx5e_priv*priv;++list_for_each_entry_safe(rule,tmp,&ft->trk_new_rules.rules,list)+tc_ct_del_trk_new_rule(rule);++priv=netdev_priv(ct_priv->netdev);+mlx5_modify_header_dealloc(priv->mdev,ft->trk_new_rules.modify_hdr);+}+staticstructmlx5_ct_ft*mlx5_tc_ct_add_ft_cb(structmlx5_tc_ct_priv*ct_priv,u16zone,structnf_flowtable*nf_ft){+structnf_conntrack_zonectzone;structmlx5_ct_ft*ft;interr;
From: Roi Dayan <redacted>
To avoid false lock dependency warning set the ct_entries_ht lock
class different than the lock class of the ht being used when deleting
last flow from a group and then deleting a group, we get into del_sw_flow_group()
which call rhashtable_destroy on fg->ftes_hash which will take ht->mutex but
it's different than the ht->mutex here.
======================================================
WARNING: possible circular locking dependency detected
5.10.0-rc2+ #8 Tainted: G O
------------------------------------------------------
revalidator23/24009 is trying to acquire lock:
ffff888128d83828 (&node->lock){++++}-{3:3}, at: mlx5_del_flow_rules+0x83/0x7a0 [mlx5_core]
but task is already holding lock:
ffff8881081ef518 (&ht->mutex){+.+.}-{3:3}, at: rhashtable_free_and_destroy+0x37/0x720
which lock already depends on the new lock.
Fixes: 9808dd0a2aee ("net/mlx5e: CT: Use rhashtable's ct entries instead of a separate list")
Signed-off-by: Roi Dayan <redacted>
Reviewed-by: Paul Blakey <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 10 ++++++++++
1 file changed, 10 insertions(+)
From: Paul Blakey <redacted>
Add support for mirroring before the CT action by splitting the pre ct rule.
Mirror outputs are done first on the tc chain,prio table rule (the fwd
rule), which will then forward to a per port fwd table.
On this fwd table, we insert the original pre ct rule that forwards to
ct/ct nat table.
Signed-off-by: Paul Blakey <redacted>
Signed-off-by: Maor Dickman <redacted>
Reviewed-by: Roi Dayan <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
.../ethernet/mellanox/mlx5/core/en/tc_ct.c | 4 +++
.../net/ethernet/mellanox/mlx5/core/en_tc.c | 25 ++++++++++---------
2 files changed, 17 insertions(+), 12 deletions(-)
@@ -3264,7 +3268,8 @@ static bool actions_match_supported(struct mlx5e_priv *priv,actions=flow->attr->action;if(mlx5e_is_eswitch_flow(flow)){-if(flow->attr->esw_attr->split_count&&ct_flow){+if(flow->attr->esw_attr->split_count&&ct_flow&&+!MLX5_CAP_GEN(flow->attr->esw_attr->in_mdev,reg_c_preserve)){/* All registers used by ct are cleared when using*splitrules.*/
@@ -4373,6 +4378,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,returnerr;flow_flag_set(flow,CT);+esw_attr->split_count=esw_attr->out_count;break;default:NL_SET_ERR_MSG_MOD(extack,"The offload action is not supported");
From: Roi Dayan <redacted>
No need to pass zero spec to mlx5_add_flow_rules() as the
function can handle null spec.
Signed-off-by: Roi Dayan <redacted>
Reviewed-by: Oz Shlomo <redacted>
Reviewed-by: Paul Blakey <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -1220,9 +1220,8 @@ static int tc_ct_pre_ct_add_rules(struct mlx5_ct_ft *ct_ft,pre_ct->flow_rule=rule;/* add miss rule */-memset(spec,0,sizeof(*spec));dest.ft=nat?ct_priv->ct_nat:ct_priv->ct;-rule=mlx5_add_flow_rules(ft,spec,&flow_act,&dest,1);+rule=mlx5_add_flow_rules(ft,NULL,&flow_act,&dest,1);if(IS_ERR(rule)){err=PTR_ERR(rule);ct_dbg("Failed to add pre ct miss rule zone %d",zone);
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to the slow
path table.
Signed-off-by: Roi Dayan <redacted>
Reviewed-by: Paul Blakey <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
.../ethernet/mellanox/mlx5/core/en/tc_ct.c | 104 ++++++++++++++++--
1 file changed, 96 insertions(+), 8 deletions(-)
@@ -1893,6 +1897,72 @@ mlx5_tc_ct_init_check_support(struct mlx5e_priv *priv,returnmlx5_tc_ct_init_check_nic_support(priv,err_msg);}+staticstructmlx5_flow_handle*+tc_ct_add_miss_rule(structmlx5_flow_table*ft,+structmlx5_flow_table*next_ft)+{+structmlx5_flow_destinationdest={};+structmlx5_flow_actact={};++act.flags=FLOW_ACT_IGNORE_FLOW_LEVEL|FLOW_ACT_NO_APPEND;+act.action=MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;+dest.type=MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE;+dest.ft=next_ft;++returnmlx5_add_flow_rules(ft,NULL,&act,&dest,1);+}++staticint+tc_ct_add_ct_table_miss_rule(structmlx5_tc_ct_priv*ct_priv)+{+intinlen=MLX5_ST_SZ_BYTES(create_flow_group_in);+structmlx5_flow_handle*miss_rule;+structmlx5_flow_group*miss_group;+intmax_fte=ct_priv->ct->max_fte;+u32*flow_group_in;+interr=0;++flow_group_in=kvzalloc(inlen,GFP_KERNEL);+if(!flow_group_in)+return-ENOMEM;++/* create miss group */+MLX5_SET(create_flow_group_in,flow_group_in,start_flow_index,+max_fte-2);+MLX5_SET(create_flow_group_in,flow_group_in,end_flow_index,+max_fte-1);+miss_group=mlx5_create_flow_group(ct_priv->ct,flow_group_in);+if(IS_ERR(miss_group)){+err=PTR_ERR(miss_group);+gotoerr_miss_grp;+}++/* add miss rule to next fdb */+miss_rule=tc_ct_add_miss_rule(ct_priv->ct,ct_priv->trk_new_ct);+if(IS_ERR(miss_rule)){+err=PTR_ERR(miss_rule);+gotoerr_miss_rule;+}++ct_priv->miss_grp=miss_group;+ct_priv->miss_rule=miss_rule;+kvfree(flow_group_in);+return0;++err_miss_rule:+mlx5_destroy_flow_group(miss_group);+err_miss_grp:+kvfree(flow_group_in);+returnerr;+}++staticvoid+tc_ct_del_ct_table_miss_rule(structmlx5_tc_ct_priv*ct_priv)+{+mlx5_del_flow_rules(ct_priv->miss_rule);+mlx5_destroy_flow_group(ct_priv->miss_grp);+}+#define INIT_ERR_PREFIX "tc ct offload init failed"structmlx5_tc_ct_priv*
@@ -1962,6 +2032,18 @@ mlx5_tc_ct_init(struct mlx5e_priv *priv, struct mlx5_fs_chains *chains,gotoerr_post_ct_tbl;}+ct_priv->trk_new_ct=mlx5_chains_create_global_table(chains);+if(IS_ERR(ct_priv->trk_new_ct)){+err=PTR_ERR(ct_priv->trk_new_ct);+mlx5_core_warn(dev,"%s, failed to create trk new ct table err: %d",+INIT_ERR_PREFIX,err);+gotoerr_trk_new_ct_tbl;+}++err=tc_ct_add_ct_table_miss_rule(ct_priv);+if(err)+gotoerr_init_ct_tbl;+idr_init(&ct_priv->fte_ids);mutex_init(&ct_priv->control_lock);mutex_init(&ct_priv->shared_counter_lock);
@@ -1786,12 +1786,10 @@ int mlx5e_rq_set_handlers(struct mlx5e_rq *rq, struct mlx5e_params *params, boolrq->dealloc_wqe=mlx5e_dealloc_rx_mpwqe;rq->handle_rx_cqe=priv->profile->rx_handlers->handle_rx_cqe_mpwqe;-#ifdef CONFIG_MLX5_EN_IPSECif(MLX5_IPSEC_DEV(mdev)){netdev_err(netdev,"MPWQE RQ with IPSec offload not supported\n");return-EINVAL;}-#endifif(!rq->handle_rx_cqe){netdev_err(netdev,"RX handler of MPWQE RQ is not set\n");return-EINVAL;
From: Roi Dayan <redacted>
miss_rule and prio_s args are not being referenced before assigned
so there is no need to init them.
Signed-off-by: Roi Dayan <redacted>
Reviewed-by: Oz Shlomo <redacted>
Reviewed-by: Paul Blakey <redacted>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/lib/fs_chains.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Hi,
On Thu, Jan 07, 2021 at 09:30:47PM -0800, Saeed Mahameed wrote:
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
If it doesn't require stateful processing, please enlight me on why
conntrack is being used in the first place. What's the use case here?
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to the slow
path table.
AFAICU this new +trk+new ct table is a wildcard match on sport with
specific dports. Also AFAICU, such entries will not be visible to the
userspace then. Is this right?
Marcelo
I didn't test but I think this will add a hard dependency to
nf_conntrack_core and will cause conntrack to always be loaded by
mlx5_core, which is not good for some use cases.
nf_ct_tmpl_alloc() is defined in nf_conntrack_core.c.
762f926d6f19 ("net/sched: act_ct: Make tcf_ct_flow_table_restore_skb
inline") was done similarly to avoid this.
+ if (!ft->tmpl)
+ goto err_tmpl;
+
+ __set_bit(IPS_CONFIRMED_BIT, &ft->tmpl->status);
+ nf_conntrack_get(&ft->tmpl->ct_general);
+
err = nf_flow_table_offload_add_cb(ft->nf_ft,
mlx5_tc_ct_block_flow_offload, ft);
if (err)
From: Roi Dayan <hidden> Date: 2021-01-10 07:46:43
On 2021-01-08 11:48 PM, Marcelo Ricardo Leitner wrote:
Hi,
On Thu, Jan 07, 2021 at 09:30:47PM -0800, Saeed Mahameed wrote:
quoted
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
If it doesn't require stateful processing, please enlight me on why
conntrack is being used in the first place. What's the use case here?
The use case for example is when we have vxlan traffic but we do
conntrack on the inner packet (rules on the physical port) so
we never get established but on miss we can still offload as normal
vxlan traffic.
quoted
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to the slow
path table.
AFAICU this new +trk+new ct table is a wildcard match on sport with
specific dports. Also AFAICU, such entries will not be visible to the
userspace then. Is this right?
Marcelo
From: Roi Dayan <hidden> Date: 2021-01-10 07:53:58
On 2021-01-10 9:45 AM, Roi Dayan wrote:
On 2021-01-08 11:48 PM, Marcelo Ricardo Leitner wrote:
quoted
Hi,
On Thu, Jan 07, 2021 at 09:30:47PM -0800, Saeed Mahameed wrote:
quoted
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
If it doesn't require stateful processing, please enlight me on why
conntrack is being used in the first place. What's the use case here?
The use case for example is when we have vxlan traffic but we do
conntrack on the inner packet (rules on the physical port) so
we never get established but on miss we can still offload as normal
vxlan traffic.
my mistake about "inner packet". we do CT on the underlay network, i.e.
the outer header.
quoted
quoted
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to the
slow
path table.
AFAICU this new +trk+new ct table is a wildcard match on sport with
specific dports. Also AFAICU, such entries will not be visible to the
userspace then. Is this right?
Marcelo
I didn't test but I think this will add a hard dependency to
nf_conntrack_core and will cause conntrack to always be loaded by
mlx5_core, which is not good for some use cases.
nf_ct_tmpl_alloc() is defined in nf_conntrack_core.c.
762f926d6f19 ("net/sched: act_ct: Make tcf_ct_flow_table_restore_skb
inline") was done similarly to avoid this.
right. we will take a look what we can do with this.
thanks
quoted
+ if (!ft->tmpl)
+ goto err_tmpl;
+
+ __set_bit(IPS_CONFIRMED_BIT, &ft->tmpl->status);
+ nf_conntrack_get(&ft->tmpl->ct_general);
+
err = nf_flow_table_offload_add_cb(ft->nf_ft,
mlx5_tc_ct_block_flow_offload, ft);
if (err)
On Sun, Jan 10, 2021 at 09:52:55AM +0200, Roi Dayan wrote:
On 2021-01-10 9:45 AM, Roi Dayan wrote:
quoted
On 2021-01-08 11:48 PM, Marcelo Ricardo Leitner wrote:
quoted
Hi,
On Thu, Jan 07, 2021 at 09:30:47PM -0800, Saeed Mahameed wrote:
quoted
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
If it doesn't require stateful processing, please enlight me on why
conntrack is being used in the first place. What's the use case here?
The use case for example is when we have vxlan traffic but we do
conntrack on the inner packet (rules on the physical port) so
we never get established but on miss we can still offload as normal
vxlan traffic.
my mistake about "inner packet". we do CT on the underlay network, i.e.
the outer header.
I miss why the CT match is being used there then. Isn't it a config
issue/waste of resources? What is CT adding to the matches/actions
being done on these flows?
quoted
quoted
quoted
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to
the slow
path table.
AFAICU this new +trk+new ct table is a wildcard match on sport with
specific dports. Also AFAICU, such entries will not be visible to the
userspace then. Is this right?
Marcelo
From: Oz Shlomo <hidden> Date: 2021-01-12 09:27:52
On 1/12/2021 1:51 AM, Marcelo Ricardo Leitner wrote:
On Sun, Jan 10, 2021 at 09:52:55AM +0200, Roi Dayan wrote:
quoted
On 2021-01-10 9:45 AM, Roi Dayan wrote:
quoted
On 2021-01-08 11:48 PM, Marcelo Ricardo Leitner wrote:
quoted
Hi,
On Thu, Jan 07, 2021 at 09:30:47PM -0800, Saeed Mahameed wrote:
quoted
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
If it doesn't require stateful processing, please enlight me on why
conntrack is being used in the first place. What's the use case here?
The use case for example is when we have vxlan traffic but we do
conntrack on the inner packet (rules on the physical port) so
we never get established but on miss we can still offload as normal
vxlan traffic.
my mistake about "inner packet". we do CT on the underlay network, i.e.
the outer header.
I miss why the CT match is being used there then. Isn't it a config
issue/waste of resources? What is CT adding to the matches/actions
being done on these flows?
Consider a use case where the network port receives both east-west encapsulated traffic and
north-south non-encapsulated traffic that requires NAT.
One possible configuration is to first apply the CT-NAT action.
Established north-south connections will successfully execute the nat action and will set the +est
ct state.
However, the +new state may apply either for valid east-west traffic (e.g. vxlan) due to source port
entropy, or to insecure north-south traffic that the fw should block. The user may distinguish
between the two cases, for example, by matching on the dest udp port.
quoted
quoted
quoted
quoted
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to
the slow
path table.
AFAICU this new +trk+new ct table is a wildcard match on sport with
specific dports. Also AFAICU, such entries will not be visible to the
userspace then. Is this right?
Marcelo
On Tue, Jan 12, 2021 at 11:27:04AM +0200, Oz Shlomo wrote:
On 1/12/2021 1:51 AM, Marcelo Ricardo Leitner wrote:
quoted
On Sun, Jan 10, 2021 at 09:52:55AM +0200, Roi Dayan wrote:
quoted
On 2021-01-10 9:45 AM, Roi Dayan wrote:
quoted
On 2021-01-08 11:48 PM, Marcelo Ricardo Leitner wrote:
quoted
Hi,
On Thu, Jan 07, 2021 at 09:30:47PM -0800, Saeed Mahameed wrote:
quoted
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
If it doesn't require stateful processing, please enlight me on why
conntrack is being used in the first place. What's the use case here?
The use case for example is when we have vxlan traffic but we do
conntrack on the inner packet (rules on the physical port) so
we never get established but on miss we can still offload as normal
vxlan traffic.
my mistake about "inner packet". we do CT on the underlay network, i.e.
the outer header.
I miss why the CT match is being used there then. Isn't it a config
issue/waste of resources? What is CT adding to the matches/actions
being done on these flows?
Consider a use case where the network port receives both east-west
encapsulated traffic and north-south non-encapsulated traffic that requires
NAT.
One possible configuration is to first apply the CT-NAT action.
Established north-south connections will successfully execute the nat action
and will set the +est ct state.
However, the +new state may apply either for valid east-west traffic (e.g.
vxlan) due to source port entropy, or to insecure north-south traffic that
the fw should block. The user may distinguish between the two cases, for
example, by matching on the dest udp port.
Sorry but I still don't see the big picture. :-]
What do you consider as east-west and north-south traffic? My initial
understanding of east-west is traffic between VFs and north-south
would be in and out to the wire. You mentioned that north-south is
insecure, it would match, but then, non-encapsulated?
So it seems you referred to the datacenter. East-west is traffic
between hosts on the same datacenter, and north-south is traffic that
goes out of it. This seems to match.
Assuming it's the latter, then it seems that the idea is to work
around a config simplification that was done by the user. As
mentioned on the changelog, such protocols do not require stateful
processing, and AFAICU this patch twists conntrack so that the user
can have simplified rules. Why can't the user have specific rules for
the tunnels, and other for dealing with north-south traffic? The fw
would still be able to block unwanted traffic.
My main problems with this is this, that it is making conntrack do
stuff that the user may not be expecting it to do, and that packets
may get matched (maybe even unintentionally) and the system won't have
visibility on them. Maybe I'm just missing something?
quoted
quoted
quoted
quoted
quoted
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to
the slow
path table.
AFAICU this new +trk+new ct table is a wildcard match on sport with
specific dports. Also AFAICU, such entries will not be visible to the
userspace then. Is this right?
Marcelo
From: Oz Shlomo <hidden> Date: 2021-01-14 14:04:49
On 1/14/2021 3:02 PM, Marcelo Ricardo Leitner wrote:
On Tue, Jan 12, 2021 at 11:27:04AM +0200, Oz Shlomo wrote:
quoted
On 1/12/2021 1:51 AM, Marcelo Ricardo Leitner wrote:
quoted
On Sun, Jan 10, 2021 at 09:52:55AM +0200, Roi Dayan wrote:
quoted
On 2021-01-10 9:45 AM, Roi Dayan wrote:
quoted
On 2021-01-08 11:48 PM, Marcelo Ricardo Leitner wrote:
quoted
Hi,
On Thu, Jan 07, 2021 at 09:30:47PM -0800, Saeed Mahameed wrote:
quoted
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
If it doesn't require stateful processing, please enlight me on why
conntrack is being used in the first place. What's the use case here?
The use case for example is when we have vxlan traffic but we do
conntrack on the inner packet (rules on the physical port) so
we never get established but on miss we can still offload as normal
vxlan traffic.
my mistake about "inner packet". we do CT on the underlay network, i.e.
the outer header.
I miss why the CT match is being used there then. Isn't it a config
issue/waste of resources? What is CT adding to the matches/actions
being done on these flows?
Consider a use case where the network port receives both east-west
encapsulated traffic and north-south non-encapsulated traffic that requires
NAT.
One possible configuration is to first apply the CT-NAT action.
Established north-south connections will successfully execute the nat action
and will set the +est ct state.
However, the +new state may apply either for valid east-west traffic (e.g.
vxlan) due to source port entropy, or to insecure north-south traffic that
the fw should block. The user may distinguish between the two cases, for
example, by matching on the dest udp port.
Sorry but I still don't see the big picture. :-]
What do you consider as east-west and north-south traffic? My initial
understanding of east-west is traffic between VFs and north-south
would be in and out to the wire. You mentioned that north-south is
insecure, it would match, but then, non-encapsulated?
So it seems you referred to the datacenter. East-west is traffic
between hosts on the same datacenter, and north-south is traffic that
goes out of it. This seems to match.
Right.
Assuming it's the latter, then it seems that the idea is to work
around a config simplification that was done by the user. As
mentioned on the changelog, such protocols do not require stateful
processing, and AFAICU this patch twists conntrack so that the user
can have simplified rules. Why can't the user have specific rules for
the tunnels, and other for dealing with north-south traffic? The fw
would still be able to block unwanted traffic.
We cannot control what the user is doing.
This is a valid tc configuration and would work using tc software datapath.
However, in such configurations vxlan packets would not be processed in hardware because they are
marked as new connections.
My main problems with this is this, that it is making conntrack do
stuff that the user may not be expecting it to do, and that packets
may get matched (maybe even unintentionally) and the system won't have
visibility on them. Maybe I'm just missing something?
This is why we restricted this feature to udp protocols that will never enter established state due
to source port entropy.
Do you see a problematic use case that can arise?
quoted
quoted
quoted
quoted
quoted
quoted
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to
the slow
path table.
AFAICU this new +trk+new ct table is a wildcard match on sport with
specific dports. Also AFAICU, such entries will not be visible to the
userspace then. Is this right?
Marcelo
On Thu, Jan 14, 2021 at 04:03:43PM +0200, Oz Shlomo wrote:
On 1/14/2021 3:02 PM, Marcelo Ricardo Leitner wrote:
quoted
On Tue, Jan 12, 2021 at 11:27:04AM +0200, Oz Shlomo wrote:
quoted
On 1/12/2021 1:51 AM, Marcelo Ricardo Leitner wrote:
quoted
On Sun, Jan 10, 2021 at 09:52:55AM +0200, Roi Dayan wrote:
quoted
On 2021-01-10 9:45 AM, Roi Dayan wrote:
quoted
On 2021-01-08 11:48 PM, Marcelo Ricardo Leitner wrote:
quoted
Hi,
On Thu, Jan 07, 2021 at 09:30:47PM -0800, Saeed Mahameed wrote:
quoted
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
If it doesn't require stateful processing, please enlight me on why
conntrack is being used in the first place. What's the use case here?
The use case for example is when we have vxlan traffic but we do
conntrack on the inner packet (rules on the physical port) so
we never get established but on miss we can still offload as normal
vxlan traffic.
my mistake about "inner packet". we do CT on the underlay network, i.e.
the outer header.
I miss why the CT match is being used there then. Isn't it a config
issue/waste of resources? What is CT adding to the matches/actions
being done on these flows?
Consider a use case where the network port receives both east-west
encapsulated traffic and north-south non-encapsulated traffic that requires
NAT.
One possible configuration is to first apply the CT-NAT action.
Established north-south connections will successfully execute the nat action
and will set the +est ct state.
However, the +new state may apply either for valid east-west traffic (e.g.
vxlan) due to source port entropy, or to insecure north-south traffic that
the fw should block. The user may distinguish between the two cases, for
example, by matching on the dest udp port.
Sorry but I still don't see the big picture. :-]
What do you consider as east-west and north-south traffic? My initial
understanding of east-west is traffic between VFs and north-south
would be in and out to the wire. You mentioned that north-south is
insecure, it would match, but then, non-encapsulated?
So it seems you referred to the datacenter. East-west is traffic
between hosts on the same datacenter, and north-south is traffic that
goes out of it. This seems to match.
Right.
quoted
Assuming it's the latter, then it seems that the idea is to work
around a config simplification that was done by the user. As
mentioned on the changelog, such protocols do not require stateful
processing, and AFAICU this patch twists conntrack so that the user
can have simplified rules. Why can't the user have specific rules for
the tunnels, and other for dealing with north-south traffic? The fw
would still be able to block unwanted traffic.
We cannot control what the user is doing.
Right, but we can educate and point them towards better configs. With
non-optimal configs it's fair to expect non-optimal effects.
This is a valid tc configuration and would work using tc software datapath.
However, in such configurations vxlan packets would not be processed in
hardware because they are marked as new connections.
Makes sense.
quoted
My main problems with this is this, that it is making conntrack do
stuff that the user may not be expecting it to do, and that packets
may get matched (maybe even unintentionally) and the system won't have
visibility on them. Maybe I'm just missing something?
This is why we restricted this feature to udp protocols that will never
enter established state due to source port entropy.
Do you see a problematic use case that can arise?
For use case, the only one I see is if someone wants to use this
feature for another application/dstport. It's hardcoded to tunnels
ones.
It feels that the problem is not being solved at the right place. It
will work well for hardware processing, while for software it will
work while having a ton of conntrack entries. Different behaviors that
can lead to people wasting time. Like, trying to debug on why srcport
is not getting randomized when offloaded, while in fact they are, it's
just masked.
As this is a fallback (iow, search is done in 2 levels at least), I
wonder what other approaches were considered. I'm thinking two for
now. One is to add a flag to conntrack entries that allow them to be
this generic. Finding the right conntrack entry probably gets harder,
but when the user dumps /proc/net/nf_conntrack, it says something. On
how/when to add this flag, maybe act_ct can do it if dstport matches
something and/or a sysctl specifying a port list.
The other one may sound an overkill, but is to work with conntrack
expectations somehow.
The first one is closer to the current proposal. It basically makes
the port list configurable and move the "do it" decision to outside
the driver, where the admin can have more control. If conntrack itself
can also leverage it and avoid having tons of entries, even better, as
then we have both behaviors in sync.
Thoughts?
quoted
quoted
quoted
quoted
quoted
quoted
quoted
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to
the slow
path table.
AFAICU this new +trk+new ct table is a wildcard match on sport with
specific dports. Also AFAICU, such entries will not be visible to the
userspace then. Is this right?
Marcelo
From: Oz Shlomo <hidden> Date: 2021-01-20 16:12:20
On 1/14/2021 11:50 PM, Marcelo Ricardo Leitner wrote:
On Thu, Jan 14, 2021 at 04:03:43PM +0200, Oz Shlomo wrote:
quoted
On 1/14/2021 3:02 PM, Marcelo Ricardo Leitner wrote:
quoted
On Tue, Jan 12, 2021 at 11:27:04AM +0200, Oz Shlomo wrote:
quoted
On 1/12/2021 1:51 AM, Marcelo Ricardo Leitner wrote:
quoted
On Sun, Jan 10, 2021 at 09:52:55AM +0200, Roi Dayan wrote:
quoted
On 2021-01-10 9:45 AM, Roi Dayan wrote:
quoted
On 2021-01-08 11:48 PM, Marcelo Ricardo Leitner wrote:
quoted
Hi,
On Thu, Jan 07, 2021 at 09:30:47PM -0800, Saeed Mahameed wrote:
quoted
From: Roi Dayan <redacted>
Connection tracking associates the connection state per packet. The
first packet of a connection is assigned with the +trk+new state. The
connection enters the established state once a packet is seen on the
other direction.
Currently we offload only the established flows. However, UDP traffic
using source port entropy (e.g. vxlan, RoCE) will never enter the
established state. Such protocols do not require stateful processing,
and therefore could be offloaded.
If it doesn't require stateful processing, please enlight me on why
conntrack is being used in the first place. What's the use case here?
The use case for example is when we have vxlan traffic but we do
conntrack on the inner packet (rules on the physical port) so
we never get established but on miss we can still offload as normal
vxlan traffic.
my mistake about "inner packet". we do CT on the underlay network, i.e.
the outer header.
I miss why the CT match is being used there then. Isn't it a config
issue/waste of resources? What is CT adding to the matches/actions
being done on these flows?
Consider a use case where the network port receives both east-west
encapsulated traffic and north-south non-encapsulated traffic that requires
NAT.
One possible configuration is to first apply the CT-NAT action.
Established north-south connections will successfully execute the nat action
and will set the +est ct state.
However, the +new state may apply either for valid east-west traffic (e.g.
vxlan) due to source port entropy, or to insecure north-south traffic that
the fw should block. The user may distinguish between the two cases, for
example, by matching on the dest udp port.
Sorry but I still don't see the big picture. :-]
What do you consider as east-west and north-south traffic? My initial
understanding of east-west is traffic between VFs and north-south
would be in and out to the wire. You mentioned that north-south is
insecure, it would match, but then, non-encapsulated?
So it seems you referred to the datacenter. East-west is traffic
between hosts on the same datacenter, and north-south is traffic that
goes out of it. This seems to match.
Right.
quoted
Assuming it's the latter, then it seems that the idea is to work
around a config simplification that was done by the user. As
mentioned on the changelog, such protocols do not require stateful
processing, and AFAICU this patch twists conntrack so that the user
can have simplified rules. Why can't the user have specific rules for
the tunnels, and other for dealing with north-south traffic? The fw
would still be able to block unwanted traffic.
We cannot control what the user is doing.
Right, but we can educate and point them towards better configs. With
non-optimal configs it's fair to expect non-optimal effects.
quoted
This is a valid tc configuration and would work using tc software datapath.
However, in such configurations vxlan packets would not be processed in
hardware because they are marked as new connections.
Makes sense.
quoted
quoted
My main problems with this is this, that it is making conntrack do
stuff that the user may not be expecting it to do, and that packets
may get matched (maybe even unintentionally) and the system won't have
visibility on them. Maybe I'm just missing something?
This is why we restricted this feature to udp protocols that will never
enter established state due to source port entropy.
Do you see a problematic use case that can arise?
For use case, the only one I see is if someone wants to use this
feature for another application/dstport. It's hardcoded to tunnels
ones.
It's a hardware offload optimization feature.
This is why we chose to support specific protocols that explicitly define source port entropy.
It feels that the problem is not being solved at the right place. It
will work well for hardware processing, while for software it will
work while having a ton of conntrack entries. Different behaviors that
can lead to people wasting time. Like, trying to debug on why srcport
is not getting randomized when offloaded, while in fact they are, it's
just masked.
The SW and HW offload are functionally identical.
You are correct that with this patch the UNREPLIED CT entries will not be visible to the user
through /proc/net/nf_conntrack
As this is a fallback (iow, search is done in 2 levels at least), I
wonder what other approaches were considered. I'm thinking two for
now. One is to add a flag to conntrack entries that allow them to be
this generic. Finding the right conntrack entry probably gets harder,
but when the user dumps /proc/net/nf_conntrack, it says something. On
how/when to add this flag, maybe act_ct can do it if dstport matches
something and/or a sysctl specifying a port list.
The other one may sound an overkill, but is to work with conntrack
expectations somehow.
The first one is closer to the current proposal. It basically makes
the port list configurable and move the "do it" decision to outside
the driver, where the admin can have more control. If conntrack itself
can also leverage it and avoid having tons of entries, even better, as
then we have both behaviors in sync.
IIUC you propose a mechanism for avoiding CT processing of packets with a certain mask (e.g. based
on dst udp port). Configured by admin and enforced by act_ct or even conntrack itself.
If so, this seems like a fundamental change to nf conntrack requiring it to add packet
classification engines.
Thoughts?
I wonder if we should develop a generic mechanism to optimize CT software for a use case that is
faulty by design.
This has limited value for software as it would only reduce the conntrack table size (packet
classification is still required).
However, this feature may have a big impact on hardware offload.
Normally hardware offload relies on software to handle new connections. Causing all new connections
to be processed by software.
With this patch the hardware may autonomously set the +new connection state for the relevant
connections.
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
The change in the model is that a miss on the CT table will be forwarded
to a new +trk+new ct table and a miss there will be forwarded to
the slow
path table.
AFAICU this new +trk+new ct table is a wildcard match on sport with
specific dports. Also AFAICU, such entries will not be visible to the
userspace then. Is this right?
Marcelo
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2021-01-22 01:19:40
Hi Oz,
On Wed, Jan 20, 2021 at 06:09:48PM +0200, Oz Shlomo wrote:
On 1/14/2021 11:50 PM, Marcelo Ricardo Leitner wrote:
quoted
Thoughts?
I wonder if we should develop a generic mechanism to optimize CT software
for a use case that is faulty by design.
This has limited value for software as it would only reduce the conntrack
table size (packet classification is still required).
However, this feature may have a big impact on hardware offload.
Normally hardware offload relies on software to handle new connections.
Causing all new connections to be processed by software.
With this patch the hardware may autonomously set the +new connection state
for the relevant connections.
Could you fix this issue with unidirectional flows by checking for
IPS_CONFIRMED status bit? The idea is to hardware offload the entry
after the first packet goes through software successfully. Then, there
is no need to wait for the established state that requires to see
traffic in both directions.
On Fri, Jan 22, 2021 at 02:18:34AM +0100, Pablo Neira Ayuso wrote:
Hi Oz,
On Wed, Jan 20, 2021 at 06:09:48PM +0200, Oz Shlomo wrote:
quoted
On 1/14/2021 11:50 PM, Marcelo Ricardo Leitner wrote:
quoted
Thoughts?
I wonder if we should develop a generic mechanism to optimize CT software
for a use case that is faulty by design.
This has limited value for software as it would only reduce the conntrack
table size (packet classification is still required).
However, this feature may have a big impact on hardware offload.
Normally hardware offload relies on software to handle new connections.
Causing all new connections to be processed by software.
With this patch the hardware may autonomously set the +new connection state
for the relevant connections.
Could you fix this issue with unidirectional flows by checking for
IPS_CONFIRMED status bit? The idea is to hardware offload the entry
after the first packet goes through software successfully. Then, there
is no need to wait for the established state that requires to see
traffic in both directions.
That's an interesting idea. This way, basically all that needs to be
changed is tcf_ct_flow_table_process_conn() to handle this new
condition for UDP packets and on tcf_ct_act().
It has a small performance penaulty if compared to the original
solution, as now the first packet(s) goes to sw, but looks like a good
compromise between supporting a (from what I could understand)
somewhat lazy flow design (as I still think these didn't need to go
through conntrack), an uniform system behavior (with and without
offload, with mlx5 or another driver) and a more generic approach.
Other situations that rely on unidirectional UDP flows will benefit
from it as well.
This way I even think it doesn't need to be configurable right now.
It will be easier to add a knob to switch back to the old behavior if
needed later on, if anything.
Marcelo
From: Oz Shlomo <hidden> Date: 2021-01-26 04:48:52
On 1/22/2021 4:16 AM, Marcelo Ricardo Leitner wrote:
On Fri, Jan 22, 2021 at 02:18:34AM +0100, Pablo Neira Ayuso wrote:
quoted
Hi Oz,
On Wed, Jan 20, 2021 at 06:09:48PM +0200, Oz Shlomo wrote:
quoted
On 1/14/2021 11:50 PM, Marcelo Ricardo Leitner wrote:
quoted
Thoughts?
I wonder if we should develop a generic mechanism to optimize CT software
for a use case that is faulty by design.
This has limited value for software as it would only reduce the conntrack
table size (packet classification is still required).
However, this feature may have a big impact on hardware offload.
Normally hardware offload relies on software to handle new connections.
Causing all new connections to be processed by software.
With this patch the hardware may autonomously set the +new connection state
for the relevant connections.
Could you fix this issue with unidirectional flows by checking for
IPS_CONFIRMED status bit? The idea is to hardware offload the entry
after the first packet goes through software successfully. Then, there
is no need to wait for the established state that requires to see
traffic in both directions.
That's an interesting idea. This way, basically all that needs to be
changed is tcf_ct_flow_table_process_conn() to handle this new
condition for UDP packets and on tcf_ct_act().
Will act_ct need to maintain a port list and classify the packet to realize whether the udp packet
is part of a unidirection or biderectional udp connection?
It has a small performance penaulty if compared to the original
solution, as now the first packet(s) goes to sw, but looks like a good
compromise between supporting a (from what I could understand)
somewhat lazy flow design (as I still think these didn't need to go
through conntrack), an uniform system behavior (with and without
offload, with mlx5 or another driver) and a more generic approach.
Other situations that rely on unidirectional UDP flows will benefit
from it as well.
The hardware offload perspective is a bit different.
With this approach the system will offload a rule per connection instead of offloading one mega-flow
rule on dst udp port.
This will increase the hardware scale requirements in terms of number of offloaded rules.
In addition, a counter will need to be instantiated per rule and the software will need to manage
the aging of these connections.
We hoped that the hardware can fully offload this scenario, avoiding the need for sw processing at all.
This way I even think it doesn't need to be configurable right now.
It will be easier to add a knob to switch back to the old behavior if
needed later on, if anything.
Marcelo