Re: [ovs-dev] [PATCH v9 2/2] netdev-dpdk:Detailed packet drop statistics

[Sriram Vatala via dev]

Hi,
Please consider this as gentle remainder and kindly review the patch set.

Thanks & Regards,
Sriram.

-Original Message-
From: Sriram Vatala  
Sent: 11 October 2019 16:58
To: 'ovs-dev@openvswitch.org' ;
'i.maxim...@ovn.org' 
Subject: RE: [PATCH v9 2/2] netdev-dpdk:Detailed packet drop statistics

Hi,
Please consider this as a gentle remainder.

Thanks & Regards,
Sriram.

-Original Message-
From: Sriram Vatala  
Sent: 09 October 2019 11:53
To: 'ovs-dev@openvswitch.org' 
Cc: 'ktray...@redhat.com' ; 'Ilya Maximets'
; 'i.maxim...@ovn.org' 
Subject: RE: [PATCH v9 2/2] netdev-dpdk:Detailed packet drop statistics

Hi All,
Please consider this as a gentle remainder.

Thanks & Regards,
Sriram.

-Original Message-
From: Sriram Vatala  
Sent: 04 October 2019 13:37
To: 'ovs-dev@openvswitch.org' 
Cc: 'ktray...@redhat.com' ; 'Ilya Maximets'

Subject: RE: [PATCH v9 2/2] netdev-dpdk:Detailed packet drop statistics

Hi,
Please consider this as a gentle remainder.

Thanks & Regards,
Sriram.

-Original Message-
From: Sriram Vatala  
Sent: 25 September 2019 09:58
To: 'ovs-dev@openvswitch.org' 
Cc: 'ktray...@redhat.com' ; 'Ilya Maximets'

Subject: RE: [PATCH v9 2/2] netdev-dpdk:Detailed packet drop statistics

Hi,
Addressed comments given in patch v8. Kindly review patch v9.
 
Changes since patch v8 :
Rebased my changes on top of Ilya's patch " Reuse vhost function for dpdk
ETH custom stats". Patch set 1/2.
Addressed comments given in patch v8.

Thanks & Regards,
Sriram.

-Original Message-
From: Sriram Vatala  
Sent: 21 September 2019 08:10
To: ovs-dev@openvswitch.org; i.maxim...@samsung.com
Cc: ktray...@redhat.com; Sriram Vatala 
Subject: [PATCH v9 2/2] netdev-dpdk:Detailed packet drop statistics

OVS may be unable to transmit packets for multiple reasons and today there
is a single counter to track packets dropped due to any of those reasons.
The most common reason is that a VM is unable to read packets fast enough
causing the vhostuser port transmit queue on the OVS side to become full.
This manifests as a problem with VNFs not receiving all packets. Having a
separate drop counter to track packets dropped because the transmit queue is
full will clearly indicate that the problem is on the VM side and not in
OVS. Similarly maintaining separate counters for all possible drops helps in
indicating sensible cause for packet drops.

This patch adds custom software stats counters to track packets dropped at
port level and these counters are displayed along with other stats in
"ovs-vsctl get interface  statistics"
command. The detailed stats will be available for both dpdk and vhostuser
ports.

--
Changes since v8:
Addressed comments given by Ilya.

Signed-off-by: Sriram Vatala 
---
 Documentation/topics/dpdk/vhost-user.rst  | 13 ++-
 lib/netdev-dpdk.c | 81 +++
 utilities/bugtool/automake.mk |  3 +-
 utilities/bugtool/ovs-bugtool-get-port-stats  | 25 ++
 .../plugins/network-status/openvswitch.xml|  1 +
 5 files changed, 105 insertions(+), 18 deletions(-)  create mode 100755
utilities/bugtool/ovs-bugtool-get-port-stats

diff --git a/Documentation/topics/dpdk/vhost-user.rst
b/Documentation/topics/dpdk/vhost-user.rst
index 724aa62f6..89388a2bf 100644
--- a/Documentation/topics/dpdk/vhost-user.rst
+++ b/Documentation/topics/dpdk/vhost-user.rst
@@ -551,7 +551,18 @@ processing packets at the required rate.
 The amount of Tx retries on a vhost-user or vhost-user-client interface can
be  shown with::
 
-  $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries
+  $ ovs-vsctl get Interface dpdkvhostclient0 
+ statistics:netdev_dpdk_tx_retries
+
+When the guest is not able to consume the packets fast enough, the 
+transmit queue of the port gets filled up i.e queue runs out of free
descriptors.
+This is the most likely reason why dpdk transmit API will fail to send 
+packets besides other reasons.
+
+The amount of tx failure drops on a dpdk vhost/physical interface can 
+be shown with::
+
+  $ ovs-vsctl get Interface dpdkvhostclient0 \
+statistics:netdev_dpdk_tx_failure_drops
 
 vhost-user Dequeue Zero Copy (experimental)
 ---
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
652b57e3b..fd8f9102e 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -174,6 +174,20 @@ static const struct vhost_device_ops
virtio_net_device_ops =
 .destroy_connection = destroy_connection,  };
 
