netdev - Re: [PATCH net-next 4/6] net/sched: act_ct: Create nf flow table per zone

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Feb 2020 11:44:52 +0200
From:   Paul Blakey <paulb@...lanox.com>
To:     Edward Cree <ecree@...arflare.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Oz Shlomo <ozsh@...lanox.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Vlad Buslov <vladbu@...lanox.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>, Roi Dayan <roid@...lanox.com>
Subject: Re: [PATCH net-next 4/6] net/sched: act_ct: Create nf flow table per
 zone


On 2/24/2020 5:58 PM, Edward Cree wrote:
> On 23/02/2020 11:45, Paul Blakey wrote:
>> Use the NF flow tables infrastructure for CT offload.
>>
>> Create a nf flow table per zone.
>>
>> Next patches will add FT entries to this table, and do
>> the software offload.
>>
>> Signed-off-by: Paul Blakey <paulb@...lanox.com>
>> Reviewed-by: Jiri Pirko <jiri@...lanox.com>
>> ---
>>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c |   1 +
>>  include/net/tc_act/tc_ct.h                      |   2 +
>>  net/sched/Kconfig                               |   2 +-
>>  net/sched/act_ct.c                              | 159 +++++++++++++++++++++++-
>>  4 files changed, 162 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> index 70b5fe2..eb16136 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> @@ -45,6 +45,7 @@
>>  #include <net/tc_act/tc_tunnel_key.h>
>>  #include <net/tc_act/tc_pedit.h>
>>  #include <net/tc_act/tc_csum.h>
>> +#include <net/tc_act/tc_ct.h>
>>  #include <net/arp.h>
>>  #include <net/ipv6_stubs.h>
>>  #include "en.h"
>> diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h
>> index a8b1564..cf3492e 100644
>> --- a/include/net/tc_act/tc_ct.h
>> +++ b/include/net/tc_act/tc_ct.h
>> @@ -25,6 +25,8 @@ struct tcf_ct_params {
>>  	u16 ct_action;
>>  
>>  	struct rcu_head rcu;
>> +
>> +	struct tcf_ct_flow_table *ct_ft;
>>  };
>>  
>>  struct tcf_ct {
>> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
>> index edde0e5..bfbefb7 100644
>> --- a/net/sched/Kconfig
>> +++ b/net/sched/Kconfig
>> @@ -972,7 +972,7 @@ config NET_ACT_TUNNEL_KEY
>>  
>>  config NET_ACT_CT
>>  	tristate "connection tracking tc action"
>> -	depends on NET_CLS_ACT && NF_CONNTRACK && NF_NAT
>> +	depends on NET_CLS_ACT && NF_CONNTRACK && NF_NAT && NF_FLOW_TABLE
> Is it not possible to keep sensible/old behaviour in the case
>  of NF_FLOW_TABLE=n?  (And what about NF_FLOW_TABLE=m, which is
>  what its Kconfig help seems to advise...)

No problem with it being a module.

It is possible to allow compilation without flow table, but it will create confusion for people trying

to offload conntrack, as it would silently not happen.

>
>>  	help
>>  	  Say Y here to allow sending the packets to conntrack module.
>>  
>> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> index f685c0d..4267d7d 100644
>> --- a/net/sched/act_ct.c
>> +++ b/net/sched/act_ct.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/pkt_cls.h>
>>  #include <linux/ip.h>
>>  #include <linux/ipv6.h>
>> +#include <linux/rhashtable.h>
>>  #include <net/netlink.h>
>>  #include <net/pkt_sched.h>
>>  #include <net/pkt_cls.h>
>> @@ -24,6 +25,7 @@
>>  #include <uapi/linux/tc_act/tc_ct.h>
>>  #include <net/tc_act/tc_ct.h>
>>  
>> +#include <net/netfilter/nf_flow_table.h>
>>  #include <net/netfilter/nf_conntrack.h>
>>  #include <net/netfilter/nf_conntrack_core.h>
>>  #include <net/netfilter/nf_conntrack_zones.h>
>> @@ -31,6 +33,133 @@
>>  #include <net/netfilter/ipv6/nf_defrag_ipv6.h>
>>  #include <uapi/linux/netfilter/nf_nat.h>
>>  
>> +static struct workqueue_struct *act_ct_wq;
>> +
>> +struct tcf_ct_flow_table {
>> +	struct rhash_head node; /* In zones tables */
>> +
>> +	struct rcu_work rwork;
>> +	struct nf_flowtable nf_ft;
>> +	u16 zone;
>> +	u32 ref;
> Any reason this isn't using a refcount_t?
> -ed
it is updated under lock, so there was no need for atomic as well