Re: [PATCH net-next v4 2/2] openvswitch: Support conntrack zone limit

2018-05-23 Thread Pravin Shelar
On Mon, May 21, 2018 at 5:16 PM, Yi-Hung Wei  wrote:
> Currently, nf_conntrack_max is used to limit the maximum number of
> conntrack entries in the conntrack table for every network namespace.
> For the VMs and containers that reside in the same namespace,
> they share the same conntrack table, and the total # of conntrack entries
> for all the VMs and containers are limited by nf_conntrack_max.  In this
> case, if one of the VM/container abuses the usage the conntrack entries,
> it blocks the others from committing valid conntrack entries into the
> conntrack table.  Even if we can possibly put the VM in different network
> namespace, the current nf_conntrack_max configuration is kind of rigid
> that we cannot limit different VM/container to have different # conntrack
> entries.
>
> To address the aforementioned issue, this patch proposes to have a
> fine-grained mechanism that could further limit the # of conntrack entries
> per-zone.  For example, we can designate different zone to different VM,
> and set conntrack limit to each zone.  By providing this isolation, a
> mis-behaved VM only consumes the conntrack entries in its own zone, and
> it will not influence other well-behaved VMs.  Moreover, the users can
> set various conntrack limit to different zone based on their preference.
>
> The proposed implementation utilizes Netfilter's nf_conncount backend
> to count the number of connections in a particular zone.  If the number of
> connection is above a configured limitation, ovs will return ENOMEM to the
> userspace.  If userspace does not configure the zone limit, the limit
> defaults to zero that is no limitation, which is backward compatible to
> the behavior without this patch.
>
> The following high leve APIs are provided to the userspace:
>   - OVS_CT_LIMIT_CMD_SET:
> * set default connection limit for all zones
> * set the connection limit for a particular zone
>   - OVS_CT_LIMIT_CMD_DEL:
> * remove the connection limit for a particular zone
>   - OVS_CT_LIMIT_CMD_GET:
> * get the default connection limit for all zones
> * get the connection limit for a particular zone
>
> Signed-off-by: Yi-Hung Wei 

I have few comments, but otherwise patch looks good.
> ---
>  net/openvswitch/Kconfig |   3 +-
>  net/openvswitch/conntrack.c | 541 
> +++-
>  net/openvswitch/conntrack.h |   9 +-
>  net/openvswitch/datapath.c  |   7 +-
>  net/openvswitch/datapath.h  |   3 +
>  5 files changed, 557 insertions(+), 6 deletions(-)
>
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 2650205cdaf9..89da9512ec1e 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -9,7 +9,8 @@ config OPENVSWITCH
>(NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
>  (!NF_NAT || NF_NAT) && \
>  (!NF_NAT_IPV4 || NF_NAT_IPV4) && \
> -(!NF_NAT_IPV6 || NF_NAT_IPV6)))
> +(!NF_NAT_IPV6 || NF_NAT_IPV6) && \
> +(!NETFILTER_CONNCOUNT || 
> NETFILTER_CONNCOUNT)))
> select LIBCRC32C
> select MPLS
> select NET_MPLS_GSO
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 02fc343feb66..e8bb91420ca9 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -16,8 +16,11 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -76,6 +79,31 @@ struct ovs_conntrack_info {
>  #endif
>  };
>
> +#ifIS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
> +#define OVS_CT_LIMIT_UNLIMITED 0
> +#define OVS_CT_LIMIT_DEFAULT OVS_CT_LIMIT_UNLIMITED
> +#define CT_LIMIT_HASH_BUCKETS 512
> +static DEFINE_STATIC_KEY_FALSE(ovs_ct_limit_enabled);
> +
> +struct ovs_ct_limit {
> +   /* Elements in ovs_ct_limit_info->limits hash table */
> +   struct hlist_node hlist_node;
> +   struct rcu_head rcu;
> +   u16 zone;
> +   u32 limit;
> +};
> +
> +struct ovs_ct_limit_info {
> +   u32 default_limit;
> +   struct hlist_head *limits;
> +   struct nf_conncount_data *data __aligned(8);

Why does it need explicit alignment attribute?

> +};
> +
> +static const struct nla_policy ct_limit_policy[OVS_CT_LIMIT_ATTR_MAX + 1] = {
> +   [OVS_CT_LIMIT_ATTR_ZONE_LIMIT] = { .type = NLA_NESTED, },
> +};
> +#endif
> +
>  static bool labels_nonzero(const struct ovs_key_ct_labels *labels);

...

> +static int ovs_ct_check_limit(struct net *net,
> + const struct ovs_conntrack_info *info,
> + const struct nf_conntrack_tuple *tuple)
> +{
> +   struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
> +   const struct ovs_ct_limit_info *ct_limit_info = 
> ovs_net->ct_limit_info;
> +   u32 per_zone_limit, connections;
> +   u

[PATCH net-next v4 2/2] openvswitch: Support conntrack zone limit

2018-05-21 Thread Yi-Hung Wei
Currently, nf_conntrack_max is used to limit the maximum number of
conntrack entries in the conntrack table for every network namespace.
For the VMs and containers that reside in the same namespace,
they share the same conntrack table, and the total # of conntrack entries
for all the VMs and containers are limited by nf_conntrack_max.  In this
case, if one of the VM/container abuses the usage the conntrack entries,
it blocks the others from committing valid conntrack entries into the
conntrack table.  Even if we can possibly put the VM in different network
namespace, the current nf_conntrack_max configuration is kind of rigid
that we cannot limit different VM/container to have different # conntrack
entries.

To address the aforementioned issue, this patch proposes to have a
fine-grained mechanism that could further limit the # of conntrack entries
per-zone.  For example, we can designate different zone to different VM,
and set conntrack limit to each zone.  By providing this isolation, a
mis-behaved VM only consumes the conntrack entries in its own zone, and
it will not influence other well-behaved VMs.  Moreover, the users can
set various conntrack limit to different zone based on their preference.

The proposed implementation utilizes Netfilter's nf_conncount backend
to count the number of connections in a particular zone.  If the number of
connection is above a configured limitation, ovs will return ENOMEM to the
userspace.  If userspace does not configure the zone limit, the limit
defaults to zero that is no limitation, which is backward compatible to
the behavior without this patch.

The following high leve APIs are provided to the userspace:
  - OVS_CT_LIMIT_CMD_SET:
* set default connection limit for all zones
* set the connection limit for a particular zone
  - OVS_CT_LIMIT_CMD_DEL:
* remove the connection limit for a particular zone
  - OVS_CT_LIMIT_CMD_GET:
* get the default connection limit for all zones
* get the connection limit for a particular zone

Signed-off-by: Yi-Hung Wei 
---
 net/openvswitch/Kconfig |   3 +-
 net/openvswitch/conntrack.c | 541 +++-
 net/openvswitch/conntrack.h |   9 +-
 net/openvswitch/datapath.c  |   7 +-
 net/openvswitch/datapath.h  |   3 +
 5 files changed, 557 insertions(+), 6 deletions(-)

diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
index 2650205cdaf9..89da9512ec1e 100644
--- a/net/openvswitch/Kconfig
+++ b/net/openvswitch/Kconfig
@@ -9,7 +9,8 @@ config OPENVSWITCH
   (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
 (!NF_NAT || NF_NAT) && \
 (!NF_NAT_IPV4 || NF_NAT_IPV4) && \
-(!NF_NAT_IPV6 || NF_NAT_IPV6)))
+(!NF_NAT_IPV6 || NF_NAT_IPV6) && \
+(!NETFILTER_CONNCOUNT || 
NETFILTER_CONNCOUNT)))
select LIBCRC32C
select MPLS
select NET_MPLS_GSO
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 02fc343feb66..e8bb91420ca9 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -16,8 +16,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -76,6 +79,31 @@ struct ovs_conntrack_info {
 #endif
 };
 
+#ifIS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
+#define OVS_CT_LIMIT_UNLIMITED 0
+#define OVS_CT_LIMIT_DEFAULT OVS_CT_LIMIT_UNLIMITED
+#define CT_LIMIT_HASH_BUCKETS 512
+static DEFINE_STATIC_KEY_FALSE(ovs_ct_limit_enabled);
+
+struct ovs_ct_limit {
+   /* Elements in ovs_ct_limit_info->limits hash table */
+   struct hlist_node hlist_node;
+   struct rcu_head rcu;
+   u16 zone;
+   u32 limit;
+};
+
+struct ovs_ct_limit_info {
+   u32 default_limit;
+   struct hlist_head *limits;
+   struct nf_conncount_data *data __aligned(8);
+};
+
+static const struct nla_policy ct_limit_policy[OVS_CT_LIMIT_ATTR_MAX + 1] = {
+   [OVS_CT_LIMIT_ATTR_ZONE_LIMIT] = { .type = NLA_NESTED, },
+};
+#endif
+
 static bool labels_nonzero(const struct ovs_key_ct_labels *labels);
 
 static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
@@ -1036,6 +1064,89 @@ static bool labels_nonzero(const struct 
ovs_key_ct_labels *labels)
return false;
 }
 
+#ifIS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
+static struct hlist_head *ct_limit_hash_bucket(
+   const struct ovs_ct_limit_info *info, u16 zone)
+{
+   return &info->limits[zone & (CT_LIMIT_HASH_BUCKETS - 1)];
+}
+
+/* Call with ovs_mutex */
+static void ct_limit_set(const struct ovs_ct_limit_info *info,
+struct ovs_ct_limit *new_ct_limit)
+{
+   struct ovs_ct_limit *ct_limit;
+   struct hlist_head *head;
+
+   head = ct_limit_hash_bucket(info, new_ct_limit->zone);
+   hlist_for_each_entry_rcu(ct_limit, head, hlist_node) {
+