+/* Custom software stats for dpdk ports */ struct netdev_dpdk_sw_stats 
+{
+/* No. of retries when unable to transmit. */
+uint64_t tx_retries;
+/* Packet drops when unable to transmit; Probably Tx queue is full. */
+uint64_t tx_failure_drops;
+/* Packet length greater than device MTU. */
+uint64_t tx_mtu_exceeded_drops;
+/* Packet drops in egress policer processing. */
+uint64_t tx_qos_drops;
+/* Packet drops in ingress policer 

Re: [ovs-dev] [PATCH 2/2] datapath: Make metadata_dst tunnel work in IP_TUNNEL_INFO_BRIDGE mode

[wenxu]

On 10/15/2019 5:14 AM, Gregory Rose wrote:
>
>
> On 10/8/2019 4:51 PM, Greg Rose wrote:
>> From: wenxu 
>>
>> Upstream commit:
>>  commit 18b6f717483a835fb98de9f0df6c724df9324e78
>>  Author: wenxu 
>>  Date:   Thu Mar 28 12:43:23 2019 +0800
>>
>>  openvswitch: Make metadata_dst tunnel work in IP_TUNNEL_INFO_BRIDGE mode
>>
>>  There is currently no support for the multicast/broadcast aspects
>>  of VXLAN in ovs. In the datapath flow the tun_dst must specific.
>>  But in the IP_TUNNEL_INFO_BRIDGE mode the tun_dst can not be specific.
>>  And the packet can forward through the fdb table of vxlan devcice. In
>>  this mode the broadcast/multicast packet can be sent through the
>>  following ways in ovs.
>>
>>  ovs-vsctl add-port br0 vxlan -- set in vxlan type=vxlan \
>>  options:key=1000 options:remote_ip=flow
>>  ovs-ofctl add-flow br0 in_port=LOCAL,dl_dst=ff:ff:ff:ff:ff:ff, \
>>  action=output:vxlan
>>
>>  bridge fdb append ff:ff:ff:ff:ff:ff dev vxlan_sys_4789 dst 172.168.0.1 \
>>  src_vni 1000 vni 1000 self
>>  bridge fdb append ff:ff:ff:ff:ff:ff dev vxlan_sys_4789 dst 172.168.0.2 \
>>  src_vni 1000 vni 1000 self
>>
>>  Signed-off-by: wenxu 
>>  Acked-by: Pravin B Shelar 
>>  Signed-off-by: David S. Miller 
>>
>> Also fixup case statement in lib/odp-util.c to make the compiler happy.
>>
>> Cc: wenxu 
>> Signed-off-by: Greg Rose 
>
> Hi Wenxu,
>
> would you mind giving this patch a review - I just want to make sure I didn't 
> miss anything.
>
> Thanks,
>
> - Greg
>
Looks good to me!


Thanks

wenxu

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] ovs container build.sh requires python3

[aginwala]

Hi Ben:

Can you also push this to OVS master? Got it merged in ovn repo already.

On Fri, Oct 11, 2019 at 2:06 PM  wrote:

> From: Aliasgar Ginwala 
>
> building ovn/ovs container breaks while configure:
> checking for Python 3 (version 3.4 or later)... no
> configure: error: Python 3.4 or later is required but not found in
> /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin,
> please install it or set  to point to it
>
> As per commit 1ca0323e7c29dc7ef5a615c265df0460208f92de
> Require Python 3 and remove support for Python 2.
>
> Signed-off-by: Aliasgar Ginwala 
> ---
>  utilities/docker/debian/build-kernel-modules.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/utilities/docker/debian/build-kernel-modules.sh
> b/utilities/docker/debian/build-kernel-modules.sh
> index 1b12720b9..18ac35764 100755
> --- a/utilities/docker/debian/build-kernel-modules.sh
> +++ b/utilities/docker/debian/build-kernel-modules.sh
> @@ -18,8 +18,8 @@ GITHUB_SRC=$3
>
>  # Install deps
>  linux="linux-image-$KERNEL_VERSION linux-headers-$KERNEL_VERSION"
> -build_deps="apt-utils libelf-dev build-essential libssl-dev python \
> -python-six wget gdb autoconf libtool git automake bzip2 debhelper \
> +build_deps="apt-utils libelf-dev build-essential libssl-dev python3 \
> +python3-six wget gdb autoconf libtool git automake bzip2 debhelper \
>  dh-autoreconf openssl"
>
>  apt-get update
> --
> 2.20.1 (Apple Git-117)
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1 ovn] ovn-nb/sbctl.c: Use env variables for passing options.

[aginwala]

On Mon, Oct 14, 2019 at 4:38 PM Ben Pfaff  wrote:

> On Mon, Oct 14, 2019 at 03:33:25PM -0700, Ben Pfaff wrote:
> > On Wed, Oct 09, 2019 at 04:56:40PM -0700, amgin...@gmail.com wrote:
> > > From: Aliasgar Ginwala 
> > >
> > > Add new env variables OVN_NBCTL_OPTIONS and OVN_SBCTL_OPTIONS for
> > > ovn-nbctl and ovn-sbctl respectively where user can set any single
> > > supported option. e.g export OVN_NBCTL_OPTIONS=--no-leader-only.
> > > Above env var OVN_NBCTL_OPTIONS have no effect if user runs
> > > command as ovn-nbctl --no-leader-only 
> > >
> > > Signed-off-by: Aliasgar Ginwala 
> >
> > I think that this could be factored out into lib/command-line.c rather
> > than done as a copy-and-paste into two files.
> >
> > I don't think that the ops_passed variable needs to be static.
>
> Also, I'd consider giving an example in the documentation of how to use
> this new variable.  The purpose is unclear enough that I might not be
> able to quickly guess how or why to use it without an example.
>
Thanks for the review Ben:
I can move that logic to command-line.c to new function in ovs repo
as ovs_cmdl_env_parse_all and use it in nbctl and sbctl.   Will update
example in doc accordingly in v2.

> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Important

[info.nmooreba]

Nice to meet you,

There is a vital issue I need to discuss with you urgently. Confirm your
email valid for conscious security reason.

Best regards,
My contact E-mail: info...@zoho.com
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 03/11] datapath: add seqadj extension when NAT is used.

[Flavio Leitner]

On Mon, 14 Oct 2019 10:37:43 -0700
Yi-Hung Wei  wrote:

> From: Flavio Leitner 
> 
> upstream patch:
> 
> commit fa7e428c6b7ed3281610511a2b2ec716d9894be8
> Author: Flavio Leitner 
> Date:   Mon Mar 25 15:58:31 2019 -0300
> 
> openvswitch: add seqadj extension when NAT is used.
> 
> When the conntrack is initialized, there is no helper attached
> yet so the nat info initialization (nf_nat_setup_info) skips
> adding the seqadj ext.
> 
> A helper is attached later when the conntrack is not confirmed
> but is going to be committed. In this case, if NAT is needed then
> adds the seqadj ext as well.
> 
> Fixes: 16ec3d4fbb96 ("openvswitch: Fix cached ct with helper.")
> Signed-off-by: Flavio Leitner 
> Acked-by: Pravin B Shelar 
> Signed-off-by: David S. Miller 
> 
> Signed-off-by: Yi-Hung Wei 


LGTM
fbl


> ---
>  datapath/conntrack.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 291d4f4723d9..1b345a03e704 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1063,6 +1063,12 @@ static int __ovs_ct_lookup(struct net *net,
> struct sw_flow_key *key, GFP_ATOMIC);
>   if (err)
>   return err;
> +
> + /* helper installed, add seqadj if NAT is
> required */
> + if (info->nat && !nfct_seqadj(ct)) {
> + if (!nfct_seqadj_ext_add(ct))
> + return -EINVAL;
> + }
>   }
>  
>   /* Call the helper only if:

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1 ovn] ovn-nb/sbctl.c: Use env variables for passing options.

[Ben Pfaff]

On Mon, Oct 14, 2019 at 03:33:25PM -0700, Ben Pfaff wrote:
> On Wed, Oct 09, 2019 at 04:56:40PM -0700, amgin...@gmail.com wrote:
> > From: Aliasgar Ginwala 
> > 
> > Add new env variables OVN_NBCTL_OPTIONS and OVN_SBCTL_OPTIONS for
> > ovn-nbctl and ovn-sbctl respectively where user can set any single
> > supported option. e.g export OVN_NBCTL_OPTIONS=--no-leader-only.
> > Above env var OVN_NBCTL_OPTIONS have no effect if user runs
> > command as ovn-nbctl --no-leader-only 
> > 
> > Signed-off-by: Aliasgar Ginwala 
> 
> I think that this could be factored out into lib/command-line.c rather
> than done as a copy-and-paste into two files.
> 
> I don't think that the ops_passed variable needs to be static.

Also, I'd consider giving an example in the documentation of how to use
this new variable.  The purpose is unclear enough that I might not be
able to quickly guess how or why to use it without an example.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovs-vlan-bug-workaround: Remove.

[Ben Pfaff]

On Mon, Oct 14, 2019 at 03:18:01PM -0700, Gregory Rose wrote:
> On 10/10/2019 12:07 PM, Ben Pfaff wrote:
> > This workaround only applied to kernels earlier than 2.6.37, but OVS
> > only supports 3.10 and later.
> > 
> > As the original author of this code, I won't miss it.
> > 
> > Signed-off-by: Ben Pfaff 
> 
> Nice.
> 
> I applied the patch and compile tested it.  I then ran 'make check' and
> 'make check-kmod' and found no regression
> or other issues.  Reviewing the code it looks fine to me - reviewing patches
> that remove stuff is easier.
> 
> Thanks Ben, always a plus to get rid of old gunk.
> 
> Tested-by: Greg Rose 
> Reviewed-by: Greg Rose 

Thanks a lot, Greg.  I applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 11/11] datapath: Allow attaching helper in later commit

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:56 AM Yi-Hung Wei  wrote:
>
> Upstream commit:
> commit 248d45f1e1934f7849fbdc35ef1e57151cf063eb
> Author: Yi-Hung Wei 
> Date:   Fri Oct 4 09:26:44 2019 -0700
>
> openvswitch: Allow attaching helper in later commit
>
> This patch allows to attach conntrack helper to a confirmed conntrack
> entry.  Currently, we can only attach alg helper to a conntrack entry
> when it is in the unconfirmed state.  This patch enables an use case
> that we can firstly commit a conntrack entry after it passed some
> initial conditions.  After that the processing pipeline will further
> check a couple of packets to determine if the connection belongs to
> a particular application, and attach alg helper to the connection
> in a later stage.
>
> Signed-off-by: Yi-Hung Wei 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  datapath/conntrack.c | 21 +
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index f6e9386f4707..838cf63c908f 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1045,6 +1045,8 @@ static int __ovs_ct_lookup(struct net *net, struct 
> sw_flow_key *key,
>
> ct = nf_ct_get(skb, );
> if (ct) {
> +   bool add_helper = false;
> +
> /* Packets starting a new connection must be NATted before the
>  * helper, so that the helper knows about the NAT.  We enforce
>  * this by delaying both NAT and helper calls for unconfirmed
> @@ -1062,16 +1064,17 @@ static int __ovs_ct_lookup(struct net *net, struct 
> sw_flow_key *key,
> }
>
> /* Userspace may decide to perform a ct lookup without a 
> helper
> -* specified followed by a (recirculate and) commit with one.
> -* Therefore, for unconfirmed connections which we will 
> commit,
> -* we need to attach the helper here.
> +* specified followed by a (recirculate and) commit with one,
> +* or attach a helper in a later commit.  Therefore, for
> +* connections which we will commit, we may need to attach
> +* the helper here.
>  */
> -   if (!nf_ct_is_confirmed(ct) && info->commit &&
> -   info->helper && !nfct_help(ct)) {
> +   if (info->commit && info->helper && !nfct_help(ct)) {
> int err = __nf_ct_try_assign_helper(ct, info->ct,
> GFP_ATOMIC);
> if (err)
> return err;
> +   add_helper = true;
>
> /* helper installed, add seqadj if NAT is required */
> if (info->nat && !nfct_seqadj(ct)) {
> @@ -1081,11 +1084,13 @@ static int __ovs_ct_lookup(struct net *net, struct 
> sw_flow_key *key,
> }
>
> /* Call the helper only if:
> -* - nf_conntrack_in() was executed above ("!cached") for a
> -*   confirmed connection, or
> +* - nf_conntrack_in() was executed above ("!cached") or a
> +*   helper was just attached ("add_helper") for a confirmed
> +*   connection, or
>  * - When committing an unconfirmed connection.
>  */
> -   if ((nf_ct_is_confirmed(ct) ? !cached : info->commit) &&
> +   if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
> + info->commit) &&
> ovs_ct_helper(skb, info->family) != NF_ACCEPT) {
> return -EINVAL;
> }
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 10/11] datapath: Fix log message in ovs conntrack

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:55 AM Yi-Hung Wei  wrote:
>
> Upstream commit:
> commit 12c6bc38f99bb168b7f16bdb5e855a51a23ee9ec
> Author: Yi-Hung Wei 
> Date:   Wed Aug 21 17:16:10 2019 -0700
>
> openvswitch: Fix log message in ovs conntrack
>
> Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action")
> Signed-off-by: Yi-Hung Wei 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  datapath/conntrack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index ba73962b2214..f6e9386f4707 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1663,7 +1663,7 @@ static int parse_ct(const struct nlattr *attr, struct 
> ovs_conntrack_info *info,
> case OVS_CT_ATTR_TIMEOUT:
> memcpy(info->timeout, nla_data(a), nla_len(a));
> if (!memchr(info->timeout, '\0', nla_len(a))) {
> -   OVS_NLERR(log, "Invalid conntrack helper");
> +   OVS_NLERR(log, "Invalid conntrack timeout");
> return -EINVAL;
> }
> break;
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 09/11] datapath: Replace removed NF_NAT_NEEDED with IS_ENABLED(CONFIG_NF_NAT)

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:55 AM Yi-Hung Wei  wrote:
>
> Backports the following upstream commit with some backward compatibility
> change.
>
> commit f319ca6557c10a711facc4dd60197470796d3ec1
> Author: Geert Uytterhoeven 
> Date:   Wed May 8 08:52:32 2019 +0200
>
> openvswitch: Replace removed NF_NAT_NEEDED with IS_ENABLED(CONFIG_NF_NAT)
>
> Commit 4806e975729f99c7 ("netfilter: replace NF_NAT_NEEDED with
> IS_ENABLED(CONFIG_NF_NAT)") removed CONFIG_NF_NAT_NEEDED, but a new user
> popped up afterwards.
>
> Fixes: fec9c271b8f1bde1 ("openvswitch: load and reference the NAT 
> helper.")
> Signed-off-by: Geert Uytterhoeven 
> Acked-by: Florian Westphal 
> Acked-by: Flavio Leitner 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  datapath/conntrack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 86e7dd24bb9b..ba73962b2214 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1406,7 +1406,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info 
> *info, const char *name,
> return -ENOMEM;
> }
>
> -#ifdef CONFIG_NF_NAT_NEEDED
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
> if (info->nat) {
> ret = nf_nat_helper_try_module_get(name, info->family,
>key->ip.proto);
> @@ -1909,7 +1909,7 @@ void ovs_ct_free_action(const struct nlattr *a)
>  static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
>  {
> if (ct_info->helper) {
> -#ifdef CONFIG_NF_NAT_NEEDED
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
> if (ct_info->nat)
> nf_nat_helper_put(ct_info->helper);
>  #endif
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 08/11] datapath: Check for null pointer return from nla_nest_start_noflag

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:54 AM Yi-Hung Wei  wrote:
>
> From: Colin Ian King 
>
> upstream commit:
>
> commit ca96534630e2edfd73121c487c957b17eca3b7d7
> Author: Colin Ian King 
> Date:   Wed May 1 14:41:58 2019 +0100
>
> openvswitch: check for null pointer return from nla_nest_start_noflag
>
> The call to nla_nest_start_noflag can return null in the unlikely
> event that nla_put returns -EMSGSIZE.  Check for this condition to
> avoid a null pointer dereference on pointer nla_reply.
>
> Addresses-Coverity: ("Dereference null return value")
> Fixes: 11efd5cb04a1 ("openvswitch: Support conntrack zone limit")
> Signed-off-by: Colin Ian King 
> Acked-by: Yi-Hung Wei 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  datapath/conntrack.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 9a7eab655142..86e7dd24bb9b 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -2273,6 +2273,10 @@ static int ovs_ct_limit_cmd_get(struct sk_buff *skb, 
> struct genl_info *info)
> return PTR_ERR(reply);
>
> nla_reply = nla_nest_start_noflag(reply, 
> OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
> +   if (!nla_reply) {
> +   err = -EMSGSIZE;
> +   goto exit_err;
> +   }
>
> if (a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]) {
> err = ovs_ct_limit_get_zone_limit(
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 07/11] datapath: Load and reference the NAT helper.

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:54 AM Yi-Hung Wei  wrote:
>
> This commit backports the following upstream commit, and two functions
> in nf_conntrack_helper.h.
>
> Upstream commit:
> commit fec9c271b8f1bde1086be5aa415cdb586e0dc800
> Author: Flavio Leitner 
> Date:   Wed Apr 17 11:46:17 2019 -0300
>
> openvswitch: load and reference the NAT helper.
>
> This improves the original commit 17c357efe5ec ("openvswitch: load
> NAT helper") where it unconditionally tries to load the module for
> every flow using NAT, so not efficient when loading multiple flows.
> It also doesn't hold any references to the NAT module while the
> flow is active.
>
> This change fixes those problems. It will try to load the module
> only if it's not present. It grabs a reference to the NAT module
> and holds it while the flow is active. Finally, an error message
> shows up if either actions above fails.
>
> Fixes: 17c357efe5ec ("openvswitch: load NAT helper")
> Signed-off-by: Flavio Leitner 
> Signed-off-by: Pablo Neira Ayuso 
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  acinclude.m4   |  4 
>  datapath/conntrack.c   | 27 
> +-
>  .../include/net/netfilter/nf_conntrack_helper.h| 17 ++
>  3 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index 055f5387db19..22f92723b00d 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -904,6 +904,10 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_helper.h],
>[nf_conntrack_helper_put],
>[OVS_DEFINE(HAVE_NF_CONNTRACK_HELPER_PUT)])
> +  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_helper.h],
> +  [nf_nat_helper_try_module_get])
> +  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_helper.h],
> +  [nf_nat_helper_put])
>
> OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h],:space:]]]SKB_GSO_UDP[[[:space:,
>[OVS_DEFINE([HAVE_SKB_GSO_UDP])])
>OVS_GREP_IFELSE([$KSRC/include/net/dst.h],[DST_NOCACHE],
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 0c0d43bec2e5..9a7eab655142 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1391,6 +1391,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info 
> *info, const char *name,
>  {
> struct nf_conntrack_helper *helper;
> struct nf_conn_help *help;
> +   int ret = 0;
>
> helper = nf_conntrack_helper_try_module_get(name, info->family,
> key->ip.proto);
> @@ -1405,13 +1406,22 @@ static int ovs_ct_add_helper(struct 
> ovs_conntrack_info *info, const char *name,
> return -ENOMEM;
> }
>
> +#ifdef CONFIG_NF_NAT_NEEDED
> +   if (info->nat) {
> +   ret = nf_nat_helper_try_module_get(name, info->family,
> +  key->ip.proto);
> +   if (ret) {
> +   nf_conntrack_helper_put(helper);
> +   OVS_NLERR(log, "Failed to load \"%s\" NAT helper, 
> error: %d",
> + name, ret);
> +   return ret;
> +   }
> +   }
> +#endif
> +
> rcu_assign_pointer(help->helper, helper);
> info->helper = helper;
> -
> -   if (info->nat)
> -   request_module("ip_nat_%s", name);
> -
> -   return 0;
> +   return ret;
>  }
>
>  #if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
> @@ -1898,8 +1908,13 @@ void ovs_ct_free_action(const struct nlattr *a)
>
>  static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
>  {
> -   if (ct_info->helper)
> +   if (ct_info->helper) {
> +#ifdef CONFIG_NF_NAT_NEEDED
> +   if (ct_info->nat)
> +   nf_nat_helper_put(ct_info->helper);
> +#endif
> nf_conntrack_helper_put(ct_info->helper);
> +   }
> if (ct_info->ct) {
> if (ct_info->timeout[0])
> nf_ct_destroy_timeout(ct_info->ct);
> diff --git 
> a/datapath/linux/compat/include/net/netfilter/nf_conntrack_helper.h 
> b/datapath/linux/compat/include/net/netfilter/nf_conntrack_helper.h
> index b6a3d0bf75b3..78f97375b66e 100644
> --- a/datapath/linux/compat/include/net/netfilter/nf_conntrack_helper.h
> +++ b/datapath/linux/compat/include/net/netfilter/nf_conntrack_helper.h
> @@ -19,4 +19,21 @@ rpl_nf_ct_helper_ext_add(struct nf_conn *ct,
>  #define nf_ct_helper_ext_add rpl_nf_ct_helper_ext_add
>  #endif /* HAVE_NF_CT_HELPER_EXT_ADD_TAKES_HELPER */
>
> +#ifndef HAVE_NF_NAT_HELPER_TRY_MODULE_GET
> +static inline int rpl_nf_nat_helper_try_module_get(const char *name, u16 
> l3num,
> +  u8 protonum)
> +{
> +   

Re: [ovs-dev] [PATCH 06/11] datapath: genetlink: optionally validate strictly/dumps

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:53 AM Yi-Hung Wei  wrote:
>
> This patch backports the following upstream commit within the
> openvswitch kernel module with some checks so that it also works
> in the older kernel.
>
> Upstream commit:
> commit ef6243acb4782df587a4d7d6c310fa5b5d82684b
> Author: Johannes Berg 
> Date:   Fri Apr 26 14:07:31 2019 +0200
>
> genetlink: optionally validate strictly/dumps
>
> Add options to strictly validate messages and dump messages,
> sometimes perhaps validating dump messages non-strictly may
> be required, so add an option for that as well.
>
> Since none of this can really be applied to existing commands,
> set the options everwhere using the following spatch:
>
> @@
> identifier ops;
> expression X;
> @@
> struct genl_ops ops[] = {
> ...,
>  {
> .cmd = X,
> +   .validate = GENL_DONT_VALIDATE_STRICT | 
> GENL_DONT_VALIDATE_DUMP,
> ...
>  },
> ...
> };
>
> For new commands one should just not copy the .validate 'opt-out'
> flags and thus get strict validation.
>
> Signed-off-by: Johannes Berg 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  acinclude.m4 |  1 +
>  datapath/conntrack.c |  9 +
>  datapath/datapath.c  | 39 +++
>  datapath/meter.c | 12 
>  4 files changed, 61 insertions(+)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index fe121ab9126d..055f5387db19 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -817,6 +817,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>OVS_GREP_IFELSE([$KSRC/include/net/genetlink.h], [genlmsg_parse])
>OVS_GREP_IFELSE([$KSRC/include/net/genetlink.h], [genl_notify.*family],
>[OVS_DEFINE([HAVE_GENL_NOTIFY_TAKES_FAMILY])])
> +  OVS_GREP_IFELSE([$KSRC/include/net/genetlink.h], [genl_validate_flags])
>OVS_FIND_PARAM_IFELSE([$KSRC/include/net/genetlink.h],
>  [genl_notify], [net],
>  [OVS_DEFINE([HAVE_GENL_NOTIFY_TAKES_NET])])
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index b11a30965147..0c0d43bec2e5 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -2283,18 +2283,27 @@ exit_err:
>
>  static struct genl_ops ct_limit_genl_ops[] = {
> { .cmd = OVS_CT_LIMIT_CMD_SET,
> +#ifdef HAVE_GENL_VALIDATE_FLAGS
> +   .validate = GENL_DONT_VALIDATE_STRICT | 
> GENL_DONT_VALIDATE_DUMP,
> +#endif
> .flags = GENL_ADMIN_PERM, /* Requires CAP_NET_ADMIN
>* privilege. */
> .policy = ct_limit_policy,
> .doit = ovs_ct_limit_cmd_set,
> },
> { .cmd = OVS_CT_LIMIT_CMD_DEL,
> +#ifdef HAVE_GENL_VALIDATE_FLAGS
> +   .validate = GENL_DONT_VALIDATE_STRICT | 
> GENL_DONT_VALIDATE_DUMP,
> +#endif
> .flags = GENL_ADMIN_PERM, /* Requires CAP_NET_ADMIN
>* privilege. */
> .policy = ct_limit_policy,
> .doit = ovs_ct_limit_cmd_del,
> },
> { .cmd = OVS_CT_LIMIT_CMD_GET,
> +#ifdef HAVE_GENL_VALIDATE_FLAGS
> +   .validate = GENL_DONT_VALIDATE_STRICT | 
> GENL_DONT_VALIDATE_DUMP,
> +#endif
> .flags = 0,   /* OK for unprivileged users. */
> .policy = ct_limit_policy,
> .doit = ovs_ct_limit_cmd_get,
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 78e2e6310529..f4244ea09869 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -652,6 +652,9 @@ static const struct nla_policy 
> packet_policy[OVS_PACKET_ATTR_MAX + 1] = {
>
>  static struct genl_ops dp_packet_genl_ops[] = {
> { .cmd = OVS_PACKET_CMD_EXECUTE,
> +#ifdef HAVE_GENL_VALIDATE_FLAGS
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +#endif
>   .flags = GENL_UNS_ADMIN_PERM, /* Requires CAP_NET_ADMIN privilege. 
> */
>   .policy = packet_policy,
>   .doit = ovs_packet_cmd_execute
> @@ -1440,22 +1443,34 @@ static const struct nla_policy 
> flow_policy[OVS_FLOW_ATTR_MAX + 1] = {
>
>  static struct genl_ops dp_flow_genl_ops[] = {
> { .cmd = OVS_FLOW_CMD_NEW,
> +#ifdef HAVE_GENL_VALIDATE_FLAGS
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +#endif
>   .flags = GENL_UNS_ADMIN_PERM, /* Requires CAP_NET_ADMIN privilege. 
> */
>   .policy = flow_policy,
>   .doit = ovs_flow_cmd_new
> },
> { .cmd = OVS_FLOW_CMD_DEL,
> +#ifdef HAVE_GENL_VALIDATE_FLAGS
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +#endif
>   .flags = GENL_UNS_ADMIN_PERM, /* Requires CAP_NET_ADMIN privilege. 
> */
>   .policy = flow_policy,
> 

Re: [ovs-dev] [PATCH 05/11] datapath: Use nla_nest_start_noflag()

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:53 AM Yi-Hung Wei  wrote:
>
> This patch backports the openvswitch changes and update the compat layer
> for the following upstream patch.
>
> commit ae0be8de9a53cda3505865c11826d8ff0640237c
> Author: Michal Kubecek 
> Date:   Fri Apr 26 11:13:06 2019 +0200
>
> netlink: make nla_nest_start() add NLA_F_NESTED flag
>
> Even if the NLA_F_NESTED flag was introduced more than 11 years ago, most
> netlink based interfaces (including recently added ones) are still not
> setting it in kernel generated messages. Without the flag, message parsers
> not aware of attribute semantics (e.g. wireshark dissector or libmnl's
> mnl_nlmsg_fprintf()) cannot recognize nested attributes and won't display
> the structure of their contents.
>
> Unfortunately we cannot just add the flag everywhere as there may be
> userspace applications which check nlattr::nla_type directly rather than
> through a helper masking out the flags. Therefore the patch renames
> nla_nest_start() to nla_nest_start_noflag() and introduces 
> nla_nest_start()
> as a wrapper adding NLA_F_NESTED. The calls which add NLA_F_NESTED 
> manually
> are rewritten to use nla_nest_start().
>
> Except for changes in include/net/netlink.h, the patch was generated using
> this semantic patch:
>
> @@ expression E1, E2; @@
> -nla_nest_start(E1, E2)
> +nla_nest_start_noflag(E1, E2)
>
> @@ expression E1, E2; @@
> -nla_nest_start_noflag(E1, E2 | NLA_F_NESTED)
> +nla_nest_start(E1, E2)
>
> Signed-off-by: Michal Kubecek 
> Acked-by: Jiri Pirko 
> Acked-by: David Ahern 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  acinclude.m4|  1 +
>  datapath/conntrack.c|  6 +++---
>  datapath/datapath.c |  7 +++---
>  datapath/flow_netlink.c | 33 
> +++--
>  datapath/linux/compat/include/net/netlink.h |  9 
>  datapath/meter.c|  8 +++
>  datapath/vport-vxlan.c  |  2 +-
>  datapath/vport.c|  2 +-
>  8 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index dca09abefa96..fe121ab9126d 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -844,6 +844,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_put_in_addr])
>OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_find_nested])
>OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_is_last])
> +  OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_nest_start_noflag])
>OVS_GREP_IFELSE([$KSRC/include/linux/netlink.h], [void.*netlink_set_err],
>[OVS_DEFINE([HAVE_VOID_NETLINK_SET_ERR])])
>OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netlink.h],
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 010f9af5ffd2..b11a30965147 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1776,7 +1776,7 @@ static bool ovs_ct_nat_to_attr(const struct 
> ovs_conntrack_info *info,
>  {
> struct nlattr *start;
>
> -   start = nla_nest_start(skb, OVS_CT_ATTR_NAT);
> +   start = nla_nest_start_noflag(skb, OVS_CT_ATTR_NAT);
> if (!start)
> return false;
>
> @@ -1847,7 +1847,7 @@ int ovs_ct_action_to_attr(const struct 
> ovs_conntrack_info *ct_info,
>  {
> struct nlattr *start;
>
> -   start = nla_nest_start(skb, OVS_ACTION_ATTR_CT);
> +   start = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_CT);
> if (!start)
> return -EMSGSIZE;
>
> @@ -2257,7 +2257,7 @@ static int ovs_ct_limit_cmd_get(struct sk_buff *skb, 
> struct genl_info *info)
> if (IS_ERR(reply))
> return PTR_ERR(reply);
>
> -   nla_reply = nla_nest_start(reply, OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
> +   nla_reply = nla_nest_start_noflag(reply, 
> OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
>
> if (a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]) {
> err = ovs_ct_limit_get_zone_limit(
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 94e4f6ffd6e9..78e2e6310529 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -475,7 +475,8 @@ static int queue_userspace_packet(struct datapath *dp, 
> struct sk_buff *skb,
>
>
> if (upcall_info->egress_tun_info) {
> -   nla = nla_nest_start(user_skb, 
> OVS_PACKET_ATTR_EGRESS_TUN_KEY);
> +   nla = nla_nest_start_noflag(user_skb,
> +   OVS_PACKET_ATTR_EGRESS_TUN_KEY);
> if (!nla) {
> err = -EMSGSIZE;
> goto out;
> @@ -487,7 +488,7 @@ static int queue_userspace_packet(struct datapath *dp, 
> struct sk_buff *skb,
> }
>
> if 

Re: [ovs-dev] [PATCH 04/11] datapath: Handle NF_NAT_NEEDED replacement

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:52 AM Yi-Hung Wei  wrote:
>
> Starting from the following upstream commit, NF_NAT_NEEDED is replaced
> by IS_ENABLED(CONFIG_NF_NAT) in the upstream kernel. This patch makes
> some changes so that our in tree ovs kernel module is compatible to
> both old and new kernels.
>
> Upstream commit:
> commit 4806e975729f99c7908d1688a143f1e16d464e6c
> Author: Florian Westphal 
> Date:   Wed Mar 27 09:22:26 2019 +0100
>
> netfilter: replace NF_NAT_NEEDED with IS_ENABLED(CONFIG_NF_NAT)
>
> NF_NAT_NEEDED is true whenever nat support for either ipv4 or ipv6 is
> enabled.  Now that the af-specific nat configuration switches have been
> removed, IS_ENABLED(CONFIG_NF_NAT) has the same effect.
>
> Signed-off-by: Florian Westphal 
> Signed-off-by: Pablo Neira Ayuso 
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  acinclude.m4 |  1 +
>  datapath/conntrack.c | 25 +
>  2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index cc80026f2127..dca09abefa96 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -676,6 +676,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>OVS_FIND_FIELD_IFELSE([$KSRC/include/linux/netfilter.h], [nf_hook_ops],
>  [owner], [OVS_DEFINE([HAVE_NF_HOOKS_OPS_OWNER])])
>OVS_GREP_IFELSE([$KSRC/include/linux/netfilter.h], [NFPROTO_INET])
> +  OVS_GREP_IFELSE([$KSRC/include/linux/netfilter.h], [CONFIG_NF_NAT_NEEDED])
>
>
>OVS_FIND_FIELD_IFELSE([$KSRC/include/linux/netfilter_ipv6.h], 
> [nf_ipv6_ops],
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 1b345a03e704..010f9af5ffd2 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -34,7 +34,16 @@
>  #include 
>  #include 
>
> -#ifdef CONFIG_NF_NAT_NEEDED
> +/* Upstream commit 4806e975729f ("netfilter: replace NF_NAT_NEEDED with
> + * IS_ENABLED(CONFIG_NF_NAT)") replaces the config checking on NF_NAT_NEEDED
> + * with CONFIG_NF_NAT.  We will replace the checking on NF_NAT_NEEDED for the
> + * newer kernel with the marco in order to keep backward compatiblity.
> + */
> +#ifndef HAVE_CONFIG_NF_NAT_NEEDED
> +#define CONFIG_NF_NAT_NEEDED  CONFIG_NF_NAT
> +#endif
> +
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
>  /* Starting from upstream commit 3bf195ae6037 ("netfilter: nat: merge
>   * nf_nat_ipv4,6 into nat core") in kernel 5.1.  nf_nat_ipv4,6 are merged
>   * into nf_nat.  In order to keep backward compatibility, we keep the config
> @@ -100,7 +109,7 @@ struct ovs_conntrack_info {
> struct md_labels labels;
> char timeout[CTNL_TIMEOUT_NAME_MAX];
> struct nf_ct_timeout *nf_ct_timeout;
> -#ifdef CONFIG_NF_NAT_NEEDED
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
> struct nf_nat_range2 range;  /* Only present for SRC NAT and DST NAT. 
> */
>  #endif
>  };
> @@ -786,7 +795,7 @@ static bool skb_nfct_cached(struct net *net,
> return ct_executed;
>  }
>
> -#ifdef CONFIG_NF_NAT_NEEDED
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
>  /* Modelled after nf_nat_ipv[46]_fn().
>   * range is only used for new, uninitialized NAT state.
>   * Returns either NF_ACCEPT or NF_DROP.
> @@ -1405,7 +1414,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info 
> *info, const char *name,
> return 0;
>  }
>
> -#ifdef CONFIG_NF_NAT_NEEDED
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
>  static int parse_nat(const struct nlattr *attr,
>  struct ovs_conntrack_info *info, bool log)
>  {
> @@ -1547,7 +1556,7 @@ static const struct ovs_ct_len_tbl 
> ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
> .maxlen = sizeof(struct md_labels) },
> [OVS_CT_ATTR_HELPER]= { .minlen = 1,
> .maxlen = NF_CT_HELPER_NAME_LEN },
> -#ifdef CONFIG_NF_NAT_NEEDED
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
> /* NAT length is checked when parsing the nested attributes. */
> [OVS_CT_ATTR_NAT]   = { .minlen = 0, .maxlen = INT_MAX },
>  #endif
> @@ -1627,7 +1636,7 @@ static int parse_ct(const struct nlattr *attr, struct 
> ovs_conntrack_info *info,
> return -EINVAL;
> }
> break;
> -#ifdef CONFIG_NF_NAT_NEEDED
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
> case OVS_CT_ATTR_NAT: {
> int err = parse_nat(a, info, log);
>
> @@ -1761,7 +1770,7 @@ err_free_ct:
> return err;
>  }
>
> -#ifdef CONFIG_NF_NAT_NEEDED
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
>  static bool ovs_ct_nat_to_attr(const struct ovs_conntrack_info *info,
>struct sk_buff *skb)
>  {
> @@ -1871,7 +1880,7 @@ int ovs_ct_action_to_attr(const struct 
> ovs_conntrack_info *ct_info,
> return -EMSGSIZE;
> }
>
> -#ifdef CONFIG_NF_NAT_NEEDED
> +#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
> if (ct_info->nat && 

Re: [ovs-dev] [PATCH 02/11] datapath: Detect upstream nf_nat change

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:51 AM Yi-Hung Wei  wrote:
>
> The following two upstream commits merge nf_nat_ipv4 and nf_nat_ipv6
> into nf_nat core, and move some header files around.  To handle
> these modifications, this patch detects the upstream changes, uses
> the header files and config symbols properly.
>
> Ideally, we should replace CONFIG_NF_NAT_IPV4 and CONFIG_NF_NAT_IPV6 with
> CONFIG_NF_NAT and CONFIG_IPV6.  In order to keep backward compatibility,
> we keep the checking of CONFIG_NF_NAT_IPV4/6 as is for the old kernel,
> and replace them with marco for the new kernel.
>
> upstream commits:
> 3bf195ae6037 ("netfilter: nat: merge nf_nat_ipv4,6 into nat core")
> d2c5c103b133 ("netfilter: nat: remove nf_nat_l3proto.h and nf_nat_core.h")
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  acinclude.m4 |  2 ++
>  datapath/conntrack.c | 13 -
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index 4072a7c8f58a..cc80026f2127 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -713,6 +713,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_nat.h], 
> [nf_ct_nat_ext_add])
>OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_nat.h], 
> [nf_nat_alloc_null_binding])
>OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_nat.h], [nf_nat_range2])
> +  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_nat.h], [nf_nat_packet],
> +  [OVS_DEFINE([HAVE_UPSTREAM_NF_NAT])])
>OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_seqadj.h], 
> [nf_ct_seq_adjust])
>OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_count.h], 
> [nf_conncount_gc_list],
>
> [OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_count.h],
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index afdd65b4cb7c..291d4f4723d9 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -35,10 +35,21 @@
>  #include 
>
>  #ifdef CONFIG_NF_NAT_NEEDED
> +/* Starting from upstream commit 3bf195ae6037 ("netfilter: nat: merge
> + * nf_nat_ipv4,6 into nat core") in kernel 5.1.  nf_nat_ipv4,6 are merged
> + * into nf_nat.  In order to keep backward compatibility, we keep the config
> + * checking as is for the old kernel, and replace them with marco for the
> + * new kernel. */
> +#ifdef HAVE_UPSTREAM_NF_NAT
> +#include 
> +#define CONFIG_NF_NAT_IPV4 CONFIG_NF_NAT
> +#define CONFIG_NF_NAT_IPV6 CONFIG_IPV6
> +#else
>  #include 
>  #include 
>  #include 
> -#endif
> +#endif /* HAVE_UPSTREAM_NF_NAT */
> +#endif /* CONFIG_NF_NAT_NEEDED */
>
>  #include "datapath.h"
>  #include "conntrack.h"
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 03/11] datapath: add seqadj extension when NAT is used.

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 10:51 AM Yi-Hung Wei  wrote:
>
> From: Flavio Leitner 
>
> upstream patch:
>
> commit fa7e428c6b7ed3281610511a2b2ec716d9894be8
> Author: Flavio Leitner 
> Date:   Mon Mar 25 15:58:31 2019 -0300
>
> openvswitch: add seqadj extension when NAT is used.
>
> When the conntrack is initialized, there is no helper attached
> yet so the nat info initialization (nf_nat_setup_info) skips
> adding the seqadj ext.
>
> A helper is attached later when the conntrack is not confirmed
> but is going to be committed. In this case, if NAT is needed then
> adds the seqadj ext as well.
>
> Fixes: 16ec3d4fbb96 ("openvswitch: Fix cached ct with helper.")
> Signed-off-by: Flavio Leitner 
> Acked-by: Pravin B Shelar 
> Signed-off-by: David S. Miller 
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  datapath/conntrack.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index 291d4f4723d9..1b345a03e704 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -1063,6 +1063,12 @@ static int __ovs_ct_lookup(struct net *net, struct 
> sw_flow_key *key,
> GFP_ATOMIC);
> if (err)
> return err;
> +
> +   /* helper installed, add seqadj if NAT is required */
> +   if (info->nat && !nfct_seqadj(ct)) {
> +   if (!nfct_seqadj_ext_add(ct))
> +   return -EINVAL;
> +   }
> }
>
> /* Call the helper only if:
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 01/11] datapath: Replace nf_ct_invert_tuplepr() with nf_ct_invert_tuple()

[Yifeng Sun]

A minor issue in commit message: pl_nf_ct_invert_tuple => rpl_nf_ct_invert_tuple

Other than that, LGTM, thanks.

Reviewed-by: Yifeng Sun 


On Mon, Oct 14, 2019 at 10:50 AM Yi-Hung Wei  wrote:
>
> After upstream net-next commit 303e0c558959 ("netfilter: conntrack:
> avoid unneeded nf_conntrack_l4proto lookups") nf_ct_invert_tuplepr()
> is no longer available in the kernel.
>
> Ideally, we should be in sync with upstream kernel by calling
> nf_ct_invert_tuple() directly in conntrack.c.  However,
> nf_ct_invert_tuple() has different function signature in older kernel,
> and it would be hard to replace that in the compat layer. Thus, we
> use pl_nf_ct_invert_tuple() in conntrack.c and maintain compatibility
> in the compat layer so that ovs kernel module runs smoothly in both
> new and old kernel.
>
> Signed-off-by: Yi-Hung Wei 
> ---
>  acinclude.m4   |  2 ++
>  datapath/conntrack.c   |  2 +-
>  .../linux/compat/include/net/netfilter/nf_conntrack_core.h | 14 
> ++
>  3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index 52f92870eaaa..4072a7c8f58a 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -697,6 +697,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>[nf_ct_set])
>OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack.h],
>[nf_ct_is_untracked])
> +  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack.h],
> +  [nf_ct_invert_tuplepr])
>OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_zones.h],
>[nf_ct_zone_init])
>OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_l3proto.h],
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index e328afe1ad15..afdd65b4cb7c 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -668,7 +668,7 @@ ovs_ct_find_existing(struct net *net, const struct 
> nf_conntrack_zone *zone,
> if (natted) {
> struct nf_conntrack_tuple inverse;
>
> -   if (!nf_ct_invert_tuplepr(, )) {
> +   if (!rpl_nf_ct_invert_tuple(, )) {
> pr_debug("ovs_ct_find_existing: Inversion failed!\n");
> return NULL;
> }
> diff --git a/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h 
> b/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h
> index 10158011fd4d..ad52bc9412d8 100644
> --- a/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h
> +++ b/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h
> @@ -113,4 +113,18 @@ rpl_nf_conntrack_in(struct sk_buff *skb, const struct 
> nf_hook_state *state)
>  #define nf_conntrack_in rpl_nf_conntrack_in
>  #endif /* HAVE_NF_CONNTRACK_IN_TAKES_NF_HOOK_STATE */
>
> +#ifdef HAVE_NF_CT_INVERT_TUPLEPR
> +static inline bool rpl_nf_ct_invert_tuple(struct nf_conntrack_tuple *inverse,
> +   const struct nf_conntrack_tuple *orig)
> +{
> +   return nf_ct_invert_tuplepr(inverse, orig);
> +}
> +#else
> +static inline bool rpl_nf_ct_invert_tuple(struct nf_conntrack_tuple *inverse,
> +   const struct nf_conntrack_tuple *orig)
> +{
> +   return nf_ct_invert_tuple(inverse, orig);
> +}
> +#endif /* HAVE_NF_CT_INVERT_TUPLEPR */
> +
>  #endif /* _NF_CONNTRACK_CORE_WRAPPER_H */
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ofproto-dpif-xlate: Restore table ID on error in xlate_table_action().

[Ben Pfaff]

Found by inspection.

Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto-dpif-xlate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f92cb62c80ce..0fa5d8a7c61b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4369,6 +4369,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t 
in_port, uint8_t table_id,
 !is_ip_any(>xin->flow)) {
 xlate_report_error(ctx,
"resubmit(ct) with non-tracked or non-IP 
packet!");
+ctx->table_id = old_table_id;
 return;
 }
 tuple_swap(>xin->flow, ctx->wc);
-- 
2.21.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1 ovn] ovn-nb/sbctl.c: Use env variables for passing options.

[Ben Pfaff]

On Wed, Oct 09, 2019 at 04:56:40PM -0700, amgin...@gmail.com wrote:
> From: Aliasgar Ginwala 
> 
> Add new env variables OVN_NBCTL_OPTIONS and OVN_SBCTL_OPTIONS for
> ovn-nbctl and ovn-sbctl respectively where user can set any single
> supported option. e.g export OVN_NBCTL_OPTIONS=--no-leader-only.
> Above env var OVN_NBCTL_OPTIONS have no effect if user runs
> command as ovn-nbctl --no-leader-only 
> 
> Signed-off-by: Aliasgar Ginwala 

I think that this could be factored out into lib/command-line.c rather
than done as a copy-and-paste into two files.

I don't think that the ops_passed variable needs to be static.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Documentation: Convert multiple manpages to ReST.

[Ben Pfaff]

On Thu, Oct 10, 2019 at 07:58:02PM -0400, 0-day Robot wrote:
> WARNING: New doc ovs-appctl.8.rst not listed in Documentation/automake.mk
> WARNING: New doc ovs-ctl.8.rst not listed in Documentation/automake.mk
> WARNING: New doc ovs-l3ping.8.rst not listed in Documentation/automake.mk
> WARNING: New doc ovs-parse-backtrace.8.rst not listed in 
> Documentation/automake.mk
> WARNING: New doc ovs-pki.8.rst not listed in Documentation/automake.mk
> WARNING: New doc ovs-tcpdump.8.rst not listed in Documentation/automake.mk
> WARNING: New doc ovs-tcpundump.1.rst not listed in Documentation/automake.mk

I think this is a false positive because checkpatch doesn't understand
that DOC_SOURCE includes $(RST_MANPAGES) via macro expansion.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovs-vlan-bug-workaround: Remove.

[Gregory Rose]

On 10/10/2019 12:07 PM, Ben Pfaff wrote:

This workaround only applied to kernels earlier than 2.6.37, but OVS
only supports 3.10 and later.

As the original author of this code, I won't miss it.

Signed-off-by: Ben Pfaff 


Nice.

I applied the patch and compile tested it.  I then ran 'make check' and 
'make check-kmod' and found no regression
or other issues.  Reviewing the code it looks fine to me - reviewing 
patches that remove stuff is easier.


Thanks Ben, always a plus to get rid of old gunk.

Tested-by: Greg Rose 
Reviewed-by: Greg Rose 


---
  Documentation/faq/vlan.rst|  40 -
  Documentation/ref/index.rst   |   4 -
  acinclude.m4  |   2 -
  manpages.mk   |  10 --
  rhel/openvswitch-fedora.spec.in   |   4 +-
  rhel/openvswitch.spec.in  |   4 +-
  tests/interface-reconfigure.at|   9 +-
  utilities/.gitignore  |   2 -
  utilities/automake.mk |   8 -
  utilities/ovs-vlan-bug-workaround.8.in|  86 ---
  utilities/ovs-vlan-bug-workaround.c   | 145 --
  utilities/ovs-vlan-bugs.man   |  22 ---
  xenserver/openvswitch-xen.spec.in |   2 -
  ...rce_libexec_InterfaceReconfigureVswitch.py |   6 -
  14 files changed, 3 insertions(+), 341 deletions(-)
  delete mode 100644 utilities/ovs-vlan-bug-workaround.8.in
  delete mode 100644 utilities/ovs-vlan-bug-workaround.c
  delete mode 100644 utilities/ovs-vlan-bugs.man

diff --git a/Documentation/faq/vlan.rst b/Documentation/faq/vlan.rst
index 80e6660f5e77..d64db7a0d321 100644
--- a/Documentation/faq/vlan.rst
+++ b/Documentation/faq/vlan.rst
@@ -77,46 +77,6 @@ Q: What's a VLAN?
  
  Q: VLANs don't work.
  
-A: Many drivers in Linux kernels before version 3.3 had VLAN-related bugs.

-If you are having problems with VLANs that you suspect to be driver
-related, then you have several options:
-
-- Upgrade to Linux 3.3 or later.
-
-- Build and install a fixed version of the particular driver that is
-  causing trouble, if one is available.
-
-- Use a NIC whose driver does not have VLAN problems.
-
-- Use "VLAN splinters", a feature in Open vSwitch 1.4 upto 2.5 that works
-  around bugs in kernel drivers.  To enable VLAN splinters on interface
-  eth0, use the command::
-
-  $ ovs-vsctl set interface eth0 
other-config:enable-vlan-splinters=true
-
-  For VLAN splinters to be effective, Open vSwitch must know which VLANs
-  are in use.  See the "VLAN splinters" section in the Interface table in
-  ovs-vswitchd.conf.db(5) for details on how Open vSwitch infers in-use
-  VLANs.
-
-  VLAN splinters increase memory use and reduce performance, so use them
-  only if needed.
-
-- Apply the "vlan workaround" patch from the XenServer kernel patch queue,
-  build Open vSwitch against this patched kernel, and then use
-  ovs-vlan-bug-workaround(8) to enable the VLAN workaround for each
-  interface whose driver is buggy.
-
-  (This is a nontrivial exercise, so this option is included only for
-  completeness.)
-
-It is not always easy to tell whether a Linux kernel driver has buggy VLAN
-support.  The ovs-vlan-test(8) and ovs-test(8) utilities can help you test.
-See their manpages for details.  Of the two utilities, ovs-test(8) is newer
-and more thorough, but ovs-vlan-test(8) may be easier to use.
-
-Q: VLANs still don't work.  I've tested the driver so I know that it's OK.
-
  A: Do you have VLANs enabled on the physical switch that OVS is attached
  to?  Make sure that the port is configured to trunk the VLAN or VLANs that
  you are using with OVS.
diff --git a/Documentation/ref/index.rst b/Documentation/ref/index.rst
index 0cb5ef571676..83f031c4f646 100644
--- a/Documentation/ref/index.rst
+++ b/Documentation/ref/index.rst
@@ -126,10 +126,6 @@ The remainder are still in roff format can be found below:
   - `(pdf) 
`__
   - `(html) 
`__
   - `(plain text) 
`__
-   * - ovs-vlan-bug-workaround(8)
- - `(pdf) 
`__
- - `(html) 
`__
- - `(plain text) 
`__
 * - ovs-vlan-test(8)
   - `(pdf) 
`__
   - `(html) 
`__
diff --git a/acinclude.m4 b/acinclude.m4
index 52f92870eaaa..b3e5f94690c8 100644
--- 

Re: [ovs-dev] [PATCH 2/2] datapath: Make metadata_dst tunnel work in IP_TUNNEL_INFO_BRIDGE mode

[Gregory Rose]

On 10/8/2019 4:51 PM, Greg Rose wrote:

From: wenxu 

Upstream commit:
 commit 18b6f717483a835fb98de9f0df6c724df9324e78
 Author: wenxu 
 Date:   Thu Mar 28 12:43:23 2019 +0800

 openvswitch: Make metadata_dst tunnel work in IP_TUNNEL_INFO_BRIDGE mode

 There is currently no support for the multicast/broadcast aspects
 of VXLAN in ovs. In the datapath flow the tun_dst must specific.
 But in the IP_TUNNEL_INFO_BRIDGE mode the tun_dst can not be specific.
 And the packet can forward through the fdb table of vxlan devcice. In
 this mode the broadcast/multicast packet can be sent through the
 following ways in ovs.

 ovs-vsctl add-port br0 vxlan -- set in vxlan type=vxlan \
 options:key=1000 options:remote_ip=flow
 ovs-ofctl add-flow br0 in_port=LOCAL,dl_dst=ff:ff:ff:ff:ff:ff, \
 action=output:vxlan

 bridge fdb append ff:ff:ff:ff:ff:ff dev vxlan_sys_4789 dst 172.168.0.1 \
 src_vni 1000 vni 1000 self
 bridge fdb append ff:ff:ff:ff:ff:ff dev vxlan_sys_4789 dst 172.168.0.2 \
 src_vni 1000 vni 1000 self

 Signed-off-by: wenxu 
 Acked-by: Pravin B Shelar 
 Signed-off-by: David S. Miller 

Also fixup case statement in lib/odp-util.c to make the compiler happy.

Cc: wenxu 
Signed-off-by: Greg Rose 


Hi Wenxu,

would you mind giving this patch a review - I just want to make sure I 
didn't miss anything.


Thanks,

- Greg


---
  datapath/flow_netlink.c   | 46 ++-
  datapath/linux/compat/include/linux/openvswitch.h |  1 +
  lib/odp-util.c|  2 +
  3 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 0f7ab53..b3d1069 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -406,6 +406,7 @@ static const struct ovs_len_tbl 
ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1]
[OVS_TUNNEL_KEY_ATTR_IPV6_SRC]  = { .len = sizeof(struct in6_addr) 
},
[OVS_TUNNEL_KEY_ATTR_IPV6_DST]  = { .len = sizeof(struct in6_addr) 
},
[OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS]   = { .len = OVS_ATTR_VARIABLE },
+   [OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE]   = { .len = 0 },
  };
  
  static const struct ovs_len_tbl

@@ -669,6 +670,7 @@ static int ip_tun_from_nlattr(const struct nlattr *attr,
  bool log)
  {
bool ttl = false, ipv4 = false, ipv6 = false;
+   bool info_bridge_mode = false;
__be16 tun_flags = 0;
int opts_type = 0;
struct nlattr *a;
@@ -785,6 +787,10 @@ static int ip_tun_from_nlattr(const struct nlattr *attr,
tun_flags |= TUNNEL_ERSPAN_OPT;
opts_type = type;
break;
+   case OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE:
+   info_bridge_mode = true;
+   ipv4 = true;
+   break;
default:
OVS_NLERR(log, "Unknown IP tunnel attribute %d",
  type);
@@ -815,16 +821,29 @@ static int ip_tun_from_nlattr(const struct nlattr *attr,
OVS_NLERR(log, "IP tunnel dst address not specified");
return -EINVAL;
}
-   if (ipv4 && !match->key->tun_key.u.ipv4.dst) {
-   OVS_NLERR(log, "IPv4 tunnel dst address is zero");
-   return -EINVAL;
+   if (ipv4) {
+   if (info_bridge_mode) {
+   if (match->key->tun_key.u.ipv4.src ||
+   match->key->tun_key.u.ipv4.dst ||
+   match->key->tun_key.tp_src ||
+   match->key->tun_key.tp_dst ||
+   match->key->tun_key.ttl ||
+   match->key->tun_key.tos ||
+   tun_flags & ~TUNNEL_KEY) {
+   OVS_NLERR(log, "IPv4 tun info is not 
correct");
+   return -EINVAL;
+   }
+   } else if (!match->key->tun_key.u.ipv4.dst) {
+   OVS_NLERR(log, "IPv4 tunnel dst address is 
zero");
+   return -EINVAL;
+   }
}
if (ipv6 && ipv6_addr_any(>key->tun_key.u.ipv6.dst)) {
OVS_NLERR(log, "IPv6 tunnel dst address is zero");
return -EINVAL;
}
  
-		if (!ttl) {

+   if (!ttl && !info_bridge_mode) {
OVS_NLERR(log, "IP tunnel TTL not specified.");
return -EINVAL;
}
@@ -853,12 +872,17 @@ static int vxlan_opt_to_nlattr(struct sk_buff *skb,
  static int 

Re: [ovs-dev] [PATCH] dpif-netlink: Fix some variable naming.

[Ben Pfaff]

Thank you for the review.  I applied this to master.

On Mon, Oct 14, 2019 at 12:16:59PM -0700, Yifeng Sun wrote:
> LGTM, thanks.
> 
> Reviewed-by: Yifeng Sun 
> 
> On Mon, Oct 14, 2019 at 11:28 AM Ben Pfaff  wrote:
> >
> > Usually a plural name refers to an array, but 'socks' and 'socksp' were
> > only single objects, so this changes their names to 'sock' and 'sockp'.
> >
> > Usually a 'p' suffix means that a variable is an output argument, but
> > that was only true in one place here, so this changes the names of the
> > other variables to plain 'sock'.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  lib/dpif-netlink.c | 48 +++---
> >  1 file changed, 24 insertions(+), 24 deletions(-)
> >
> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > index ebe22106e0fc..d1f9b81db84f 100644
> > --- a/lib/dpif-netlink.c
> > +++ b/lib/dpif-netlink.c
> > @@ -249,11 +249,11 @@ static int dpif_netlink_port_query__(const struct 
> > dpif_netlink *dpif,
> >   struct dpif_port *dpif_port);
> >
> >  static int
> > -create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, struct nl_sock 
> > **socksp)
> > +create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, struct nl_sock 
> > **sockp)
> >  OVS_REQ_WRLOCK(dpif->upcall_lock)
> >  {
> >  #ifndef _WIN32
> > -return nl_sock_create(NETLINK_GENERIC, socksp);
> > +return nl_sock_create(NETLINK_GENERIC, sockp);
> >  #else
> >  /* Pick netlink sockets to use in a round-robin fashion from each
> >   * handler's pool of sockets. */
> > @@ -263,13 +263,13 @@ create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, 
> > struct nl_sock **socksp)
> >
> >  /* A pool of sockets is allocated when the handler is initialized. */
> >  if (sock_pool == NULL) {
> > -*socksp = NULL;
> > +*sockp = NULL;
> >  return EINVAL;
> >  }
> >
> >  ovs_assert(index < VPORT_SOCK_POOL_SIZE);
> > -*socksp = sock_pool[index].nl_sock;
> > -ovs_assert(*socksp);
> > +*sockp = sock_pool[index].nl_sock;
> > +ovs_assert(*sockp);
> >  index = (index == VPORT_SOCK_POOL_SIZE - 1) ? 0 : index + 1;
> >  handler->last_used_pool_idx = index;
> >  return 0;
> > @@ -277,10 +277,10 @@ create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, 
> > struct nl_sock **socksp)
> >  }
> >
> >  static void
> > -close_nl_sock(struct nl_sock *socksp)
> > +close_nl_sock(struct nl_sock *sock)
> >  {
> >  #ifndef _WIN32
> > -nl_sock_destroy(socksp);
> > +nl_sock_destroy(sock);
> >  #endif
> >  }
> >
> > @@ -450,7 +450,7 @@ vport_get_pid(struct dpif_netlink *dpif, uint32_t 
> > port_idx,
> >
> >  static int
> >  vport_add_channel(struct dpif_netlink *dpif, odp_port_t port_no,
> > -  struct nl_sock *socksp)
> > +  struct nl_sock *sock)
> >  {
> >  struct epoll_event event;
> >  uint32_t port_idx = odp_to_u32(port_no);
> > @@ -458,7 +458,7 @@ vport_add_channel(struct dpif_netlink *dpif, odp_port_t 
> > port_no,
> >  int error;
> >
> >  if (dpif->handlers == NULL) {
> > -close_nl_sock(socksp);
> > +close_nl_sock(sock);
> >  return 0;
> >  }
> >
> > @@ -499,14 +499,14 @@ vport_add_channel(struct dpif_netlink *dpif, 
> > odp_port_t port_no,
> >  struct dpif_handler *handler = >handlers[i];
> >
> >  #ifndef _WIN32
> > -if (epoll_ctl(handler->epoll_fd, EPOLL_CTL_ADD, nl_sock_fd(socksp),
> > +if (epoll_ctl(handler->epoll_fd, EPOLL_CTL_ADD, nl_sock_fd(sock),
> >) < 0) {
> >  error = errno;
> >  goto error;
> >  }
> >  #endif
> >  }
> > -dpif->channels[port_idx].sock = socksp;
> > +dpif->channels[port_idx].sock = sock;
> >  dpif->channels[port_idx].last_poll = LLONG_MIN;
> >
> >  return 0;
> > @@ -515,7 +515,7 @@ error:
> >  #ifndef _WIN32
> >  while (i--) {
> >  epoll_ctl(dpif->handlers[i].epoll_fd, EPOLL_CTL_DEL,
> > -  nl_sock_fd(socksp), NULL);
> > +  nl_sock_fd(sock), NULL);
> >  }
> >  #endif
> >  dpif->channels[port_idx].sock = NULL;
> > @@ -750,12 +750,12 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, 
> > const char *name,
> >  {
> >  struct dpif_netlink_vport request, reply;
> >  struct ofpbuf *buf;
> > -struct nl_sock *socksp = NULL;
> > +struct nl_sock *sock = NULL;
> >  uint32_t upcall_pids = 0;
> >  int error = 0;
> >
> >  if (dpif->handlers) {
> > -error = create_nl_sock(dpif, );
> > +error = create_nl_sock(dpif, );
> >  if (error) {
> >  return error;
> >  }
> > @@ -768,8 +768,8 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, 
> > const char *name,
> >  request.name = name;
> >
> >  request.port_no = *port_nop;
> > -if (socksp) {
> > -upcall_pids = nl_sock_pid(socksp);
> > +if (sock) {
> > +upcall_pids = nl_sock_pid(sock);
> >  }

Re: [ovs-dev] [PATCH v3] ovsdb-server: Don't drop all connections on read/write status change.

[Ben Pfaff]

On Tue, Oct 15, 2019 at 12:42:04AM +0530, Numan Siddique wrote:
> On Mon, Oct 14, 2019 at 11:45 PM Numan Siddique  wrote:
> 
> >
> >
> > On Mon, Oct 14, 2019 at 11:37 PM Ben Pfaff  wrote:
> >
> >> On Mon, Oct 14, 2019 at 08:50:02PM +0530, nusid...@redhat.com wrote:
> >> > From: Numan Siddique 
> >> >
> >> > The commit [1] force drops all connections when the db read/write
> >> status changes.
> >> > Prior to the commit [1], when there was read/write status change, the
> >> existing
> >> > jsonrpc sessions with 'db_change_aware' set to true, were not updated
> >> with the
> >> > changed 'read_only' value. If the db status was changed to 'standby',
> >> the existing
> >> > clients could still write to the db.
> >>
> >> Thanks, applied to master.
> >>
> >
> > Thanks for applying the patch.
> >
> 
> Can this patch be back ported to 2.12 please.

Done.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH V1] Add offload packets statistics

[Ben Pfaff]

On Fri, Oct 11, 2019 at 11:46:29AM +0800, zhaozhanxu wrote:
> Add argument '-m' for command ovs-appctl bridge/dump-flows
> to display the offloaded packets statistics.

This seems like a reasonable addition, and the code looks OK too.

Before, with 2 pointers as function parameters, it was OK to have them
individually.  I am less comfortable with 4 pointers as function
parameters.  Did you consider defining a structure?  It might make the
code better.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netlink: Fix some variable naming.

[Yifeng Sun]

LGTM, thanks.

Reviewed-by: Yifeng Sun 

On Mon, Oct 14, 2019 at 11:28 AM Ben Pfaff  wrote:
>
> Usually a plural name refers to an array, but 'socks' and 'socksp' were
> only single objects, so this changes their names to 'sock' and 'sockp'.
>
> Usually a 'p' suffix means that a variable is an output argument, but
> that was only true in one place here, so this changes the names of the
> other variables to plain 'sock'.
>
> Signed-off-by: Ben Pfaff 
> ---
>  lib/dpif-netlink.c | 48 +++---
>  1 file changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index ebe22106e0fc..d1f9b81db84f 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -249,11 +249,11 @@ static int dpif_netlink_port_query__(const struct 
> dpif_netlink *dpif,
>   struct dpif_port *dpif_port);
>
>  static int
> -create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, struct nl_sock **socksp)
> +create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, struct nl_sock **sockp)
>  OVS_REQ_WRLOCK(dpif->upcall_lock)
>  {
>  #ifndef _WIN32
> -return nl_sock_create(NETLINK_GENERIC, socksp);
> +return nl_sock_create(NETLINK_GENERIC, sockp);
>  #else
>  /* Pick netlink sockets to use in a round-robin fashion from each
>   * handler's pool of sockets. */
> @@ -263,13 +263,13 @@ create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, 
> struct nl_sock **socksp)
>
>  /* A pool of sockets is allocated when the handler is initialized. */
>  if (sock_pool == NULL) {
> -*socksp = NULL;
> +*sockp = NULL;
>  return EINVAL;
>  }
>
>  ovs_assert(index < VPORT_SOCK_POOL_SIZE);
> -*socksp = sock_pool[index].nl_sock;
> -ovs_assert(*socksp);
> +*sockp = sock_pool[index].nl_sock;
> +ovs_assert(*sockp);
>  index = (index == VPORT_SOCK_POOL_SIZE - 1) ? 0 : index + 1;
>  handler->last_used_pool_idx = index;
>  return 0;
> @@ -277,10 +277,10 @@ create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, 
> struct nl_sock **socksp)
>  }
>
>  static void
> -close_nl_sock(struct nl_sock *socksp)
> +close_nl_sock(struct nl_sock *sock)
>  {
>  #ifndef _WIN32
> -nl_sock_destroy(socksp);
> +nl_sock_destroy(sock);
>  #endif
>  }
>
> @@ -450,7 +450,7 @@ vport_get_pid(struct dpif_netlink *dpif, uint32_t 
> port_idx,
>
>  static int
>  vport_add_channel(struct dpif_netlink *dpif, odp_port_t port_no,
> -  struct nl_sock *socksp)
> +  struct nl_sock *sock)
>  {
>  struct epoll_event event;
>  uint32_t port_idx = odp_to_u32(port_no);
> @@ -458,7 +458,7 @@ vport_add_channel(struct dpif_netlink *dpif, odp_port_t 
> port_no,
>  int error;
>
>  if (dpif->handlers == NULL) {
> -close_nl_sock(socksp);
> +close_nl_sock(sock);
>  return 0;
>  }
>
> @@ -499,14 +499,14 @@ vport_add_channel(struct dpif_netlink *dpif, odp_port_t 
> port_no,
>  struct dpif_handler *handler = >handlers[i];
>
>  #ifndef _WIN32
> -if (epoll_ctl(handler->epoll_fd, EPOLL_CTL_ADD, nl_sock_fd(socksp),
> +if (epoll_ctl(handler->epoll_fd, EPOLL_CTL_ADD, nl_sock_fd(sock),
>) < 0) {
>  error = errno;
>  goto error;
>  }
>  #endif
>  }
> -dpif->channels[port_idx].sock = socksp;
> +dpif->channels[port_idx].sock = sock;
>  dpif->channels[port_idx].last_poll = LLONG_MIN;
>
>  return 0;
> @@ -515,7 +515,7 @@ error:
>  #ifndef _WIN32
>  while (i--) {
>  epoll_ctl(dpif->handlers[i].epoll_fd, EPOLL_CTL_DEL,
> -  nl_sock_fd(socksp), NULL);
> +  nl_sock_fd(sock), NULL);
>  }
>  #endif
>  dpif->channels[port_idx].sock = NULL;
> @@ -750,12 +750,12 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, 
> const char *name,
>  {
>  struct dpif_netlink_vport request, reply;
>  struct ofpbuf *buf;
> -struct nl_sock *socksp = NULL;
> +struct nl_sock *sock = NULL;
>  uint32_t upcall_pids = 0;
>  int error = 0;
>
>  if (dpif->handlers) {
> -error = create_nl_sock(dpif, );
> +error = create_nl_sock(dpif, );
>  if (error) {
>  return error;
>  }
> @@ -768,8 +768,8 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, const 
> char *name,
>  request.name = name;
>
>  request.port_no = *port_nop;
> -if (socksp) {
> -upcall_pids = nl_sock_pid(socksp);
> +if (sock) {
> +upcall_pids = nl_sock_pid(sock);
>  }
>  request.n_upcall_pids = 1;
>  request.upcall_pids = _pids;
> @@ -788,11 +788,11 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, 
> const char *name,
>dpif_name(>dpif), *port_nop);
>  }
>
> -close_nl_sock(socksp);
> +close_nl_sock(sock);
>  goto exit;
>  }
>
> -error = vport_add_channel(dpif, *port_nop, socksp);
> +error 

Re: [ovs-dev] [PATCH v3] ovsdb-server: Don't drop all connections on read/write status change.

[Numan Siddique]

On Mon, Oct 14, 2019 at 11:45 PM Numan Siddique  wrote:

>
>
> On Mon, Oct 14, 2019 at 11:37 PM Ben Pfaff  wrote:
>
>> On Mon, Oct 14, 2019 at 08:50:02PM +0530, nusid...@redhat.com wrote:
>> > From: Numan Siddique 
>> >
>> > The commit [1] force drops all connections when the db read/write
>> status changes.
>> > Prior to the commit [1], when there was read/write status change, the
>> existing
>> > jsonrpc sessions with 'db_change_aware' set to true, were not updated
>> with the
>> > changed 'read_only' value. If the db status was changed to 'standby',
>> the existing
>> > clients could still write to the db.
>>
>> Thanks, applied to master.
>>
>
> Thanks for applying the patch.
>

Can this patch be back ported to 2.12 please.

Thanks

Numan


>
> Numan
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] dpif-netlink: Fix some variable naming.

[Ben Pfaff]

Usually a plural name refers to an array, but 'socks' and 'socksp' were
only single objects, so this changes their names to 'sock' and 'sockp'.

Usually a 'p' suffix means that a variable is an output argument, but
that was only true in one place here, so this changes the names of the
other variables to plain 'sock'.

Signed-off-by: Ben Pfaff 
---
 lib/dpif-netlink.c | 48 +++---
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index ebe22106e0fc..d1f9b81db84f 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -249,11 +249,11 @@ static int dpif_netlink_port_query__(const struct 
dpif_netlink *dpif,
  struct dpif_port *dpif_port);
 
 static int
-create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, struct nl_sock **socksp)
+create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, struct nl_sock **sockp)
 OVS_REQ_WRLOCK(dpif->upcall_lock)
 {
 #ifndef _WIN32
-return nl_sock_create(NETLINK_GENERIC, socksp);
+return nl_sock_create(NETLINK_GENERIC, sockp);
 #else
 /* Pick netlink sockets to use in a round-robin fashion from each
  * handler's pool of sockets. */
@@ -263,13 +263,13 @@ create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, 
struct nl_sock **socksp)
 
 /* A pool of sockets is allocated when the handler is initialized. */
 if (sock_pool == NULL) {
-*socksp = NULL;
+*sockp = NULL;
 return EINVAL;
 }
 
 ovs_assert(index < VPORT_SOCK_POOL_SIZE);
-*socksp = sock_pool[index].nl_sock;
-ovs_assert(*socksp);
+*sockp = sock_pool[index].nl_sock;
+ovs_assert(*sockp);
 index = (index == VPORT_SOCK_POOL_SIZE - 1) ? 0 : index + 1;
 handler->last_used_pool_idx = index;
 return 0;
@@ -277,10 +277,10 @@ create_nl_sock(struct dpif_netlink *dpif OVS_UNUSED, 
struct nl_sock **socksp)
 }
 
 static void
-close_nl_sock(struct nl_sock *socksp)
+close_nl_sock(struct nl_sock *sock)
 {
 #ifndef _WIN32
-nl_sock_destroy(socksp);
+nl_sock_destroy(sock);
 #endif
 }
 
@@ -450,7 +450,7 @@ vport_get_pid(struct dpif_netlink *dpif, uint32_t port_idx,
 
 static int
 vport_add_channel(struct dpif_netlink *dpif, odp_port_t port_no,
-  struct nl_sock *socksp)
+  struct nl_sock *sock)
 {
 struct epoll_event event;
 uint32_t port_idx = odp_to_u32(port_no);
@@ -458,7 +458,7 @@ vport_add_channel(struct dpif_netlink *dpif, odp_port_t 
port_no,
 int error;
 
 if (dpif->handlers == NULL) {
-close_nl_sock(socksp);
+close_nl_sock(sock);
 return 0;
 }
 
@@ -499,14 +499,14 @@ vport_add_channel(struct dpif_netlink *dpif, odp_port_t 
port_no,
 struct dpif_handler *handler = >handlers[i];
 
 #ifndef _WIN32
-if (epoll_ctl(handler->epoll_fd, EPOLL_CTL_ADD, nl_sock_fd(socksp),
+if (epoll_ctl(handler->epoll_fd, EPOLL_CTL_ADD, nl_sock_fd(sock),
   ) < 0) {
 error = errno;
 goto error;
 }
 #endif
 }
-dpif->channels[port_idx].sock = socksp;
+dpif->channels[port_idx].sock = sock;
 dpif->channels[port_idx].last_poll = LLONG_MIN;
 
 return 0;
@@ -515,7 +515,7 @@ error:
 #ifndef _WIN32
 while (i--) {
 epoll_ctl(dpif->handlers[i].epoll_fd, EPOLL_CTL_DEL,
-  nl_sock_fd(socksp), NULL);
+  nl_sock_fd(sock), NULL);
 }
 #endif
 dpif->channels[port_idx].sock = NULL;
@@ -750,12 +750,12 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, const 
char *name,
 {
 struct dpif_netlink_vport request, reply;
 struct ofpbuf *buf;
-struct nl_sock *socksp = NULL;
+struct nl_sock *sock = NULL;
 uint32_t upcall_pids = 0;
 int error = 0;
 
 if (dpif->handlers) {
-error = create_nl_sock(dpif, );
+error = create_nl_sock(dpif, );
 if (error) {
 return error;
 }
@@ -768,8 +768,8 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, const 
char *name,
 request.name = name;
 
 request.port_no = *port_nop;
-if (socksp) {
-upcall_pids = nl_sock_pid(socksp);
+if (sock) {
+upcall_pids = nl_sock_pid(sock);
 }
 request.n_upcall_pids = 1;
 request.upcall_pids = _pids;
@@ -788,11 +788,11 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, const 
char *name,
   dpif_name(>dpif), *port_nop);
 }
 
-close_nl_sock(socksp);
+close_nl_sock(sock);
 goto exit;
 }
 
-error = vport_add_channel(dpif, *port_nop, socksp);
+error = vport_add_channel(dpif, *port_nop, sock);
 if (error) {
 VLOG_INFO("%s: could not add channel for port %s",
 dpif_name(>dpif), name);
@@ -803,7 +803,7 @@ dpif_netlink_port_add__(struct dpif_netlink *dpif, const 
char *name,
 request.dp_ifindex = dpif->dp_ifindex;
 request.port_no = *port_nop;
 

Re: [ovs-dev] [PATCH] dpif-netlink: Free leaked nl_sock

[Ben Pfaff]

On Fri, Oct 11, 2019 at 03:50:47PM -0700, Yifeng Sun wrote:
> Valgrind reports:
> 20 bytes in 1 blocks are definitely lost in loss record 94 of 353
> by 0x532594: xmalloc (util.c:138)
> by 0x553EAD: nl_sock_create (netlink-socket.c:146)
> by 0x54331D: create_nl_sock (dpif-netlink.c:255)
> by 0x54331D: dpif_netlink_port_add__ (dpif-netlink.c:756)
> by 0x5435F6: dpif_netlink_port_add_compat (dpif-netlink.c:876)
> by 0x5435F6: dpif_netlink_port_add (dpif-netlink.c:922)
> by 0x47EC1D: dpif_port_add (dpif.c:584)
> by 0x42B35F: port_add (ofproto-dpif.c:3721)
> by 0x41E64A: ofproto_port_add (ofproto.c:2032)
> by 0x40B3FE: iface_do_create (bridge.c:1817)
> by 0x40B3FE: iface_create (bridge.c:1855)
> by 0x40B3FE: bridge_add_ports__ (bridge.c:943)
> by 0x40D14A: bridge_add_ports (bridge.c:959)
> by 0x40D14A: bridge_reconfigure (bridge.c:673)
> by 0x410D75: bridge_run (bridge.c:3050)
> by 0x407614: main (ovs-vswitchd.c:127)
> 
> This leak is because when vport_add_channel() returns 0, it is expected
> to take the ownership of 'socksp'. This patch fixes this issue.
> 
> Signed-off-by: Yifeng Sun 

Thank you.  I applied this to master and backported it as far as it
would go.

There's some bad naming here.  I'll follow up with improvements.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] ovsdb-server: Don't drop all connections on read/write status change.

[Numan Siddique]

On Mon, Oct 14, 2019 at 11:37 PM Ben Pfaff  wrote:

> On Mon, Oct 14, 2019 at 08:50:02PM +0530, nusid...@redhat.com wrote:
> > From: Numan Siddique 
> >
> > The commit [1] force drops all connections when the db read/write status
> changes.
> > Prior to the commit [1], when there was read/write status change, the
> existing
> > jsonrpc sessions with 'db_change_aware' set to true, were not updated
> with the
> > changed 'read_only' value. If the db status was changed to 'standby',
> the existing
> > clients could still write to the db.
>
> Thanks, applied to master.
>

Thanks for applying the patch.

Numan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] ovsdb-server: Don't drop all connections on read/write status change.

[Numan Siddique]

On Mon, Oct 14, 2019, 11:42 PM Han Zhou  wrote:

>
>
> On Mon, Oct 14, 2019 at 8:20 AM  wrote:
>
>> From: Numan Siddique 
>>
>> The commit [1] force drops all connections when the db read/write status
>> changes.
>> Prior to the commit [1], when there was read/write status change, the
>> existing
>> jsonrpc sessions with 'db_change_aware' set to true, were not updated
>> with the
>> changed 'read_only' value. If the db status was changed to 'standby', the
>> existing
>> clients could still write to the db.
>>
>> In the case of pacemaker OVN HA, OVN OCF script 'start' action starts the
>> ovsdb-servers in read-only state and later, it sets to read-write in the
>> 'promote' action. We have observed that if some ovn-controllers connect to
>> the SB ovsdb-server (in read-only state) just before the 'promote' action,
>> the connection is not reset all the times and these ovn-controllers
>> remain connected
>> to the SB ovsdb-server in read-only state all the time. Even though
>> the commit [1] calls 'ovsdb_jsonrpc_server_reconnect()' with 'forced' flag
>> set to true when the db read/write status changes, somehow the FSM misses
>> resetting
>> the connections of these ovn-controllers.
>>
>> I think this needs to be addressed in the FSM. This patch doesn't address
>> this FSM issue. Instead it changes the behavior of
>> 'ovsdb_jsonrpc_server_set_read_only()'
>> by setting the 'read_only' flag of all the jsonrpc sessions instead of
>> forcefully
>> resetting the connection.
>>
>> I think there is no need to reset the connection. In large scale
>> production
>> deployements with OVN, this results in unnecessary waste of CPU cycles as
>> ovn-controllers
>> will have to connect twice - once during 'start' action and again during
>> 'promote'.
>>
>> [1] - 2a9679e3b2c6("ovsdb-server: drop all connections on read/write
>> status change")
>>
>> Acked-by: Dumitru Ceara 
>> Signed-off-by: Numan Siddique 
>> ---
>>
>>
> Thanks Numan. Is this the root cause of the ovn-controller transaction
> failure you mentioned at last OVN meeting?
>

Hi Han,
Yes. This is the root cause. Earlier I was suspecting ovn-controller though.

Thanks
Numan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] ovsdb-server: Don't drop all connections on read/write status change.

[Han Zhou]

On Mon, Oct 14, 2019 at 8:20 AM  wrote:

> From: Numan Siddique 
>
> The commit [1] force drops all connections when the db read/write status
> changes.
> Prior to the commit [1], when there was read/write status change, the
> existing
> jsonrpc sessions with 'db_change_aware' set to true, were not updated with
> the
> changed 'read_only' value. If the db status was changed to 'standby', the
> existing
> clients could still write to the db.
>
> In the case of pacemaker OVN HA, OVN OCF script 'start' action starts the
> ovsdb-servers in read-only state and later, it sets to read-write in the
> 'promote' action. We have observed that if some ovn-controllers connect to
> the SB ovsdb-server (in read-only state) just before the 'promote' action,
> the connection is not reset all the times and these ovn-controllers remain
> connected
> to the SB ovsdb-server in read-only state all the time. Even though
> the commit [1] calls 'ovsdb_jsonrpc_server_reconnect()' with 'forced' flag
> set to true when the db read/write status changes, somehow the FSM misses
> resetting
> the connections of these ovn-controllers.
>
> I think this needs to be addressed in the FSM. This patch doesn't address
> this FSM issue. Instead it changes the behavior of
> 'ovsdb_jsonrpc_server_set_read_only()'
> by setting the 'read_only' flag of all the jsonrpc sessions instead of
> forcefully
> resetting the connection.
>
> I think there is no need to reset the connection. In large scale production
> deployements with OVN, this results in unnecessary waste of CPU cycles as
> ovn-controllers
> will have to connect twice - once during 'start' action and again during
> 'promote'.
>
> [1] - 2a9679e3b2c6("ovsdb-server: drop all connections on read/write
> status change")
>
> Acked-by: Dumitru Ceara 
> Signed-off-by: Numan Siddique 
> ---
>
>
Thanks Numan. Is this the root cause of the ovn-controller transaction
failure you mentioned at last OVN meeting?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] ovsdb-server: Don't drop all connections on read/write status change.

[Ben Pfaff]

On Mon, Oct 14, 2019 at 08:50:02PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique 
> 
> The commit [1] force drops all connections when the db read/write status 
> changes.
> Prior to the commit [1], when there was read/write status change, the existing
> jsonrpc sessions with 'db_change_aware' set to true, were not updated with the
> changed 'read_only' value. If the db status was changed to 'standby', the 
> existing
> clients could still write to the db.

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] OVN: Send RARP for vif ports for which OVN does not know the IP.

[Aaron Conole]

0-day Robot  writes:

> Bleep bloop.  Greetings Ankur Sharma, I am a robot and I have tried out your 
> patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> git-am:
> fatal: sha1 information is lacking or useless (controller/pinctrl.c).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 OVN: Send RARP for vif ports for which OVN does not know 
> the IP.
> The copy of the patch that failed is found in:
>
> /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
> When you have resolved this problem, run "git am --resolved".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
>
> Please check this out.  If you feel there has been an error, please email 
> acon...@redhat.com

One way to avoid this error in the future would be to make your subject
metadata include 'ovn' (ie: [PATCH ovn v3] OVN: Send RARP for vif
ports...)

> Thanks,
> 0-day Robot
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] netdev-dpdk: Track vhost tx contention.

[Aaron Conole]

David Marchand  writes:

> Add a coverage counter to help diagnose contention on the vhost txqs.
> This is seen as dropped packets on the physical ports for rates that
> are usually handled fine by OVS.
> Document how to further debug this contention with perf.
>
> Signed-off-by: David Marchand 
> ---
> Changelog since v1:
> - added documentation as a bonus: not sure this is the right place, or if it
>   really makes sense to enter into such details. But I still find it useful.
>   Comments?

It's useful, and I think it makes sense here.

> ---
>  Documentation/topics/dpdk/vhost-user.rst | 61 
> 
>  lib/netdev-dpdk.c|  8 -
>  2 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/topics/dpdk/vhost-user.rst 
> b/Documentation/topics/dpdk/vhost-user.rst
> index fab87bd..c7e605e 100644
> --- a/Documentation/topics/dpdk/vhost-user.rst
> +++ b/Documentation/topics/dpdk/vhost-user.rst
> @@ -623,3 +623,64 @@ Because of this limitation, this feature is considered 
> 'experimental'.
>  Further information can be found in the
>  `DPDK documentation
>  `__
> +
> +Troubleshooting vhost-user tx contention
> +
> +
> +Depending on the number of a virtio port Rx queues enabled by a guest and on
> +the number of PMDs used on OVS side, OVS can end up with contention occuring
> +on the lock protecting the vhost Tx queue.

Maybe make the wording specific to a vhostuser port?  I think someone
might make a wrong conclusion if they use the virtio PMD as a dpdk port
instead of using the vhostuser ports.  Not sure *why* someone might do
that, but it's a possibility and this counter won't tick for those
cases.

> +This problem can be hard to catch since it is noticeable as an increased cpu
> +cost for handling the received packets and, usually, as drops in the
> +statistics of the physical port receiving the packets.
> +
> +To identify such a situation, a coverage statistic is available::
> +
> +  $ ovs-appctl coverage/read-counter vhost_tx_contention
> +  59530681
> +
> +If you want to further debug this contention, perf can be used if your OVS
> +daemon had been compiled with debug symbols.
> +
> +First, identify the point in the binary sources where the contention occurs::
> +
> +  $ perf probe -x $(which ovs-vswitchd) -L __netdev_dpdk_vhost_send \
> + |grep -B 3 -A 3 'COVERAGE_INC(vhost_tx_contention)'
> +   }
> +
> +   21  if (unlikely(!rte_spinlock_trylock(>tx_q[qid].tx_lock))) 
> {
> +   22  COVERAGE_INC(vhost_tx_contention);
> +   23  rte_spinlock_lock(>tx_q[qid].tx_lock);
> +   }
> +
> +Then, place a probe at the line where the lock is taken.
> +You can add additional context to catch which port and queue are concerned::
> +
> +  $ perf probe -x $(which ovs-vswitchd) \
> +'vhost_tx_contention=__netdev_dpdk_vhost_send:23 netdev->name:string qid'
> +
> +Finally, gather data and generate a report::
> +
> +  $ perf record -e probe_ovs:vhost_tx_contention -aR sleep 10
> +  [ perf record: Woken up 120 times to write data ]
> +  [ perf record: Captured and wrote 30.151 MB perf.data (356278 samples) ]
> +
> +  $ perf report -F +pid --stdio
> +  # To display the perf.data header info, please use --header/--header-only 
> options.
> +  #
> +  #
> +  # Total Lost Samples: 0
> +  #
> +  # Samples: 356K of event 'probe_ovs:vhost_tx_contention'
> +  # Event count (approx.): 356278
> +  #
> +  # Overhead  Pid:CommandTrace output
> +  #   .  
> +  #
> +  55.57%83332:pmd-c01/id:33  (9e9775) name="vhost0" qid=0
> +  44.43%8:pmd-c15/id:34  (9e9775) name="vhost0" qid=0
> +
> +
> +  #
> +  # (Tip: Treat branches as callchains: perf report --branch-history)
> +  #
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 7f709ff..3525870 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -41,6 +41,7 @@
>  #include 
>  
>  #include "cmap.h"
> +#include "coverage.h"
>  #include "dirs.h"
>  #include "dp-packet.h"
>  #include "dpdk.h"
> @@ -72,6 +73,8 @@ enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
>  VLOG_DEFINE_THIS_MODULE(netdev_dpdk);
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>  
> +COVERAGE_DEFINE(vhost_tx_contention);
> +
>  #define DPDK_PORT_WATCHDOG_INTERVAL 5
>  
>  #define OVS_CACHE_LINE_SIZE CACHE_LINE_SIZE
> @@ -2353,7 +2356,10 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int 
> qid,
>  goto out;
>  }
>  
> -rte_spinlock_lock(>tx_q[qid].tx_lock);
> +if (unlikely(!rte_spinlock_trylock(>tx_q[qid].tx_lock))) {
> +COVERAGE_INC(vhost_tx_contention);
> +rte_spinlock_lock(>tx_q[qid].tx_lock);
> +}
>  
>  cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);
>  /* Check has QoS has been configured for the netdev */

[ovs-dev] [PATCH 11/11] datapath: Allow attaching helper in later commit

[Yi-Hung Wei]

Upstream commit:
commit 248d45f1e1934f7849fbdc35ef1e57151cf063eb
Author: Yi-Hung Wei 
Date:   Fri Oct 4 09:26:44 2019 -0700

openvswitch: Allow attaching helper in later commit

This patch allows to attach conntrack helper to a confirmed conntrack
entry.  Currently, we can only attach alg helper to a conntrack entry
when it is in the unconfirmed state.  This patch enables an use case
that we can firstly commit a conntrack entry after it passed some
initial conditions.  After that the processing pipeline will further
check a couple of packets to determine if the connection belongs to
a particular application, and attach alg helper to the connection
in a later stage.

Signed-off-by: Yi-Hung Wei 
Signed-off-by: David S. Miller 

Signed-off-by: Yi-Hung Wei 
---
 datapath/conntrack.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index f6e9386f4707..838cf63c908f 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -1045,6 +1045,8 @@ static int __ovs_ct_lookup(struct net *net, struct 
sw_flow_key *key,
 
ct = nf_ct_get(skb, );
if (ct) {
+   bool add_helper = false;
+
/* Packets starting a new connection must be NATted before the
 * helper, so that the helper knows about the NAT.  We enforce
 * this by delaying both NAT and helper calls for unconfirmed
@@ -1062,16 +1064,17 @@ static int __ovs_ct_lookup(struct net *net, struct 
sw_flow_key *key,
}
 
/* Userspace may decide to perform a ct lookup without a helper
-* specified followed by a (recirculate and) commit with one.
-* Therefore, for unconfirmed connections which we will commit,
-* we need to attach the helper here.
+* specified followed by a (recirculate and) commit with one,
+* or attach a helper in a later commit.  Therefore, for
+* connections which we will commit, we may need to attach
+* the helper here.
 */
-   if (!nf_ct_is_confirmed(ct) && info->commit &&
-   info->helper && !nfct_help(ct)) {
+   if (info->commit && info->helper && !nfct_help(ct)) {
int err = __nf_ct_try_assign_helper(ct, info->ct,
GFP_ATOMIC);
if (err)
return err;
+   add_helper = true;
 
/* helper installed, add seqadj if NAT is required */
if (info->nat && !nfct_seqadj(ct)) {
@@ -1081,11 +1084,13 @@ static int __ovs_ct_lookup(struct net *net, struct 
sw_flow_key *key,
}
 
/* Call the helper only if:
-* - nf_conntrack_in() was executed above ("!cached") for a
-*   confirmed connection, or
+* - nf_conntrack_in() was executed above ("!cached") or a
+*   helper was just attached ("add_helper") for a confirmed
+*   connection, or
 * - When committing an unconfirmed connection.
 */
-   if ((nf_ct_is_confirmed(ct) ? !cached : info->commit) &&
+   if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
+ info->commit) &&
ovs_ct_helper(skb, info->family) != NF_ACCEPT) {
return -EINVAL;
}
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 10/11] datapath: Fix log message in ovs conntrack

[Yi-Hung Wei]

Upstream commit:
commit 12c6bc38f99bb168b7f16bdb5e855a51a23ee9ec
Author: Yi-Hung Wei 
Date:   Wed Aug 21 17:16:10 2019 -0700

openvswitch: Fix log message in ovs conntrack

Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action")
Signed-off-by: Yi-Hung Wei 
Signed-off-by: David S. Miller 

Signed-off-by: Yi-Hung Wei 
---
 datapath/conntrack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index ba73962b2214..f6e9386f4707 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -1663,7 +1663,7 @@ static int parse_ct(const struct nlattr *attr, struct 
ovs_conntrack_info *info,
case OVS_CT_ATTR_TIMEOUT:
memcpy(info->timeout, nla_data(a), nla_len(a));
if (!memchr(info->timeout, '\0', nla_len(a))) {
-   OVS_NLERR(log, "Invalid conntrack helper");
+   OVS_NLERR(log, "Invalid conntrack timeout");
return -EINVAL;
}
break;
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 09/11] datapath: Replace removed NF_NAT_NEEDED with IS_ENABLED(CONFIG_NF_NAT)

[Yi-Hung Wei]

Backports the following upstream commit with some backward compatibility
change.

commit f319ca6557c10a711facc4dd60197470796d3ec1
Author: Geert Uytterhoeven 
Date:   Wed May 8 08:52:32 2019 +0200

openvswitch: Replace removed NF_NAT_NEEDED with IS_ENABLED(CONFIG_NF_NAT)

Commit 4806e975729f99c7 ("netfilter: replace NF_NAT_NEEDED with
IS_ENABLED(CONFIG_NF_NAT)") removed CONFIG_NF_NAT_NEEDED, but a new user
popped up afterwards.

Fixes: fec9c271b8f1bde1 ("openvswitch: load and reference the NAT helper.")
Signed-off-by: Geert Uytterhoeven 
Acked-by: Florian Westphal 
Acked-by: Flavio Leitner 
Signed-off-by: David S. Miller 

Signed-off-by: Yi-Hung Wei 
---
 datapath/conntrack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 86e7dd24bb9b..ba73962b2214 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -1406,7 +1406,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info 
*info, const char *name,
return -ENOMEM;
}
 
-#ifdef CONFIG_NF_NAT_NEEDED
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
if (info->nat) {
ret = nf_nat_helper_try_module_get(name, info->family,
   key->ip.proto);
@@ -1909,7 +1909,7 @@ void ovs_ct_free_action(const struct nlattr *a)
 static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
 {
if (ct_info->helper) {
-#ifdef CONFIG_NF_NAT_NEEDED
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
if (ct_info->nat)
nf_nat_helper_put(ct_info->helper);
 #endif
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 08/11] datapath: Check for null pointer return from nla_nest_start_noflag

[Yi-Hung Wei]

From: Colin Ian King 

upstream commit:

commit ca96534630e2edfd73121c487c957b17eca3b7d7
Author: Colin Ian King 
Date:   Wed May 1 14:41:58 2019 +0100

openvswitch: check for null pointer return from nla_nest_start_noflag

The call to nla_nest_start_noflag can return null in the unlikely
event that nla_put returns -EMSGSIZE.  Check for this condition to
avoid a null pointer dereference on pointer nla_reply.

Addresses-Coverity: ("Dereference null return value")
Fixes: 11efd5cb04a1 ("openvswitch: Support conntrack zone limit")
Signed-off-by: Colin Ian King 
Acked-by: Yi-Hung Wei 
Signed-off-by: David S. Miller 

Signed-off-by: Yi-Hung Wei 
---
 datapath/conntrack.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 9a7eab655142..86e7dd24bb9b 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -2273,6 +2273,10 @@ static int ovs_ct_limit_cmd_get(struct sk_buff *skb, 
struct genl_info *info)
return PTR_ERR(reply);
 
nla_reply = nla_nest_start_noflag(reply, OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
+   if (!nla_reply) {
+   err = -EMSGSIZE;
+   goto exit_err;
+   }
 
if (a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]) {
err = ovs_ct_limit_get_zone_limit(
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 07/11] datapath: Load and reference the NAT helper.

[Yi-Hung Wei]

This commit backports the following upstream commit, and two functions
in nf_conntrack_helper.h.

Upstream commit:
commit fec9c271b8f1bde1086be5aa415cdb586e0dc800
Author: Flavio Leitner 
Date:   Wed Apr 17 11:46:17 2019 -0300

openvswitch: load and reference the NAT helper.

This improves the original commit 17c357efe5ec ("openvswitch: load
NAT helper") where it unconditionally tries to load the module for
every flow using NAT, so not efficient when loading multiple flows.
It also doesn't hold any references to the NAT module while the
flow is active.

This change fixes those problems. It will try to load the module
only if it's not present. It grabs a reference to the NAT module
and holds it while the flow is active. Finally, an error message
shows up if either actions above fails.

Fixes: 17c357efe5ec ("openvswitch: load NAT helper")
Signed-off-by: Flavio Leitner 
Signed-off-by: Pablo Neira Ayuso 

Signed-off-by: Yi-Hung Wei 
---
 acinclude.m4   |  4 
 datapath/conntrack.c   | 27 +-
 .../include/net/netfilter/nf_conntrack_helper.h| 17 ++
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 055f5387db19..22f92723b00d 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -904,6 +904,10 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_helper.h],
   [nf_conntrack_helper_put],
   [OVS_DEFINE(HAVE_NF_CONNTRACK_HELPER_PUT)])
+  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_helper.h],
+  [nf_nat_helper_try_module_get])
+  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_helper.h],
+  [nf_nat_helper_put])
   
OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h],:space:]]]SKB_GSO_UDP[[[:space:,
   [OVS_DEFINE([HAVE_SKB_GSO_UDP])])
   OVS_GREP_IFELSE([$KSRC/include/net/dst.h],[DST_NOCACHE],
diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 0c0d43bec2e5..9a7eab655142 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -1391,6 +1391,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info 
*info, const char *name,
 {
struct nf_conntrack_helper *helper;
struct nf_conn_help *help;
+   int ret = 0;
 
helper = nf_conntrack_helper_try_module_get(name, info->family,
key->ip.proto);
@@ -1405,13 +1406,22 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info 
*info, const char *name,
return -ENOMEM;
}
 
+#ifdef CONFIG_NF_NAT_NEEDED
+   if (info->nat) {
+   ret = nf_nat_helper_try_module_get(name, info->family,
+  key->ip.proto);
+   if (ret) {
+   nf_conntrack_helper_put(helper);
+   OVS_NLERR(log, "Failed to load \"%s\" NAT helper, 
error: %d",
+ name, ret);
+   return ret;
+   }
+   }
+#endif
+
rcu_assign_pointer(help->helper, helper);
info->helper = helper;
-
-   if (info->nat)
-   request_module("ip_nat_%s", name);
-
-   return 0;
+   return ret;
 }
 
 #if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
@@ -1898,8 +1908,13 @@ void ovs_ct_free_action(const struct nlattr *a)
 
 static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
 {
-   if (ct_info->helper)
+   if (ct_info->helper) {
+#ifdef CONFIG_NF_NAT_NEEDED
+   if (ct_info->nat)
+   nf_nat_helper_put(ct_info->helper);
+#endif
nf_conntrack_helper_put(ct_info->helper);
+   }
if (ct_info->ct) {
if (ct_info->timeout[0])
nf_ct_destroy_timeout(ct_info->ct);
diff --git a/datapath/linux/compat/include/net/netfilter/nf_conntrack_helper.h 
b/datapath/linux/compat/include/net/netfilter/nf_conntrack_helper.h
index b6a3d0bf75b3..78f97375b66e 100644
--- a/datapath/linux/compat/include/net/netfilter/nf_conntrack_helper.h
+++ b/datapath/linux/compat/include/net/netfilter/nf_conntrack_helper.h
@@ -19,4 +19,21 @@ rpl_nf_ct_helper_ext_add(struct nf_conn *ct,
 #define nf_ct_helper_ext_add rpl_nf_ct_helper_ext_add
 #endif /* HAVE_NF_CT_HELPER_EXT_ADD_TAKES_HELPER */
 
+#ifndef HAVE_NF_NAT_HELPER_TRY_MODULE_GET
+static inline int rpl_nf_nat_helper_try_module_get(const char *name, u16 l3num,
+  u8 protonum)
+{
+   request_module("ip_nat_%s", name);
+   return 0;
+}
+#define nf_nat_helper_try_module_get rpl_nf_nat_helper_try_module_get
+#endif /* HAVE_NF_NAT_HELPER_TRY_MODULE_GET */
+
+#ifndef HAVE_NF_NAT_HELPER_PUT
+void rpl_nf_nat_helper_put(struct nf_conntrack_helper *helper)
+{
+}
+#define nf_nat_helper_put rpl_nf_nat_helper_put

[ovs-dev] [PATCH 06/11] datapath: genetlink: optionally validate strictly/dumps

[Yi-Hung Wei]

This patch backports the following upstream commit within the
openvswitch kernel module with some checks so that it also works
in the older kernel.

Upstream commit:
commit ef6243acb4782df587a4d7d6c310fa5b5d82684b
Author: Johannes Berg 
Date:   Fri Apr 26 14:07:31 2019 +0200

genetlink: optionally validate strictly/dumps

Add options to strictly validate messages and dump messages,
sometimes perhaps validating dump messages non-strictly may
be required, so add an option for that as well.

Since none of this can really be applied to existing commands,
set the options everwhere using the following spatch:

@@
identifier ops;
expression X;
@@
struct genl_ops ops[] = {
...,
 {
.cmd = X,
+   .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
...
 },
...
};

For new commands one should just not copy the .validate 'opt-out'
flags and thus get strict validation.

Signed-off-by: Johannes Berg 
Signed-off-by: David S. Miller 

Signed-off-by: Yi-Hung Wei 
---
 acinclude.m4 |  1 +
 datapath/conntrack.c |  9 +
 datapath/datapath.c  | 39 +++
 datapath/meter.c | 12 
 4 files changed, 61 insertions(+)

diff --git a/acinclude.m4 b/acinclude.m4
index fe121ab9126d..055f5387db19 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -817,6 +817,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/net/genetlink.h], [genlmsg_parse])
   OVS_GREP_IFELSE([$KSRC/include/net/genetlink.h], [genl_notify.*family],
   [OVS_DEFINE([HAVE_GENL_NOTIFY_TAKES_FAMILY])])
+  OVS_GREP_IFELSE([$KSRC/include/net/genetlink.h], [genl_validate_flags])
   OVS_FIND_PARAM_IFELSE([$KSRC/include/net/genetlink.h],
 [genl_notify], [net],
 [OVS_DEFINE([HAVE_GENL_NOTIFY_TAKES_NET])])
diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index b11a30965147..0c0d43bec2e5 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -2283,18 +2283,27 @@ exit_err:
 
 static struct genl_ops ct_limit_genl_ops[] = {
{ .cmd = OVS_CT_LIMIT_CMD_SET,
+#ifdef HAVE_GENL_VALIDATE_FLAGS
+   .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+#endif
.flags = GENL_ADMIN_PERM, /* Requires CAP_NET_ADMIN
   * privilege. */
.policy = ct_limit_policy,
.doit = ovs_ct_limit_cmd_set,
},
{ .cmd = OVS_CT_LIMIT_CMD_DEL,
+#ifdef HAVE_GENL_VALIDATE_FLAGS
+   .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+#endif
.flags = GENL_ADMIN_PERM, /* Requires CAP_NET_ADMIN
   * privilege. */
.policy = ct_limit_policy,
.doit = ovs_ct_limit_cmd_del,
},
{ .cmd = OVS_CT_LIMIT_CMD_GET,
+#ifdef HAVE_GENL_VALIDATE_FLAGS
+   .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+#endif
.flags = 0,   /* OK for unprivileged users. */
.policy = ct_limit_policy,
.doit = ovs_ct_limit_cmd_get,
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 78e2e6310529..f4244ea09869 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -652,6 +652,9 @@ static const struct nla_policy 
packet_policy[OVS_PACKET_ATTR_MAX + 1] = {
 
 static struct genl_ops dp_packet_genl_ops[] = {
{ .cmd = OVS_PACKET_CMD_EXECUTE,
+#ifdef HAVE_GENL_VALIDATE_FLAGS
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+#endif
  .flags = GENL_UNS_ADMIN_PERM, /* Requires CAP_NET_ADMIN privilege. */
  .policy = packet_policy,
  .doit = ovs_packet_cmd_execute
@@ -1440,22 +1443,34 @@ static const struct nla_policy 
flow_policy[OVS_FLOW_ATTR_MAX + 1] = {
 
 static struct genl_ops dp_flow_genl_ops[] = {
{ .cmd = OVS_FLOW_CMD_NEW,
+#ifdef HAVE_GENL_VALIDATE_FLAGS
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+#endif
  .flags = GENL_UNS_ADMIN_PERM, /* Requires CAP_NET_ADMIN privilege. */
  .policy = flow_policy,
  .doit = ovs_flow_cmd_new
},
{ .cmd = OVS_FLOW_CMD_DEL,
+#ifdef HAVE_GENL_VALIDATE_FLAGS
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+#endif
  .flags = GENL_UNS_ADMIN_PERM, /* Requires CAP_NET_ADMIN privilege. */
  .policy = flow_policy,
  .doit = ovs_flow_cmd_del
},
{ .cmd = OVS_FLOW_CMD_GET,
+#ifdef HAVE_GENL_VALIDATE_FLAGS
+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+#endif
  .flags = 0,   /* OK for unprivileged users. */
  .policy = flow_policy,
  .doit = ovs_flow_cmd_get,
  .dumpit 

[ovs-dev] [PATCH 05/11] datapath: Use nla_nest_start_noflag()

[Yi-Hung Wei]

This patch backports the openvswitch changes and update the compat layer
for the following upstream patch.

commit ae0be8de9a53cda3505865c11826d8ff0640237c
Author: Michal Kubecek 
Date:   Fri Apr 26 11:13:06 2019 +0200

netlink: make nla_nest_start() add NLA_F_NESTED flag

Even if the NLA_F_NESTED flag was introduced more than 11 years ago, most
netlink based interfaces (including recently added ones) are still not
setting it in kernel generated messages. Without the flag, message parsers
not aware of attribute semantics (e.g. wireshark dissector or libmnl's
mnl_nlmsg_fprintf()) cannot recognize nested attributes and won't display
the structure of their contents.

Unfortunately we cannot just add the flag everywhere as there may be
userspace applications which check nlattr::nla_type directly rather than
through a helper masking out the flags. Therefore the patch renames
nla_nest_start() to nla_nest_start_noflag() and introduces nla_nest_start()
as a wrapper adding NLA_F_NESTED. The calls which add NLA_F_NESTED manually
are rewritten to use nla_nest_start().

Except for changes in include/net/netlink.h, the patch was generated using
this semantic patch:

@@ expression E1, E2; @@
-nla_nest_start(E1, E2)
+nla_nest_start_noflag(E1, E2)

@@ expression E1, E2; @@
-nla_nest_start_noflag(E1, E2 | NLA_F_NESTED)
+nla_nest_start(E1, E2)

Signed-off-by: Michal Kubecek 
Acked-by: Jiri Pirko 
Acked-by: David Ahern 
Signed-off-by: David S. Miller 

Signed-off-by: Yi-Hung Wei 
---
 acinclude.m4|  1 +
 datapath/conntrack.c|  6 +++---
 datapath/datapath.c |  7 +++---
 datapath/flow_netlink.c | 33 +++--
 datapath/linux/compat/include/net/netlink.h |  9 
 datapath/meter.c|  8 +++
 datapath/vport-vxlan.c  |  2 +-
 datapath/vport.c|  2 +-
 8 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index dca09abefa96..fe121ab9126d 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -844,6 +844,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_put_in_addr])
   OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_find_nested])
   OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_is_last])
+  OVS_GREP_IFELSE([$KSRC/include/net/netlink.h], [nla_nest_start_noflag])
   OVS_GREP_IFELSE([$KSRC/include/linux/netlink.h], [void.*netlink_set_err],
   [OVS_DEFINE([HAVE_VOID_NETLINK_SET_ERR])])
   OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netlink.h],
diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 010f9af5ffd2..b11a30965147 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -1776,7 +1776,7 @@ static bool ovs_ct_nat_to_attr(const struct 
ovs_conntrack_info *info,
 {
struct nlattr *start;
 
-   start = nla_nest_start(skb, OVS_CT_ATTR_NAT);
+   start = nla_nest_start_noflag(skb, OVS_CT_ATTR_NAT);
if (!start)
return false;
 
@@ -1847,7 +1847,7 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info 
*ct_info,
 {
struct nlattr *start;
 
-   start = nla_nest_start(skb, OVS_ACTION_ATTR_CT);
+   start = nla_nest_start_noflag(skb, OVS_ACTION_ATTR_CT);
if (!start)
return -EMSGSIZE;
 
@@ -2257,7 +2257,7 @@ static int ovs_ct_limit_cmd_get(struct sk_buff *skb, 
struct genl_info *info)
if (IS_ERR(reply))
return PTR_ERR(reply);
 
-   nla_reply = nla_nest_start(reply, OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
+   nla_reply = nla_nest_start_noflag(reply, OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
 
if (a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]) {
err = ovs_ct_limit_get_zone_limit(
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 94e4f6ffd6e9..78e2e6310529 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -475,7 +475,8 @@ static int queue_userspace_packet(struct datapath *dp, 
struct sk_buff *skb,
 
 
if (upcall_info->egress_tun_info) {
-   nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_EGRESS_TUN_KEY);
+   nla = nla_nest_start_noflag(user_skb,
+   OVS_PACKET_ATTR_EGRESS_TUN_KEY);
if (!nla) {
err = -EMSGSIZE;
goto out;
@@ -487,7 +488,7 @@ static int queue_userspace_packet(struct datapath *dp, 
struct sk_buff *skb,
}
 
if (upcall_info->actions_len) {
-   nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_ACTIONS);
+   nla = nla_nest_start_noflag(user_skb, OVS_PACKET_ATTR_ACTIONS);
if (!nla) {
err = -EMSGSIZE;
goto out;
@@ -789,7 +790,7 @@ static int 

[ovs-dev] [PATCH 04/11] datapath: Handle NF_NAT_NEEDED replacement

[Yi-Hung Wei]

Starting from the following upstream commit, NF_NAT_NEEDED is replaced
by IS_ENABLED(CONFIG_NF_NAT) in the upstream kernel. This patch makes
some changes so that our in tree ovs kernel module is compatible to
both old and new kernels.

Upstream commit:
commit 4806e975729f99c7908d1688a143f1e16d464e6c
Author: Florian Westphal 
Date:   Wed Mar 27 09:22:26 2019 +0100

netfilter: replace NF_NAT_NEEDED with IS_ENABLED(CONFIG_NF_NAT)

NF_NAT_NEEDED is true whenever nat support for either ipv4 or ipv6 is
enabled.  Now that the af-specific nat configuration switches have been
removed, IS_ENABLED(CONFIG_NF_NAT) has the same effect.

Signed-off-by: Florian Westphal 
Signed-off-by: Pablo Neira Ayuso 

Signed-off-by: Yi-Hung Wei 
---
 acinclude.m4 |  1 +
 datapath/conntrack.c | 25 +
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index cc80026f2127..dca09abefa96 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -676,6 +676,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_FIND_FIELD_IFELSE([$KSRC/include/linux/netfilter.h], [nf_hook_ops],
 [owner], [OVS_DEFINE([HAVE_NF_HOOKS_OPS_OWNER])])
   OVS_GREP_IFELSE([$KSRC/include/linux/netfilter.h], [NFPROTO_INET])
+  OVS_GREP_IFELSE([$KSRC/include/linux/netfilter.h], [CONFIG_NF_NAT_NEEDED])
 
 
   OVS_FIND_FIELD_IFELSE([$KSRC/include/linux/netfilter_ipv6.h], [nf_ipv6_ops],
diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 1b345a03e704..010f9af5ffd2 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -34,7 +34,16 @@
 #include 
 #include 
 
-#ifdef CONFIG_NF_NAT_NEEDED
+/* Upstream commit 4806e975729f ("netfilter: replace NF_NAT_NEEDED with
+ * IS_ENABLED(CONFIG_NF_NAT)") replaces the config checking on NF_NAT_NEEDED
+ * with CONFIG_NF_NAT.  We will replace the checking on NF_NAT_NEEDED for the
+ * newer kernel with the marco in order to keep backward compatiblity.
+ */
+#ifndef HAVE_CONFIG_NF_NAT_NEEDED
+#define CONFIG_NF_NAT_NEEDED  CONFIG_NF_NAT
+#endif
+
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
 /* Starting from upstream commit 3bf195ae6037 ("netfilter: nat: merge
  * nf_nat_ipv4,6 into nat core") in kernel 5.1.  nf_nat_ipv4,6 are merged
  * into nf_nat.  In order to keep backward compatibility, we keep the config
@@ -100,7 +109,7 @@ struct ovs_conntrack_info {
struct md_labels labels;
char timeout[CTNL_TIMEOUT_NAME_MAX];
struct nf_ct_timeout *nf_ct_timeout;
-#ifdef CONFIG_NF_NAT_NEEDED
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
struct nf_nat_range2 range;  /* Only present for SRC NAT and DST NAT. */
 #endif
 };
@@ -786,7 +795,7 @@ static bool skb_nfct_cached(struct net *net,
return ct_executed;
 }
 
-#ifdef CONFIG_NF_NAT_NEEDED
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
 /* Modelled after nf_nat_ipv[46]_fn().
  * range is only used for new, uninitialized NAT state.
  * Returns either NF_ACCEPT or NF_DROP.
@@ -1405,7 +1414,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info 
*info, const char *name,
return 0;
 }
 
-#ifdef CONFIG_NF_NAT_NEEDED
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
 static int parse_nat(const struct nlattr *attr,
 struct ovs_conntrack_info *info, bool log)
 {
@@ -1547,7 +1556,7 @@ static const struct ovs_ct_len_tbl 
ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
.maxlen = sizeof(struct md_labels) },
[OVS_CT_ATTR_HELPER]= { .minlen = 1,
.maxlen = NF_CT_HELPER_NAME_LEN },
-#ifdef CONFIG_NF_NAT_NEEDED
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
/* NAT length is checked when parsing the nested attributes. */
[OVS_CT_ATTR_NAT]   = { .minlen = 0, .maxlen = INT_MAX },
 #endif
@@ -1627,7 +1636,7 @@ static int parse_ct(const struct nlattr *attr, struct 
ovs_conntrack_info *info,
return -EINVAL;
}
break;
-#ifdef CONFIG_NF_NAT_NEEDED
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
case OVS_CT_ATTR_NAT: {
int err = parse_nat(a, info, log);
 
@@ -1761,7 +1770,7 @@ err_free_ct:
return err;
 }
 
-#ifdef CONFIG_NF_NAT_NEEDED
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
 static bool ovs_ct_nat_to_attr(const struct ovs_conntrack_info *info,
   struct sk_buff *skb)
 {
@@ -1871,7 +1880,7 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info 
*ct_info,
return -EMSGSIZE;
}
 
-#ifdef CONFIG_NF_NAT_NEEDED
+#if IS_ENABLED(CONFIG_NF_NAT_NEEDED)
if (ct_info->nat && !ovs_ct_nat_to_attr(ct_info, skb))
return -EMSGSIZE;
 #endif
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 03/11] datapath: add seqadj extension when NAT is used.

[Yi-Hung Wei]

From: Flavio Leitner 

upstream patch:

commit fa7e428c6b7ed3281610511a2b2ec716d9894be8
Author: Flavio Leitner 
Date:   Mon Mar 25 15:58:31 2019 -0300

openvswitch: add seqadj extension when NAT is used.

When the conntrack is initialized, there is no helper attached
yet so the nat info initialization (nf_nat_setup_info) skips
adding the seqadj ext.

A helper is attached later when the conntrack is not confirmed
but is going to be committed. In this case, if NAT is needed then
adds the seqadj ext as well.

Fixes: 16ec3d4fbb96 ("openvswitch: Fix cached ct with helper.")
Signed-off-by: Flavio Leitner 
Acked-by: Pravin B Shelar 
Signed-off-by: David S. Miller 

Signed-off-by: Yi-Hung Wei 
---
 datapath/conntrack.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 291d4f4723d9..1b345a03e704 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -1063,6 +1063,12 @@ static int __ovs_ct_lookup(struct net *net, struct 
sw_flow_key *key,
GFP_ATOMIC);
if (err)
return err;
+
+   /* helper installed, add seqadj if NAT is required */
+   if (info->nat && !nfct_seqadj(ct)) {
+   if (!nfct_seqadj_ext_add(ct))
+   return -EINVAL;
+   }
}
 
/* Call the helper only if:
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 02/11] datapath: Detect upstream nf_nat change

[Yi-Hung Wei]

The following two upstream commits merge nf_nat_ipv4 and nf_nat_ipv6
into nf_nat core, and move some header files around.  To handle
these modifications, this patch detects the upstream changes, uses
the header files and config symbols properly.

Ideally, we should replace CONFIG_NF_NAT_IPV4 and CONFIG_NF_NAT_IPV6 with
CONFIG_NF_NAT and CONFIG_IPV6.  In order to keep backward compatibility,
we keep the checking of CONFIG_NF_NAT_IPV4/6 as is for the old kernel,
and replace them with marco for the new kernel.

upstream commits:
3bf195ae6037 ("netfilter: nat: merge nf_nat_ipv4,6 into nat core")
d2c5c103b133 ("netfilter: nat: remove nf_nat_l3proto.h and nf_nat_core.h")

Signed-off-by: Yi-Hung Wei 
---
 acinclude.m4 |  2 ++
 datapath/conntrack.c | 13 -
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 4072a7c8f58a..cc80026f2127 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -713,6 +713,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_nat.h], [nf_ct_nat_ext_add])
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_nat.h], 
[nf_nat_alloc_null_binding])
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_nat.h], [nf_nat_range2])
+  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_nat.h], [nf_nat_packet],
+  [OVS_DEFINE([HAVE_UPSTREAM_NF_NAT])])
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_seqadj.h], 
[nf_ct_seq_adjust])
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_count.h], 
[nf_conncount_gc_list],
   
[OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_count.h],
diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index afdd65b4cb7c..291d4f4723d9 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -35,10 +35,21 @@
 #include 
 
 #ifdef CONFIG_NF_NAT_NEEDED
+/* Starting from upstream commit 3bf195ae6037 ("netfilter: nat: merge
+ * nf_nat_ipv4,6 into nat core") in kernel 5.1.  nf_nat_ipv4,6 are merged
+ * into nf_nat.  In order to keep backward compatibility, we keep the config
+ * checking as is for the old kernel, and replace them with marco for the
+ * new kernel. */
+#ifdef HAVE_UPSTREAM_NF_NAT
+#include 
+#define CONFIG_NF_NAT_IPV4 CONFIG_NF_NAT
+#define CONFIG_NF_NAT_IPV6 CONFIG_IPV6
+#else
 #include 
 #include 
 #include 
-#endif
+#endif /* HAVE_UPSTREAM_NF_NAT */
+#endif /* CONFIG_NF_NAT_NEEDED */
 
 #include "datapath.h"
 #include "conntrack.h"
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 01/11] datapath: Replace nf_ct_invert_tuplepr() with nf_ct_invert_tuple()

[Yi-Hung Wei]

After upstream net-next commit 303e0c558959 ("netfilter: conntrack:
avoid unneeded nf_conntrack_l4proto lookups") nf_ct_invert_tuplepr()
is no longer available in the kernel.

Ideally, we should be in sync with upstream kernel by calling
nf_ct_invert_tuple() directly in conntrack.c.  However,
nf_ct_invert_tuple() has different function signature in older kernel,
and it would be hard to replace that in the compat layer. Thus, we
use pl_nf_ct_invert_tuple() in conntrack.c and maintain compatibility
in the compat layer so that ovs kernel module runs smoothly in both
new and old kernel.

Signed-off-by: Yi-Hung Wei 
---
 acinclude.m4   |  2 ++
 datapath/conntrack.c   |  2 +-
 .../linux/compat/include/net/netfilter/nf_conntrack_core.h | 14 ++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 52f92870eaaa..4072a7c8f58a 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -697,6 +697,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   [nf_ct_set])
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack.h],
   [nf_ct_is_untracked])
+  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack.h],
+  [nf_ct_invert_tuplepr])
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_zones.h],
   [nf_ct_zone_init])
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_l3proto.h],
diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index e328afe1ad15..afdd65b4cb7c 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -668,7 +668,7 @@ ovs_ct_find_existing(struct net *net, const struct 
nf_conntrack_zone *zone,
if (natted) {
struct nf_conntrack_tuple inverse;
 
-   if (!nf_ct_invert_tuplepr(, )) {
+   if (!rpl_nf_ct_invert_tuple(, )) {
pr_debug("ovs_ct_find_existing: Inversion failed!\n");
return NULL;
}
diff --git a/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h 
b/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h
index 10158011fd4d..ad52bc9412d8 100644
--- a/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h
+++ b/datapath/linux/compat/include/net/netfilter/nf_conntrack_core.h
@@ -113,4 +113,18 @@ rpl_nf_conntrack_in(struct sk_buff *skb, const struct 
nf_hook_state *state)
 #define nf_conntrack_in rpl_nf_conntrack_in
 #endif /* HAVE_NF_CONNTRACK_IN_TAKES_NF_HOOK_STATE */
 
+#ifdef HAVE_NF_CT_INVERT_TUPLEPR
+static inline bool rpl_nf_ct_invert_tuple(struct nf_conntrack_tuple *inverse,
+   const struct nf_conntrack_tuple *orig)
+{
+   return nf_ct_invert_tuplepr(inverse, orig);
+}
+#else
+static inline bool rpl_nf_ct_invert_tuple(struct nf_conntrack_tuple *inverse,
+   const struct nf_conntrack_tuple *orig)
+{
+   return nf_ct_invert_tuple(inverse, orig);
+}
+#endif /* HAVE_NF_CT_INVERT_TUPLEPR */
+
 #endif /* _NF_CONNTRACK_CORE_WRAPPER_H */
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 00/11] Backport upstream conntrack related patches

[Yi-Hung Wei]

This series backports conntrack related patches from upstream kernel.
It has been tested on
* Ubuntu 14.04, 16.04, and 18.04.
* RHEL 7.4-7.6.
* 5.0 kernel.

Travis test: https://travis-ci.org/YiHungWei/ovs/builds/596361660

Colin Ian King (1):
  datapath: Check for null pointer return from nla_nest_start_noflag

Flavio Leitner (1):
  datapath: add seqadj extension when NAT is used.

Yi-Hung Wei (9):
  datapath: Replace nf_ct_invert_tuplepr() with nf_ct_invert_tuple()
  datapath: Detect upstream nf_nat change
  datapath: Handle NF_NAT_NEEDED replacement
  datapath: Use nla_nest_start_noflag()
  datapath: genetlink: optionally validate strictly/dumps
  datapath: Load and reference the NAT helper.
  datapath: Replace removed NF_NAT_NEEDED with IS_ENABLED(CONFIG_NF_NAT)
  datapath: Fix log message in ovs conntrack
  datapath: Allow attaching helper in later commit

 acinclude.m4   |  11 ++
 datapath/conntrack.c   | 115 -
 datapath/datapath.c|  46 -
 datapath/flow_netlink.c|  33 +++---
 .../include/net/netfilter/nf_conntrack_core.h  |  14 +++
 .../include/net/netfilter/nf_conntrack_helper.h|  17 +++
 datapath/linux/compat/include/net/netlink.h|   9 ++
 datapath/meter.c   |  20 +++-
 datapath/vport-vxlan.c |   2 +-
 datapath/vport.c   |   2 +-
 10 files changed, 216 insertions(+), 53 deletions(-)

-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3] ovsdb-server: Don't drop all connections on read/write status change.

[nusiddiq]

From: Numan Siddique 

The commit [1] force drops all connections when the db read/write status 
changes.
Prior to the commit [1], when there was read/write status change, the existing
jsonrpc sessions with 'db_change_aware' set to true, were not updated with the
changed 'read_only' value. If the db status was changed to 'standby', the 
existing
clients could still write to the db.

In the case of pacemaker OVN HA, OVN OCF script 'start' action starts the
ovsdb-servers in read-only state and later, it sets to read-write in the
'promote' action. We have observed that if some ovn-controllers connect to
the SB ovsdb-server (in read-only state) just before the 'promote' action,
the connection is not reset all the times and these ovn-controllers remain 
connected
to the SB ovsdb-server in read-only state all the time. Even though
the commit [1] calls 'ovsdb_jsonrpc_server_reconnect()' with 'forced' flag
set to true when the db read/write status changes, somehow the FSM misses 
resetting
the connections of these ovn-controllers.

I think this needs to be addressed in the FSM. This patch doesn't address
this FSM issue. Instead it changes the behavior of 
'ovsdb_jsonrpc_server_set_read_only()'
by setting the 'read_only' flag of all the jsonrpc sessions instead of 
forcefully
resetting the connection.

I think there is no need to reset the connection. In large scale production
deployements with OVN, this results in unnecessary waste of CPU cycles as 
ovn-controllers
will have to connect twice - once during 'start' action and again during 
'promote'.

[1] - 2a9679e3b2c6("ovsdb-server: drop all connections on read/write status 
change")

Acked-by: Dumitru Ceara 
Signed-off-by: Numan Siddique 
---

v2 -> v3
---
  * Addressed minor review comment.

v1 -> v2
---
  * Addressed Dumitru's comment - Use LIST_FOR_EACH instead of
LIST_FOR_EACH_SAFE

 ovsdb/jsonrpc-server.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index ddbbc2e94..4e2dfc3d7 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -80,6 +80,8 @@ static void ovsdb_jsonrpc_session_unlock_all(struct 
ovsdb_jsonrpc_session *);
 static void ovsdb_jsonrpc_session_unlock__(struct ovsdb_lock_waiter *);
 static void ovsdb_jsonrpc_session_send(struct ovsdb_jsonrpc_session *,
struct jsonrpc_msg *);
+static void ovsdb_jsonrpc_session_set_readonly_all(
+struct ovsdb_jsonrpc_remote *remote, bool read_only);
 
 /* Triggers. */
 static void ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *,
@@ -365,10 +367,13 @@ ovsdb_jsonrpc_server_set_read_only(struct 
ovsdb_jsonrpc_server *svr,
 {
 if (svr->read_only != read_only) {
 svr->read_only = read_only;
-ovsdb_jsonrpc_server_reconnect(svr, true,
-   xstrdup(read_only
-   ? "making server read-only"
-   : "making server read/write"));
+
+struct shash_node *node;
+SHASH_FOR_EACH (node, >remotes) {
+struct ovsdb_jsonrpc_remote *remote = node->data;
+
+ovsdb_jsonrpc_session_set_readonly_all(remote, read_only);
+}
 }
 }
 
@@ -670,6 +675,17 @@ ovsdb_jsonrpc_session_reconnect_all(struct 
ovsdb_jsonrpc_remote *remote,
 }
 }
 
+static void
+ovsdb_jsonrpc_session_set_readonly_all(struct ovsdb_jsonrpc_remote *remote,
+   bool read_only)
+{
+struct ovsdb_jsonrpc_session *s;
+
+LIST_FOR_EACH (s, node, >sessions) {
+s->read_only = read_only;
+}
+}
+
 /* Sets the options for all of the JSON-RPC sessions managed by 'remote' to
  * 'options'.
  *
-- 
2.21.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovsdb-server: Don't drop all connections on read/write status change.

[Dumitru Ceara]

On Mon, Oct 14, 2019 at 11:33 AM  wrote:
>
> From: Numan Siddique 
>
> The commit [1] force drops all connections when the db read/write status 
> changes.
> Prior to the commit [1], when there was read/write status change, the existing
> jsonrpc sessions with 'db_change_aware' set to true, were not updated with the
> changed 'read_only' value. If the db status was changed to 'standby', the 
> existing
> clients could still write to the db.
>
> In the case of pacemaker OVN HA, OVN OCF script 'start' action starts the
> ovsdb-servers in read-only state and later, it sets to read-write in the
> 'promote' action. We have observed that if some ovn-controllers connect to
> the SB ovsdb-server (in read-only state) just before the 'promote' action,
> the connection is not reset all the times and these ovn-controllers remain 
> connected
> to the SB ovsdb-server in read-only state all the time. Even though
> the commit [1] calls 'ovsdb_jsonrpc_server_reconnect()' with 'forced' flag
> set to true when the db read/write status changes, somehow the FSM misses 
> resetting
> the connections of these ovn-controllers.
>
> I think this needs to be addressed in the FSM. This patch doesn't address
> this FSM issue. Instead it changes the behavior of 
> 'ovsdb_jsonrpc_server_set_read_only()'
> by setting the 'read_only' flag of all the jsonrpc sessions instead of 
> forcefully
> resetting the connection.
>
> I think there is no need to reset the connection. In large scale production
> deployements with OVN, this results in unnecessary waste of CPU cycles as 
> ovn-controllers
> will have to connect twice - once during 'start' action and again during 
> 'promote'.
>
> [1] - 2a9679e3b2c6("ovsdb-server: drop all connections on read/write status 
> change")
>
> Signed-off-by: Numan Siddique 
> ---
>
> v1 -> v2
> ---
>   * Addressed Dumitru's comment - Use LIST_FOR_EACH instead of
> LIST_FOR_EACH_SAFE
>
>  ovsdb/jsonrpc-server.c | 24 
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index ddbbc2e94..a98c5e618 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -80,6 +80,8 @@ static void ovsdb_jsonrpc_session_unlock_all(struct 
> ovsdb_jsonrpc_session *);
>  static void ovsdb_jsonrpc_session_unlock__(struct ovsdb_lock_waiter *);
>  static void ovsdb_jsonrpc_session_send(struct ovsdb_jsonrpc_session *,
> struct jsonrpc_msg *);
> +static void ovsdb_jsonrpc_session_set_readonly_all(
> +struct ovsdb_jsonrpc_remote *remote, bool read_only);
>
>  /* Triggers. */
>  static void ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *,
> @@ -365,10 +367,13 @@ ovsdb_jsonrpc_server_set_read_only(struct 
> ovsdb_jsonrpc_server *svr,
>  {
>  if (svr->read_only != read_only) {
>  svr->read_only = read_only;
> -ovsdb_jsonrpc_server_reconnect(svr, true,
> -   xstrdup(read_only
> -   ? "making server read-only"
> -   : "making server 
> read/write"));
> +struct shash_node *node;

Sorry, I forgot to mention this minor comment earlier.. Could you
please move the variable declaration one line before "svr->read_only =
read_only;" or add another empty line between the assignment and
variable declaration.

Except for that:

Acked-by: Dumitru Ceara 

> +
> +SHASH_FOR_EACH (node, >remotes) {
> +struct ovsdb_jsonrpc_remote *remote = node->data;
> +
> +ovsdb_jsonrpc_session_set_readonly_all(remote, read_only);
> +}
>  }
>  }
>
> @@ -670,6 +675,17 @@ ovsdb_jsonrpc_session_reconnect_all(struct 
> ovsdb_jsonrpc_remote *remote,
>  }
>  }
>
> +static void
> +ovsdb_jsonrpc_session_set_readonly_all(struct ovsdb_jsonrpc_remote *remote,
> +   bool read_only)
> +{
> +struct ovsdb_jsonrpc_session *s;
> +
> +LIST_FOR_EACH (s, node, >sessions) {
> +s->read_only = read_only;
> +}
> +}
> +
>  /* Sets the options for all of the JSON-RPC sessions managed by 'remote' to
>   * 'options'.
>   *
> --
> 2.21.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] (no subject)

[Rika mentari]

Hallo..
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb-server: Don't drop all connections on read/write status change.

[Numan Siddique]

On Mon, Oct 14, 2019 at 2:23 PM Dumitru Ceara  wrote:

> On Mon, Oct 14, 2019 at 8:21 AM  wrote:
> >
> > From: Numan Siddique 
> >
> > The commit [1] force drops all connections when the db read/write status
> changes.
> > Prior to the commit [1], when there was read/write status change, the
> existing
> > jsonrpc sessions with 'db_change_aware' set to true, were not updated
> with the
> > changed 'read_only' value. If the db status was changed to 'standby',
> the existing
> > clients could still write to the db.
> >
> > In the case of pacemaker OVN HA, OVN OCF script 'start' action starts the
> > ovsdb-servers in read-only state and later, it sets to read-write in the
> > 'promote' action. We have observed that if some ovn-controllers connect
> to
> > the SB ovsdb-server (in read-only state) just before the 'promote'
> action,
> > the connection is not reset all the times and these ovn-controllers
> remain connected
> > to the SB ovsdb-server in read-only state all the time. Even though
> > the commit [1] calls 'ovsdb_jsonrpc_server_reconnect()' with 'forced'
> flag
> > set to true when the db read/write status changes, somehow the FSM
> misses resetting
> > the connections of these ovn-controllers.
> >
> > I think this needs to be addressed in the FSM. This patch doesn't address
> > this FSM issue. Instead it changes the behavior of
> 'ovsdb_jsonrpc_server_set_read_only()'
> > by setting the 'read_only' flag of all the jsonrpc sessions instead of
> forcefully
> > resetting the connection.
> >
> > I think there is no need to reset the connection. In large scale
> production
> > deployements with OVN, this results in unnecessary waste of CPU cycles
> as ovn-controllers
> > will have to connect twice - once during 'start' action and again during
> 'promote'.
> >
> > [1] - 2a9679e3b2c6("ovsdb-server: drop all connections on read/write
> status change")
> >
> > Signed-off-by: Numan Siddique 
>
> Hi Numan,
>
> The code looks good to me, just a minor comment below.
>
> Thanks,
> Dumitru
>
> > ---
> >  ovsdb/jsonrpc-server.c | 24 
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> > index ddbbc2e94..066826959 100644
> > --- a/ovsdb/jsonrpc-server.c
> > +++ b/ovsdb/jsonrpc-server.c
> > @@ -80,6 +80,8 @@ static void ovsdb_jsonrpc_session_unlock_all(struct
> ovsdb_jsonrpc_session *);
> >  static void ovsdb_jsonrpc_session_unlock__(struct ovsdb_lock_waiter *);
> >  static void ovsdb_jsonrpc_session_send(struct ovsdb_jsonrpc_session *,
> > struct jsonrpc_msg *);
> > +static void ovsdb_jsonrpc_session_set_readonly_all(
> > +struct ovsdb_jsonrpc_remote *remote, bool read_only);
> >
> >  /* Triggers. */
> >  static void ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *,
> > @@ -365,10 +367,13 @@ ovsdb_jsonrpc_server_set_read_only(struct
> ovsdb_jsonrpc_server *svr,
> >  {
> >  if (svr->read_only != read_only) {
> >  svr->read_only = read_only;
> > -ovsdb_jsonrpc_server_reconnect(svr, true,
> > -   xstrdup(read_only
> > -   ? "making server
> read-only"
> > -   : "making server
> read/write"));
> > +struct shash_node *node;
> > +
> > +SHASH_FOR_EACH (node, >remotes) {
> > +struct ovsdb_jsonrpc_remote *remote = node->data;
> > +
> > +ovsdb_jsonrpc_session_set_readonly_all(remote, read_only);
> > +}
> >  }
> >  }
> >
> > @@ -670,6 +675,17 @@ ovsdb_jsonrpc_session_reconnect_all(struct
> ovsdb_jsonrpc_remote *remote,
> >  }
> >  }
> >
> > +static void
> > +ovsdb_jsonrpc_session_set_readonly_all(struct ovsdb_jsonrpc_remote
> *remote,
> > +   bool read_only)
> > +{
> > +struct ovsdb_jsonrpc_session *s, *next;
> > +
> > +LIST_FOR_EACH_SAFE (s, next, node, >sessions) {
>
> LIST_FOR_EACH should be enough here.
>
>
Thanks for the review. Agree.
Sent v2.

Thanks
Numan


> > +s->read_only = read_only;
> > +}
> > +}
> > +
> >  /* Sets the options for all of the JSON-RPC sessions managed by
> 'remote' to
> >   * 'options'.
> >   *
> > --
> > 2.21.0
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] ovsdb-server: Don't drop all connections on read/write status change.

[nusiddiq]

From: Numan Siddique 

The commit [1] force drops all connections when the db read/write status 
changes.
Prior to the commit [1], when there was read/write status change, the existing
jsonrpc sessions with 'db_change_aware' set to true, were not updated with the
changed 'read_only' value. If the db status was changed to 'standby', the 
existing
clients could still write to the db.

In the case of pacemaker OVN HA, OVN OCF script 'start' action starts the
ovsdb-servers in read-only state and later, it sets to read-write in the
'promote' action. We have observed that if some ovn-controllers connect to
the SB ovsdb-server (in read-only state) just before the 'promote' action,
the connection is not reset all the times and these ovn-controllers remain 
connected
to the SB ovsdb-server in read-only state all the time. Even though
the commit [1] calls 'ovsdb_jsonrpc_server_reconnect()' with 'forced' flag
set to true when the db read/write status changes, somehow the FSM misses 
resetting
the connections of these ovn-controllers.

I think this needs to be addressed in the FSM. This patch doesn't address
this FSM issue. Instead it changes the behavior of 
'ovsdb_jsonrpc_server_set_read_only()'
by setting the 'read_only' flag of all the jsonrpc sessions instead of 
forcefully
resetting the connection.

I think there is no need to reset the connection. In large scale production
deployements with OVN, this results in unnecessary waste of CPU cycles as 
ovn-controllers
will have to connect twice - once during 'start' action and again during 
'promote'.

[1] - 2a9679e3b2c6("ovsdb-server: drop all connections on read/write status 
change")

Signed-off-by: Numan Siddique 
---

v1 -> v2
---
  * Addressed Dumitru's comment - Use LIST_FOR_EACH instead of
LIST_FOR_EACH_SAFE

 ovsdb/jsonrpc-server.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index ddbbc2e94..a98c5e618 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -80,6 +80,8 @@ static void ovsdb_jsonrpc_session_unlock_all(struct 
ovsdb_jsonrpc_session *);
 static void ovsdb_jsonrpc_session_unlock__(struct ovsdb_lock_waiter *);
 static void ovsdb_jsonrpc_session_send(struct ovsdb_jsonrpc_session *,
struct jsonrpc_msg *);
+static void ovsdb_jsonrpc_session_set_readonly_all(
+struct ovsdb_jsonrpc_remote *remote, bool read_only);
 
 /* Triggers. */
 static void ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *,
@@ -365,10 +367,13 @@ ovsdb_jsonrpc_server_set_read_only(struct 
ovsdb_jsonrpc_server *svr,
 {
 if (svr->read_only != read_only) {
 svr->read_only = read_only;
-ovsdb_jsonrpc_server_reconnect(svr, true,
-   xstrdup(read_only
-   ? "making server read-only"
-   : "making server read/write"));
+struct shash_node *node;
+
+SHASH_FOR_EACH (node, >remotes) {
+struct ovsdb_jsonrpc_remote *remote = node->data;
+
+ovsdb_jsonrpc_session_set_readonly_all(remote, read_only);
+}
 }
 }
 
@@ -670,6 +675,17 @@ ovsdb_jsonrpc_session_reconnect_all(struct 
ovsdb_jsonrpc_remote *remote,
 }
 }
 
+static void
+ovsdb_jsonrpc_session_set_readonly_all(struct ovsdb_jsonrpc_remote *remote,
+   bool read_only)
+{
+struct ovsdb_jsonrpc_session *s;
+
+LIST_FOR_EACH (s, node, >sessions) {
+s->read_only = read_only;
+}
+}
+
 /* Sets the options for all of the JSON-RPC sessions managed by 'remote' to
  * 'options'.
  *
-- 
2.21.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb-server: Don't drop all connections on read/write status change.

[Dumitru Ceara]

On Mon, Oct 14, 2019 at 8:21 AM  wrote:
>
> From: Numan Siddique 
>
> The commit [1] force drops all connections when the db read/write status 
> changes.
> Prior to the commit [1], when there was read/write status change, the existing
> jsonrpc sessions with 'db_change_aware' set to true, were not updated with the
> changed 'read_only' value. If the db status was changed to 'standby', the 
> existing
> clients could still write to the db.
>
> In the case of pacemaker OVN HA, OVN OCF script 'start' action starts the
> ovsdb-servers in read-only state and later, it sets to read-write in the
> 'promote' action. We have observed that if some ovn-controllers connect to
> the SB ovsdb-server (in read-only state) just before the 'promote' action,
> the connection is not reset all the times and these ovn-controllers remain 
> connected
> to the SB ovsdb-server in read-only state all the time. Even though
> the commit [1] calls 'ovsdb_jsonrpc_server_reconnect()' with 'forced' flag
> set to true when the db read/write status changes, somehow the FSM misses 
> resetting
> the connections of these ovn-controllers.
>
> I think this needs to be addressed in the FSM. This patch doesn't address
> this FSM issue. Instead it changes the behavior of 
> 'ovsdb_jsonrpc_server_set_read_only()'
> by setting the 'read_only' flag of all the jsonrpc sessions instead of 
> forcefully
> resetting the connection.
>
> I think there is no need to reset the connection. In large scale production
> deployements with OVN, this results in unnecessary waste of CPU cycles as 
> ovn-controllers
> will have to connect twice - once during 'start' action and again during 
> 'promote'.
>
> [1] - 2a9679e3b2c6("ovsdb-server: drop all connections on read/write status 
> change")
>
> Signed-off-by: Numan Siddique 

Hi Numan,

The code looks good to me, just a minor comment below.

Thanks,
Dumitru

> ---
>  ovsdb/jsonrpc-server.c | 24 
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index ddbbc2e94..066826959 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -80,6 +80,8 @@ static void ovsdb_jsonrpc_session_unlock_all(struct 
> ovsdb_jsonrpc_session *);
>  static void ovsdb_jsonrpc_session_unlock__(struct ovsdb_lock_waiter *);
>  static void ovsdb_jsonrpc_session_send(struct ovsdb_jsonrpc_session *,
> struct jsonrpc_msg *);
> +static void ovsdb_jsonrpc_session_set_readonly_all(
> +struct ovsdb_jsonrpc_remote *remote, bool read_only);
>
>  /* Triggers. */
>  static void ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *,
> @@ -365,10 +367,13 @@ ovsdb_jsonrpc_server_set_read_only(struct 
> ovsdb_jsonrpc_server *svr,
>  {
>  if (svr->read_only != read_only) {
>  svr->read_only = read_only;
> -ovsdb_jsonrpc_server_reconnect(svr, true,
> -   xstrdup(read_only
> -   ? "making server read-only"
> -   : "making server 
> read/write"));
> +struct shash_node *node;
> +
> +SHASH_FOR_EACH (node, >remotes) {
> +struct ovsdb_jsonrpc_remote *remote = node->data;
> +
> +ovsdb_jsonrpc_session_set_readonly_all(remote, read_only);
> +}
>  }
>  }
>
> @@ -670,6 +675,17 @@ ovsdb_jsonrpc_session_reconnect_all(struct 
> ovsdb_jsonrpc_remote *remote,
>  }
>  }
>
> +static void
> +ovsdb_jsonrpc_session_set_readonly_all(struct ovsdb_jsonrpc_remote *remote,
> +   bool read_only)
> +{
> +struct ovsdb_jsonrpc_session *s, *next;
> +
> +LIST_FOR_EACH_SAFE (s, next, node, >sessions) {

LIST_FOR_EACH should be enough here.

> +s->read_only = read_only;
> +}
> +}
> +
>  /* Sets the options for all of the JSON-RPC sessions managed by 'remote' to
>   * 'options'.
>   *
> --
> 2.21.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovsdb-server: Don't drop all connections on read/write status change.

[nusiddiq]

From: Numan Siddique 

The commit [1] force drops all connections when the db read/write status 
changes.
Prior to the commit [1], when there was read/write status change, the existing
jsonrpc sessions with 'db_change_aware' set to true, were not updated with the
changed 'read_only' value. If the db status was changed to 'standby', the 
existing
clients could still write to the db.

In the case of pacemaker OVN HA, OVN OCF script 'start' action starts the
ovsdb-servers in read-only state and later, it sets to read-write in the
'promote' action. We have observed that if some ovn-controllers connect to
the SB ovsdb-server (in read-only state) just before the 'promote' action,
the connection is not reset all the times and these ovn-controllers remain 
connected
to the SB ovsdb-server in read-only state all the time. Even though
the commit [1] calls 'ovsdb_jsonrpc_server_reconnect()' with 'forced' flag
set to true when the db read/write status changes, somehow the FSM misses 
resetting
the connections of these ovn-controllers.

I think this needs to be addressed in the FSM. This patch doesn't address
this FSM issue. Instead it changes the behavior of 
'ovsdb_jsonrpc_server_set_read_only()'
by setting the 'read_only' flag of all the jsonrpc sessions instead of 
forcefully
resetting the connection.

I think there is no need to reset the connection. In large scale production
deployements with OVN, this results in unnecessary waste of CPU cycles as 
ovn-controllers
will have to connect twice - once during 'start' action and again during 
'promote'.

[1] - 2a9679e3b2c6("ovsdb-server: drop all connections on read/write status 
change")

Signed-off-by: Numan Siddique 
---
 ovsdb/jsonrpc-server.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index ddbbc2e94..066826959 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -80,6 +80,8 @@ static void ovsdb_jsonrpc_session_unlock_all(struct 
ovsdb_jsonrpc_session *);
 static void ovsdb_jsonrpc_session_unlock__(struct ovsdb_lock_waiter *);
 static void ovsdb_jsonrpc_session_send(struct ovsdb_jsonrpc_session *,
struct jsonrpc_msg *);
+static void ovsdb_jsonrpc_session_set_readonly_all(
+struct ovsdb_jsonrpc_remote *remote, bool read_only);
 
 /* Triggers. */
 static void ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *,
@@ -365,10 +367,13 @@ ovsdb_jsonrpc_server_set_read_only(struct 
ovsdb_jsonrpc_server *svr,
 {
 if (svr->read_only != read_only) {
 svr->read_only = read_only;
-ovsdb_jsonrpc_server_reconnect(svr, true,
-   xstrdup(read_only
-   ? "making server read-only"
-   : "making server read/write"));
+struct shash_node *node;
+
+SHASH_FOR_EACH (node, >remotes) {
+struct ovsdb_jsonrpc_remote *remote = node->data;
+
+ovsdb_jsonrpc_session_set_readonly_all(remote, read_only);
+}
 }
 }
 
@@ -670,6 +675,17 @@ ovsdb_jsonrpc_session_reconnect_all(struct 
ovsdb_jsonrpc_remote *remote,
 }
 }
 
+static void
+ovsdb_jsonrpc_session_set_readonly_all(struct ovsdb_jsonrpc_remote *remote,
+   bool read_only)
+{
+struct ovsdb_jsonrpc_session *s, *next;
+
+LIST_FOR_EACH_SAFE (s, next, node, >sessions) {
+s->read_only = read_only;
+}
+}
+
 /* Sets the options for all of the JSON-RPC sessions managed by 'remote' to
  * 'options'.
  *
-- 
2.21.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev