Re: [ovs-dev] [External] Re: Deadlock happen when delete vhostuserclient port while qemu destroy virtio

2023-03-20 Thread Han Ding 


>On 2/28/23 17:14, Ilya Maximets wrote:
>> On 2/28/23 17:05, Maxime Coquelin wrote:
>>>
>>>
>>> On 2/28/23 16:37, Ilya Maximets wrote:
>>>> On 2/28/23 16:16, Maxime Coquelin wrote:
>>>>> Hi,
>>>>>
>>>>> On 2/28/23 10:33, Wan Junjie wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, Feb 28, 2023 at 4:03 PM David Marchand
>>>>>>  wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> On Fri, Feb 24, 2023 at 2:55 AM Han Ding  
>>>>>>> wrote:
>>>>>>>>> On 2/23/23 10:35, Han Ding wrote:
>>>>>>>>>>
>>>>>>>>>> When use ovs-vsctl to delete vhostuserclient port while qemu destroy 
>>>>>>>>>> virtio,
>>>>>>>>>> there is a deadlock between OVS main thread and the vhost-events 
>>>>>>>>>> thread.
>>>>>>>>>>
>>>>>>>>>> openvswitch is 2.14 and dpdk is 20.11
>>>>>>>>>
>>>>>>>>> FWIW, 2.14 supposed to be used with 19.11.
>>>>>>>
>>>>>>> Indeed.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks for reply.
>>>>>>>>
>>>>>>>> The code flow path of openvswitch 3.0.1 and dpdk 21.11, dpdk 22.11 is 
>>>>>>>> same
>>>>>>>> for destroying vhostuserclient device. So, I think  there is a same 
>>>>>>>> bug in new version.
>>>>>>>
>>>>>>> Please double check the dpdk versions you are using, then have a look
>>>>>>> at my comment below.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Main thread:
>>>>>>>>>> ofport_remove
>>>>>>>>>>     -> netdev_unref
>>>>>>>>>>     -> netdev_dpdk_vhost_destruct
>>>>>>>>>>  -> rte_vhost_driver_unregister (If fdentry is busy, 
>>>>>>>>>> circle again until the fdentry is not busy)
>>>>>>>>>>  -> fdset_try_del  (fdentry is busy now, return -1. 
>>>>>>>>>> Goto again)
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> vhost-nets thread:
>>>>>>>>>> fdset_event_dispatch (set fdentry to busy. When destroy_device 
>>>>>>>>>> fuction return set the fdentry to no busy)
>>>>>>>>>>  -> vhost_user_read_cb
>>>>>>>>>>    -> vhost_user_msg_handler
>>>>>>>>>>    -> vhost_user_get_vring_base
>>>>>>>>>>    ->destroy_device
>>>>>>>>>>   -> ovsrcu_synchronize (Wait for other threads to 
>>>>>>>>>> quiesce)
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>> The  vhost-nets thread wait for main thread to quiesce, but the main 
>>>>>>>>>> thread now is waiting for fdentry to no busy
>>>>>>>>>> and circle all the time.
>>>>>>>>
>>>>>>
>>>>>>
>>>>>> I believe this issue exists in older versions, we have seen it before.
>>>>>>
>>>>>> When we create and destroy vm concurrently and continuously. This
>>>>>> could be reproduced easily.
>>>>>>
>>>>>> We found a related patch from dpdk which is not accepted.
>>>>>>
>>>>>> https://patchwork.dpdk.org/project/dpdk/patch/1584007039-12437-1-git-send-email-wangzh...@jd.com/
>>>>>
>>>>> Thanks for the pointer.
>>>>>
>>>>> I spent some time thinking about it, and I do not see other alternative
>>>>> than doing what the patch you shared do, i.e. returning -EAGAIN and
>>>&

Re: [ovs-dev] Deadlock happen when delete vhostuserclient port while qemu destroy virtio

2023-02-23 Thread Han Ding 


>On 2/23/23 10:35, Han Ding wrote:
>> 
>> When use ovs-vsctl to delete vhostuserclient port while qemu destroy virtio,
>> there is a deadlock between OVS main thread and the vhost-events thread.
>> 
>> openvswitch is 2.14 and dpdk is 20.11
>
>FWIW, 2.14 supposed to be used with 19.11.

Thanks for reply.  

The code flow path of openvswitch 3.0.1 and dpdk 21.11, dpdk 22.11 is same
for destroying vhostuserclient device. So, I think  there is a same bug in new 
version.


>> 
>> Main thread:
>> ofport_remove
>>   -> netdev_unref
>>       -> netdev_dpdk_vhost_destruct
>>            -> rte_vhost_driver_unregister (If fdentry is busy, circle again 
>> until the fdentry is not busy)
>>                -> fdset_try_del  (fdentry is busy now, return -1. Goto again)
>> 



>> vhost-nets thread:
>> fdset_event_dispatch (set fdentry to busy. When destroy_device fuction 
>> return set the fdentry to no busy)
>>    -> vhost_user_read_cb 
>>      -> vhost_user_msg_handler
>>          -> vhost_user_get_vring_base
>>              ->destroy_device 
>>                 -> ovsrcu_synchronize (Wait for other threads to quiesce)



>> The  vhost-nets thread wait for main thread to quiesce, but the main thread 
>> now is waiting for fdentry to no busy 
>> and circle all the time.  


>> 
>> Whether the ovsrcu_synchronize  is necessary in destrory_device
>
>Yes, we have to wait for all threads to stop using this device.
>
>> or the rte_vhost_driver_unregister is correct in fdset_try_del ?
>
>CC: David and Maxime.
>
>Best regards, Ilya Maximets


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

[ovs-dev] Deadlock happen when delete vhostuserclient port while qemu destroy virtio

2023-02-23 Thread Han Ding 


When use ovs-vsctl to delete vhostuserclient port while qemu destroy virtio, 
there is a deadlock between OVS main thread and the vhost-events thread.

openvswitch is 2.14 and dpdk is 20.11

Main thread:
ofport_remove
  -> netdev_unref
  -> netdev_dpdk_vhost_destruct
   -> rte_vhost_driver_unregister (If fdentry is busy, circle again 
until the fdentry is not busy)
   -> fdset_try_del  (fdentry is busy now, return -1. Goto again)

vhost-nets thread:
fdset_event_dispatch (set fdentry to busy. When destroy_device fuction return 
set the fdentry to no busy)
   -> vhost_user_read_cb 
 -> vhost_user_msg_handler
 -> vhost_user_get_vring_base
 ->destroy_device 
-> ovsrcu_synchronize (Wait for other threads to quiesce)

The  vhost-nets thread wait for main thread to quiesce, but the main thread now 
is waiting for fdentry to no busy 
and circle all the time.  

Whether the ovsrcu_synchronize  is necessary in destrory_device or the 
rte_vhost_driver_unregister is correct in fdset_try_del ?

Best regards, Han Ding.


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

[ovs-dev] [PATCH v4] ofproto-dpif-xlate: Remove repeated function for judge garp

2023-02-20 Thread Han Ding 


Function is_gratuitous_arp() and function is_garp() are all used to judge
whether the flow is gratuitous arp. It is not necessary to use two functions
to do the same thing and just keep one.

Gratuitous ARP message is a generally link-level broadcast messages and
carry the same IP in sender and target fields. This patch add the check
in function is_garp() whether the arp is a broadcast message and whether
the arp is an arp request or an arp reply.

Signed-off-by: Han Ding 
---

Notes:
v2:
- Add the check whether the ARP packet is a broadcast message in is_garp().

v3:
- Add the check whether the packet is an arp request or an arp reply in 
is_garp().

v4:
- Add description of differences between patch versions
- Use WC_MASK_FIELD() macro instead of memset.

 lib/flow.h   | 16 +---
 ofproto/ofproto-dpif-xlate.c | 32 ++--
 2 files changed, 15 insertions(+), 33 deletions(-)

diff --git a/lib/flow.h b/lib/flow.h
index c647ad83c..037deb6cd 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -1133,10 +1133,20 @@ static inline bool is_garp(const struct flow *flow,
struct flow_wildcards *wc)
 {
 if (is_arp(flow)) {
-return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
-FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
-}
+if (!eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, dl_dst))) 
{
+return false;
+}

+if (wc) {
+WC_MASK_FIELD(wc, nw_proto);
+}
+
+if (flow->nw_proto == ARP_OP_REQUEST ||
+flow->nw_proto == ARP_OP_REPLY) {
+return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
+FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
+}
+}
 return false;
 }

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a9cf3cbee..b3c13f6bf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct 
xbundle *out_xbundle,
 memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
 }

-/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
- * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies to
- * indicate this; newer upstream kernels use gratuitous ARP requests. */
-static bool
-is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
-{
-if (flow->dl_type != htons(ETH_TYPE_ARP)) {
-return false;
-}
-
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-if (!eth_addr_is_broadcast(flow->dl_dst)) {
-return false;
-}
-
-memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-if (flow->nw_proto == ARP_OP_REPLY) {
-return true;
-} else if (flow->nw_proto == ARP_OP_REQUEST) {
-memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
-memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
-
-return flow->nw_src == flow->nw_dst;
-} else {
-return false;
-}
-}
-
 /* Determines whether packets in 'flow' within 'xbridge' should be forwarded or
  * dropped.  Returns true if they may be forwarded, false if they should be
  * dropped.
@@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport 
*in_port,
 mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
 if (mac
 && mac_entry_get_port(xbridge->ml, mac) != in_xbundle->ofbundle
-&& (!is_gratuitous_arp(flow, ctx->wc)
+&& (!is_garp(flow, ctx->wc)
 || mac_entry_is_grat_arp_locked(mac))) {
 ovs_rwlock_unlock(>ml->rwlock);
 xlate_report(ctx, OFT_DETAIL,
@@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
 }

 /* Learn source MAC. */
-bool is_grat_arp = is_gratuitous_arp(flow, wc);
+bool is_grat_arp = is_garp(flow, wc);
 if (ctx->xin->allow_side_effects
 && flow->packet_type == htonl(PT_ETH)
 && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
--
2.27.0




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

[ovs-dev] [PATCH v4] ofproto-dpif-xlate: Remove repeated function for judge garp

2023-02-17 Thread Han Ding 
Function is_gratuitous_arp() and function is_garp() are all used to judge
whether the flow is gratuitous arp. It is not necessary to use two functions
to do the same thing and just keep one.

Gratuitous ARP message is a generally link-level broadcast messages and
carry the same IP in sender and target fields. This patch add the check
in function is_garp() whether the arp is a broadcast message and whether
the arp is an arp request or an arp reply.

Signed-off-by: Han Ding 
---

Notes:
v2:
- Add the check whether the ARP packet is a broadcast message in is_garp().

v3:
- Add the check whether the packet is an arp request or an arp reply in 
is_garp().

v4:
- Add description of differences between patch versions
- Use WC_MASK_FIELD() macro instead of memset.

 lib/flow.h   | 19 +--
 ofproto/ofproto-dpif-xlate.c | 32 ++--
 2 files changed, 19 insertions(+), 32 deletions(-)

diff --git a/lib/flow.h b/lib/flow.h
index c647ad83c..9294a1b24 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -1133,8 +1133,23 @@ static inline bool is_garp(const struct flow *flow,
struct flow_wildcards *wc)
 {
 if (is_arp(flow)) {
-return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
-FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
+if (wc) {
+WC_MASK_FIELD(wc, dl_dst);
+}
+
+if (!eth_addr_is_broadcast(flow->dl_dst)) {
+return false;
+}
+
+if (wc) {
+WC_MASK_FIELD(wc, nw_proto);
+}
+
+if (flow->nw_proto == ARP_OP_REQUEST ||
+flow->nw_proto == ARP_OP_REPLY) {
+return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
+FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
+}
 }

 return false;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a9cf3cbee..b3c13f6bf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct 
xbundle *out_xbundle,
 memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
 }

-/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
- * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies to
- * indicate this; newer upstream kernels use gratuitous ARP requests. */
-static bool
-is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
-{
-if (flow->dl_type != htons(ETH_TYPE_ARP)) {
-return false;
-}
-
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-if (!eth_addr_is_broadcast(flow->dl_dst)) {
-return false;
-}
-
-memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-if (flow->nw_proto == ARP_OP_REPLY) {
-return true;
-} else if (flow->nw_proto == ARP_OP_REQUEST) {
-memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
-memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
-
-return flow->nw_src == flow->nw_dst;
-} else {
-return false;
-}
-}
-
 /* Determines whether packets in 'flow' within 'xbridge' should be forwarded or
  * dropped.  Returns true if they may be forwarded, false if they should be
  * dropped.
@@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport 
*in_port,
 mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
 if (mac
 && mac_entry_get_port(xbridge->ml, mac) != in_xbundle->ofbundle
-&& (!is_gratuitous_arp(flow, ctx->wc)
+&& (!is_garp(flow, ctx->wc)
 || mac_entry_is_grat_arp_locked(mac))) {
 ovs_rwlock_unlock(>ml->rwlock);
 xlate_report(ctx, OFT_DETAIL,
@@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
 }

 /* Learn source MAC. */
-bool is_grat_arp = is_gratuitous_arp(flow, wc);
+bool is_grat_arp = is_garp(flow, wc);
 if (ctx->xin->allow_side_effects
 && flow->packet_type == htonl(PT_ETH)
 && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
--
2.27.0




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

Re: [ovs-dev] [PATCH v3] ofproto-dpif-xlate: Remove repeated function for judge garp

2023-02-07 Thread Han Ding 


>On 2/7/23 06:04, Han Ding wrote:
>> 
>> Function is_gratuitous_arp() and function is_garp() are all used to judge
>> whether the flow is gratuitous arp. It is not necessary to use two functions
>> to do the same thing and just keep one.
>> 
>> Signed-off-by: Han Ding 
>> ---
>>  lib/flow.h   | 15 +--
>>  ofproto/ofproto-dpif-xlate.c | 32 ++--
>>  2 files changed, 15 insertions(+), 32 deletions(-)
>> 
>> diff --git a/lib/flow.h b/lib/flow.h
>> index c647ad83c..5b37543e0 100644
>> --- a/lib/flow.h
>> +++ b/lib/flow.h
>> @@ -1133,8 +1133,19 @@ static inline bool is_garp(const struct flow *flow,
>> struct flow_wildcards *wc)
>>  {
>>  if (is_arp(flow)) {
>> -    return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
>> -    FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
>> +    if (!eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, 
>> dl_dst))) {
>> +    return false;
>> +    }
>> +
>> +    if (wc) {
>> +    memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>
>WC_MASK_FIELD() macro should be used instead of memset.

Thanks for review. I will fix it in next version.
>
>> +    }
>> +
>> +    if (flow->nw_proto == ARP_OP_REQUEST ||
>> +    flow->nw_proto == ARP_OP_REPLY) {
>> +    return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
>> +    FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
>> +    }
>>  }
>> 
>>  return false;
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index a9cf3cbee..b3c13f6bf 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct 
>> xbundle *out_xbundle,
>>  memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
>>  }
>> 
>> -/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
>> - * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies 
>> to
>> - * indicate this; newer upstream kernels use gratuitous ARP requests. */
>> -static bool
>> -is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
>> -{
>> -    if (flow->dl_type != htons(ETH_TYPE_ARP)) {
>> -    return false;
>> -    }
>> -
>> -    memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
>> -    if (!eth_addr_is_broadcast(flow->dl_dst)) {
>> -    return false;
>> -    }
>> -
>> -    memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>> -    if (flow->nw_proto == ARP_OP_REPLY) {
>> -    return true;
>> -    } else if (flow->nw_proto == ARP_OP_REQUEST) {
>> -    memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
>> -    memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
>> -
>> -    return flow->nw_src == flow->nw_dst;
>> -    } else {
>> -    return false;
>> -    }
>> -}
>> -
>>  /* Determines whether packets in 'flow' within 'xbridge' should be 
>>forwarded or
>>   * dropped.  Returns true if they may be forwarded, false if they should be
>>   * dropped.
>> @@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport 
>> *in_port,
>>  mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
>>  if (mac
>>  && mac_entry_get_port(xbridge->ml, mac) != 
>>in_xbundle->ofbundle
>> -    && (!is_gratuitous_arp(flow, ctx->wc)
>> +    && (!is_garp(flow, ctx->wc)
>>  || mac_entry_is_grat_arp_locked(mac))) {
>>  ovs_rwlock_unlock(>ml->rwlock);
>>  xlate_report(ctx, OFT_DETAIL,
>> @@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
>>  }
>> 
>>  /* Learn source MAC. */
>> -    bool is_grat_arp = is_gratuitous_arp(flow, wc);
>> +    bool is_grat_arp = is_garp(flow, wc);
>>  if (ctx->xin->allow_side_effects
>>  && flow->packet_type == htonl(PT_ETH)
>>  && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
>> --
>> 2.27.0
>


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

Re: [ovs-dev] [PATCH v3] ofproto-dpif-xlate: Remove repeated function for judge garp

2023-02-07 Thread Han Ding 


>On Tue, Feb 07, 2023 at 01:04:41PM +0800, Han Ding wrote:
>> 
>> Function is_gratuitous_arp() and function is_garp() are all used to judge
>> whether the flow is gratuitous arp. It is not necessary to use two functions
>> to do the same thing and just keep one.
>> 
>> Signed-off-by: Han Ding 
>> ---
>>  lib/flow.h   | 15 +--
>>  ofproto/ofproto-dpif-xlate.c | 32 ++--
>>  2 files changed, 15 insertions(+), 32 deletions(-)
>
>Hi,
>
>it would be helpful if you included some text here describing the
>differences between v3 and v2, and in turn, v2 and v1.


Thanks, I will add the description of differences between different versions in 
next version.


>
>I also think that this change implies many considerations, as per the
>discussion of v2 [1]. And I think it would be worth summarising those in
>the patch description - hunting down historical information when reviewing
>v2 was an involved task, for me at least. And inevitably this topic will be
>revisited in future. So it would be nice to be kind to our future selves.
>
>I would be happy to help draft some text if that helps.

It's very helpful to me. Thank you very much!


>
>[1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/400803.html
>
>As for the code changes, they look good to me :)
>
>> 
>> diff --git a/lib/flow.h b/lib/flow.h
>> index c647ad83c..5b37543e0 100644
>> --- a/lib/flow.h
>> +++ b/lib/flow.h
>> @@ -1133,8 +1133,19 @@ static inline bool is_garp(const struct flow *flow,
>> struct flow_wildcards *wc)
>>  {
>>  if (is_arp(flow)) {
>> -    return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
>> -    FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
>> +    if (!eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, 
>> dl_dst))) {
>> +    return false;
>> +    }
>> +
>> +    if (wc) {
>> +    memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>> +    }
>> +
>> +    if (flow->nw_proto == ARP_OP_REQUEST ||
>> +    flow->nw_proto == ARP_OP_REPLY) {
>> +    return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
>> +    FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
>> +    }
>>  }
>> 
>>  return false;
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index a9cf3cbee..b3c13f6bf 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct 
>> xbundle *out_xbundle,
>>  memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
>>  }
>> 
>> -/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
>> - * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies 
>> to
>> - * indicate this; newer upstream kernels use gratuitous ARP requests. */
>> -static bool
>> -is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
>> -{
>> -    if (flow->dl_type != htons(ETH_TYPE_ARP)) {
>> -    return false;
>> -    }
>> -
>> -    memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
>> -    if (!eth_addr_is_broadcast(flow->dl_dst)) {
>> -    return false;
>> -    }
>> -
>> -    memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>> -    if (flow->nw_proto == ARP_OP_REPLY) {
>> -    return true;
>> -    } else if (flow->nw_proto == ARP_OP_REQUEST) {
>> -    memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
>> -    memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
>> -
>> -    return flow->nw_src == flow->nw_dst;
>> -    } else {
>> -    return false;
>> -    }
>> -}
>> -
>>  /* Determines whether packets in 'flow' within 'xbridge' should be 
>>forwarded or
>>   * dropped.  Returns true if they may be forwarded, false if they should be
>>   * dropped.
>> @@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport 
>> *in_port,
>>  mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
>>  if (mac
>>  && mac_entry_get_port(xbridge->ml, mac) != 
>>in_xbundle->ofbundle
>> -    && (!is_gratuitous_arp(flow, ctx->wc)
>> +    && (!is_garp(flow, ctx->wc)
>> 

[ovs-dev] [PATCH v3] ofproto-dpif-xlate: Remove repeated function for judge garp

2023-02-06 Thread Han Ding 


Function is_gratuitous_arp() and function is_garp() are all used to judge
whether the flow is gratuitous arp. It is not necessary to use two functions
to do the same thing and just keep one.

Signed-off-by: Han Ding 
---
 lib/flow.h   | 15 +--
 ofproto/ofproto-dpif-xlate.c | 32 ++--
 2 files changed, 15 insertions(+), 32 deletions(-)

diff --git a/lib/flow.h b/lib/flow.h
index c647ad83c..5b37543e0 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -1133,8 +1133,19 @@ static inline bool is_garp(const struct flow *flow,
struct flow_wildcards *wc)
 {
 if (is_arp(flow)) {
-return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
-FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
+if (!eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, dl_dst))) 
{
+return false;
+}
+
+if (wc) {
+memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+}
+
+if (flow->nw_proto == ARP_OP_REQUEST ||
+flow->nw_proto == ARP_OP_REPLY) {
+return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
+FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
+}
 }

 return false;
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a9cf3cbee..b3c13f6bf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct 
xbundle *out_xbundle,
 memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
 }

-/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
- * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies to
- * indicate this; newer upstream kernels use gratuitous ARP requests. */
-static bool
-is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
-{
-if (flow->dl_type != htons(ETH_TYPE_ARP)) {
-return false;
-}
-
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-if (!eth_addr_is_broadcast(flow->dl_dst)) {
-return false;
-}
-
-memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-if (flow->nw_proto == ARP_OP_REPLY) {
-return true;
-} else if (flow->nw_proto == ARP_OP_REQUEST) {
-memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
-memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
-
-return flow->nw_src == flow->nw_dst;
-} else {
-return false;
-}
-}
-
 /* Determines whether packets in 'flow' within 'xbridge' should be forwarded or
  * dropped.  Returns true if they may be forwarded, false if they should be
  * dropped.
@@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport 
*in_port,
 mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
 if (mac
 && mac_entry_get_port(xbridge->ml, mac) != in_xbundle->ofbundle
-&& (!is_gratuitous_arp(flow, ctx->wc)
+&& (!is_garp(flow, ctx->wc)
 || mac_entry_is_grat_arp_locked(mac))) {
 ovs_rwlock_unlock(>ml->rwlock);
 xlate_report(ctx, OFT_DETAIL,
@@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
 }

 /* Learn source MAC. */
-bool is_grat_arp = is_gratuitous_arp(flow, wc);
+bool is_grat_arp = is_garp(flow, wc);
 if (ctx->xin->allow_side_effects
 && flow->packet_type == htonl(PT_ETH)
 && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
--
2.27.0




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

Re: [ovs-dev] [PATCH v2] ofproto-dpif-xlate: Remove repeated function for judge garp

2023-01-28 Thread Han Ding 



>On Tue, Jan 24, 2023 at 12:22:43PM +0100, Ilya Maximets wrote:
>
>...
>
>> I did some research that might answer or maybe clarify the questions above.
>> Namely reading ARP-related RFCs - 826 and 5227.  From these I carried
>> following:
>> 
>>  - Gratuitous ARP / ARP Announcement messages are generally link-level
>>    broadcast messages.  I didn't find any mentions of unicast GARP.
>>    This means that checking for a broadcast should generally be fine.
>> 
>>  - Gratuitous ARP messages carry the same IP in sender and target fields.
>> 
>>  - The opcode in the ARP header mostly doesn't matter.  Its only purpose
>>    is to tell the receiver that it has to generate a reply if the opcode
>>    is 'request'.  RFC 826 makes it clear that the opcode should be the
>>    absolutely last thing that is checked in the packet, and this is done
>>    after the sender information is already put into the table, i.e after
>>    the whole processing is done.  So, checking the opcode should be
>>    unnecessary and can even be incorrect in our case.
>> 
>>  - It's typical for ARP replies to be unicast.  However, broadcast replies
>>    are not prohibited and can be used in some network setups, e.g. for
>>    faster collision detection.
>>    Saying that, it seems that is_gratuitous_arp() function is not really
>>    correct, as it assumes that all broadcast ARP replies are gratuitous.
>
>Yes. That seems to date back to is_gratuitous ARP being called
>is_bcast_arp_reply(). Which seems to date back to the beginning
>of (current) git history.
>
>- Import from old repository commit
>  https://github.com/openvswitch/ovs/commit/064af42167bf4
>
>The transition to being called is_gratuitous() occurred in
>
>- vswitchd: Treat gratuitous ARP requests like gratuitous ARP replies.
>  https://github.com/openvswitch/ovs/commit/5d0ae1387c96
>
>Where there was some discussion of the Citrix (actually Xen.org) topic.
>But only to the extent that the ARP reply portion was ok,
>not what Citrix was doing in detail.
>
>Fwiiw, the relevant kernel discussion is here. And it seems
>the reply code matches the kernel implementation (and common
>interpretation of gratuitous ARP, based on that conversation):
>
>- [PATCH 0/2] fixes to arp_notify for virtual machine migration use case
>  
> https://lore.kernel.org/netdev/1273671554.7572.11190.ca...@zakaz.uk.xensource.com/
>
>>    However, I don't know how Citrix-patched Linux DomU ARP replies looked
>>    like.  But I tend to assume that they did have the same IP as sender
>>    and a target, so this check should be sufficient?  i.e. as long as we
>>    are comparing IPs without looking at the opcode, we should be fine.
>
>I don't know either. But perhaps this, assuming that Xen means Xen.org
>code, seems to indicate that they are broadcast replies with
>the same sender and target address.
>
>https://bugzilla.redhat.com/show_bug.cgi?id=573579#c13
>
>> Looking into this, the patch might be OK.  We might not even check for
>> an address to be broadcast as in v1, but I'm not sure about this.
>> Checking for a broadcast may save us from unwildcarding too many fields.
>
>Yes I think so too.
>
>Changing the any broadcast ARP reply is gratuitous behaviour of
>is_gratuitous_arp() could hurt us. Who knows who is relying on that?
>But on the balance, it does seem incorrect.
>
>I think I would be slightly happier if there
>was a check for ARP_OP_REPLY || ARP_OP_REQUEST,
>but perhaps they are the only two possible options anyway.
>

Thanks for the review. 

I think you are right, it is necessary to check the arp_op. 
The kernel code arp_process() also check the arp_op is request or reply first 
and then do arp_is_garp().

I will modify the patch in next version.


>> Thoughts?  Some fact-checking of above would be great.
>
>I'd be included to accept this patch.
>But it is not without risk


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

[ovs-dev] [PATCH v2] ofproto-dpif-xlate: Remove repeated function for judge garp

2023-01-04 Thread Han Ding 
Function is_gratuitous_arp() and function is_garp() are all used to judge
whether the flow is gratuitous arp. It is not necessary to use two functions
to do the same thing and just keep one.

Acked-by: Ilya Maximets
Signed-off-by: Han Ding 
---
 lib/flow.h   |  3 ++-
 ofproto/ofproto-dpif-xlate.c | 32 ++--
 2 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/lib/flow.h b/lib/flow.h
index c647ad83c..ade64b52d 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -1132,7 +1132,8 @@ static inline bool is_arp(const struct flow *flow)
 static inline bool is_garp(const struct flow *flow,
struct flow_wildcards *wc)
 {
-if (is_arp(flow)) {
+if (is_arp(flow) &&
+eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, dl_dst))) {
 return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
 FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
 }
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a9cf3cbee..b3c13f6bf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct 
xbundle *out_xbundle,
 memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
 }

-/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
- * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies to
- * indicate this; newer upstream kernels use gratuitous ARP requests. */
-static bool
-is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
-{
-if (flow->dl_type != htons(ETH_TYPE_ARP)) {
-return false;
-}
-
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-if (!eth_addr_is_broadcast(flow->dl_dst)) {
-return false;
-}
-
-memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-if (flow->nw_proto == ARP_OP_REPLY) {
-return true;
-} else if (flow->nw_proto == ARP_OP_REQUEST) {
-memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
-memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
-
-return flow->nw_src == flow->nw_dst;
-} else {
-return false;
-}
-}
-
 /* Determines whether packets in 'flow' within 'xbridge' should be forwarded or
  * dropped.  Returns true if they may be forwarded, false if they should be
  * dropped.
@@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport 
*in_port,
 mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
 if (mac
 && mac_entry_get_port(xbridge->ml, mac) != in_xbundle->ofbundle
-&& (!is_gratuitous_arp(flow, ctx->wc)
+&& (!is_garp(flow, ctx->wc)
 || mac_entry_is_grat_arp_locked(mac))) {
 ovs_rwlock_unlock(>ml->rwlock);
 xlate_report(ctx, OFT_DETAIL,
@@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
 }

 /* Learn source MAC. */
-bool is_grat_arp = is_gratuitous_arp(flow, wc);
+bool is_grat_arp = is_garp(flow, wc);
 if (ctx->xin->allow_side_effects
 && flow->packet_type == htonl(PT_ETH)
 && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
--
2.27.0



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

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Remove repeated function for judge garp.

2022-12-11 Thread Han Ding 



--



Han Ding
>On 12/1/22 07:39, Han Ding wrote:
>> 
>> Function is_gratuitous_arp() and function is_garp() are all used to judge
>> whether the flow is gratuitous arp. It is not necessary to use two functions
>> to do the same thing and just keep one.
>
>Hi.  Thanks for the patch!
>
>These function are indeed similar, but they are not identical.  It looks
>like is_gratuitous_arp() is handling arp replies along with arp requests,
>it also checks for the packet to be broadcast.  is_garp() doesn't do that.
>
>In case we want to keep only one implementation, we should probably keep
>the more robust version, which is is_gratuitous_arp().
>
>What do you think?
>
>Best regards, Ilya Maximets.
>
Thanks for reviewing this patch.  Now I think there are some defects all in 
is_gratuitous_arp() 
and is_garp(). Gratuitous arp is a special arp packet where the source and 
destination IP 
are both set to the same IP and the destination MAC is the broadcast address 
ff:ff:ff:ff:ff:ff. 
Function is_gratuitous_arp() check the broadcast, and whether the src ip and 
dst ip are same. But 
when the packet is arp reply, the function don't check whether the src ip and 
dst ip are same. 
The function is_garp check the arp request and arp reply and whether the src ip 
and dst ip are same.
But it doesn't check for the packet to be broadcast. 

So, I think we should probably keep is_garp() and modify it. Because is_garp() 
is a inline function in flow.h.
But is_gratuitous_arp() is a static function defined in ofproto-dpif-xlate.c.

Did I miss something ?

Best regards, Han Ding. 


>> 
>> Signed-off-by: Han Ding 
>> ---
>>  ofproto/ofproto-dpif-xlate.c | 32 ++--
>>  1 file changed, 2 insertions(+), 30 deletions(-)
>> 
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index a9cf3cbee..b3c13f6bf 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct 
>> xbundle *out_xbundle,
>>  memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
>>  }
>> 
>> -/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
>> - * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies 
>> to
>> - * indicate this; newer upstream kernels use gratuitous ARP requests. */
>> -static bool
>> -is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
>> -{
>> -    if (flow->dl_type != htons(ETH_TYPE_ARP)) {
>> -    return false;
>> -    }
>> -
>> -    memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
>> -    if (!eth_addr_is_broadcast(flow->dl_dst)) {
>> -    return false;
>> -    }
>> -
>> -    memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>> -    if (flow->nw_proto == ARP_OP_REPLY) {
>> -    return true;
>> -    } else if (flow->nw_proto == ARP_OP_REQUEST) {
>> -    memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
>> -    memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
>> -
>> -    return flow->nw_src == flow->nw_dst;
>> -    } else {
>> -    return false;
>> -    }
>> -}
>> -
>>  /* Determines whether packets in 'flow' within 'xbridge' should be 
>>forwarded or
>>   * dropped.  Returns true if they may be forwarded, false if they should be
>>   * dropped.
>> @@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport 
>> *in_port,
>>  mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
>>  if (mac
>>  && mac_entry_get_port(xbridge->ml, mac) != 
>>in_xbundle->ofbundle
>> -    && (!is_gratuitous_arp(flow, ctx->wc)
>> +    && (!is_garp(flow, ctx->wc)
>>  || mac_entry_is_grat_arp_locked(mac))) {
>>  ovs_rwlock_unlock(>ml->rwlock);
>>  xlate_report(ctx, OFT_DETAIL,
>> @@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
>>  }
>> 
>>  /* Learn source MAC. */
>> -    bool is_grat_arp = is_gratuitous_arp(flow, wc);
>> +    bool is_grat_arp = is_garp(flow, wc);
>>  if (ctx->xin->allow_side_effects
>>  && flow->packet_type == htonl(PT_ETH)
>>  && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
>> --
>> 2.27.0
>


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

[ovs-dev] [PATCH] ofproto-dpif-xlate: Remove repeated function for judge garp.

2022-11-30 Thread Han Ding 


Function is_gratuitous_arp() and function is_garp() are all used to judge
whether the flow is gratuitous arp. It is not necessary to use two functions
to do the same thing and just keep one.

Signed-off-by: Han Ding 
---
 ofproto/ofproto-dpif-xlate.c | 32 ++--
 1 file changed, 2 insertions(+), 30 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a9cf3cbee..b3c13f6bf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct 
xbundle *out_xbundle,
 memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
 }

-/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
- * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies to
- * indicate this; newer upstream kernels use gratuitous ARP requests. */
-static bool
-is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
-{
-if (flow->dl_type != htons(ETH_TYPE_ARP)) {
-return false;
-}
-
-memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-if (!eth_addr_is_broadcast(flow->dl_dst)) {
-return false;
-}
-
-memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
-if (flow->nw_proto == ARP_OP_REPLY) {
-return true;
-} else if (flow->nw_proto == ARP_OP_REQUEST) {
-memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
-memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
-
-return flow->nw_src == flow->nw_dst;
-} else {
-return false;
-}
-}
-
 /* Determines whether packets in 'flow' within 'xbridge' should be forwarded or
  * dropped.  Returns true if they may be forwarded, false if they should be
  * dropped.
@@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport 
*in_port,
 mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan);
 if (mac
 && mac_entry_get_port(xbridge->ml, mac) != in_xbundle->ofbundle
-&& (!is_gratuitous_arp(flow, ctx->wc)
+&& (!is_garp(flow, ctx->wc)
 || mac_entry_is_grat_arp_locked(mac))) {
 ovs_rwlock_unlock(>ml->rwlock);
 xlate_report(ctx, OFT_DETAIL,
@@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx)
 }

 /* Learn source MAC. */
-bool is_grat_arp = is_gratuitous_arp(flow, wc);
+bool is_grat_arp = is_garp(flow, wc);
 if (ctx->xin->allow_side_effects
 && flow->packet_type == htonl(PT_ETH)
 && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
--
2.27.0




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

[ovs-dev] [PATCH v4] ofproto-dpif-xlate: Update tunnel neighbor when receive gratuitous arp.

2022-10-19 Thread Han Ding 
OVS now just allow the ARP Relpy whitch the destination address is matched
against the known xbridge addresses to udpate tunnel neighbor. So when ovs
receive the gratuitous ARP from underlay gateway which the source address
and destination address are all gateway IP, tunnel neighbor will not be updated.

Fixes: ba07cf222a0c ("Handle gratuitous ARP requests and replies in 
tnl_arp_snoop()")
Fixes: 83c2757bd16e ("xlate: Move tnl_neigh_snoop() to 
terminate_native_tunnel()")
Acked-by: Paolo Valerio
Signed-off-by: Han Ding 
---
 ofproto/ofproto-dpif-xlate.c | 16 +---
 tests/tunnel-push-pop.at | 20 
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 3b9b26da1..2e6788ea8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4172,6 +4172,17 @@ xport_has_ip(const struct xport *xport)
 return n_in6 ? true : false;
 }

+static bool check_neighbor_reply(struct xlate_ctx *ctx, struct flow *flow)
+{
+if (flow->dl_type == htons(ETH_TYPE_ARP) ||
+flow->nw_proto == IPPROTO_ICMPV6)
+{
+return is_neighbor_reply_correct(ctx, flow);
+}
+
+return false;
+}
+
 static bool
 terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport,
 struct flow *flow, struct flow_wildcards *wc,
@@ -4192,9 +4203,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const 
struct xport *xport,
 /* If no tunnel port was found and it's about an ARP or ICMPv6 packet,
  * do tunnel neighbor snooping. */
 if (*tnl_port == ODPP_NONE &&
-(flow->dl_type == htons(ETH_TYPE_ARP) ||
- flow->nw_proto == IPPROTO_ICMPV6) &&
- is_neighbor_reply_correct(ctx, flow)) {
+(check_neighbor_reply(ctx, flow) ||
+ is_garp(flow, wc))) {
 tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
 ctx->xin->allow_side_effects);
 } else if (*tnl_port != ODPP_NONE &&
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index 92eebba2e..013ecbcaa 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], 
[0], [dnl
 1.1.2.92  f8:bc:12:44:34:b6   br0
 ])

+dnl Receiving Gratuitous ARP request with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:c8   br0
+])
+
+dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:34:b2,tha=f8:bc:12:44:34:b2))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:b2   br0
+])
+
 dnl Receive ARP reply without VLAN header
 AT_CHECK([ovs-vsctl set port br0 tag=0])
 AT_CHECK([ovs-appctl tnl/neigh/flush], [0], [OK
--
2.27.0




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

[ovs-dev] [PATCH v4] ofproto-dpif-xlate: Update tunnel neighbor when receive garp

2022-10-12 Thread Han Ding 
OVS now just allow the ARP Relpy whitch the destination address is matched
against the known xbridge addresses to udpate tunnel neighbor. So when ovs
receive the gratuitous ARP from underlay gateway which the source address
and destination address are all gateway IP, tunnel neighbor will not be updated.

Fixes: ba07cf222a0c ("Handle gratuitous ARP requests and replies in 
tnl_arp_snoop()")
Fixes: 83c2757bd16e ("xlate: Move tnl_neigh_snoop() to 
terminate_native_tunnel()")
Acked-by: Paolo Valerio
Signed-off-by: Han Ding 
---
 ofproto/ofproto-dpif-xlate.c | 15 +++
 tests/tunnel-push-pop.at | 20 
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 3b9b26da1..429cafe92 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4120,7 +4120,15 @@ is_neighbor_reply_correct(const struct xlate_ctx *ctx, 
const struct flow *flow)
 {
 bool ret = false;
 int i;
-struct xbridge_addr *xbridge_addr = xbridge_addr_ref(ctx->xbridge->addr);
+struct xbridge_addr *xbridge_addr = NULL;
+
+if (flow->dl_type != htons(ETH_TYPE_ARP) &&
+flow->nw_proto != IPPROTO_ICMPV6)
+{
+return false;
+}
+
+xbridge_addr = xbridge_addr_ref(ctx->xbridge->addr);

 /* Verify if 'nw_dst' of ARP or 'ipv6_dst' of ICMPV6 is in the list. */
 for (i = 0; xbridge_addr && i < xbridge_addr->n_addr; i++) {
@@ -4192,9 +4200,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const 
struct xport *xport,
 /* If no tunnel port was found and it's about an ARP or ICMPv6 packet,
  * do tunnel neighbor snooping. */
 if (*tnl_port == ODPP_NONE &&
-(flow->dl_type == htons(ETH_TYPE_ARP) ||
- flow->nw_proto == IPPROTO_ICMPV6) &&
- is_neighbor_reply_correct(ctx, flow)) {
+(is_neighbor_reply_correct(ctx, flow) ||
+ is_garp(flow, wc))) {
 tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
 ctx->xin->allow_side_effects);
 } else if (*tnl_port != ODPP_NONE &&
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index 92eebba2e..013ecbcaa 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], 
[0], [dnl
 1.1.2.92  f8:bc:12:44:34:b6   br0
 ])

+dnl Receiving Gratuitous ARP request with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:c8   br0
+])
+
+dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:34:b2,tha=f8:bc:12:44:34:b2))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:b2   br0
+])
+
 dnl Receive ARP reply without VLAN header
 AT_CHECK([ovs-vsctl set port br0 tag=0])
 AT_CHECK([ovs-appctl tnl/neigh/flush], [0], [OK
--
2.27.0




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

Re: [ovs-dev] [PATCH v3] ofproto-dpif-xlate: Update tunnel neighbor when receive gratuitous arp.

2022-10-10 Thread Han Ding 
Thanks for reviewing. I will change them in next version.


--



Han Ding



>Hello Han,
>
>"Han Ding"  writes:
>
>> Commit ba07cf222a add the feature "Handle gratuitous ARP requests and
>> replies in tnl_arp_snoop()". But commit 83c2757bd1 just allow the ARP whitch
>> the destination address of the ARP is matched against the known xbridge 
>> addresses.
>> So the modification of commit ba07cf222a is not effective. When ovs receive 
>> the
>> gratuitous ARP from underlay gateway which the source address and destination
>> address are all gateway IP, tunnel neighbor will not be updated.
>>
>
>I think it would be clearer formatting the commits like below:
>
>$ git -P show -s --format="%h (\"%s\")" --abbrev=12 ba07cf222a
>ba07cf222a0c ("Handle gratuitous ARP requests and replies in tnl_arp_snoop()")
>
>$ git -P show -s --format="%h (\"%s\")" --abbrev=12 83c2757bd1
>83c2757bd16e ("xlate: Move tnl_neigh_snoop() to terminate_native_tunnel()")
>
>I guess that the last commit deserves a Fixes tag as well.
>
>> Signed-off-by: Han Ding 
>> ---
>>
>> Notes:
>> v3
>> Correct the spell mistake.
>>
>> v2
>> Change author name.  
>>
>>  ofproto/ofproto-dpif-xlate.c | 10 +++---
>>  tests/tunnel-push-pop.at | 20 
>>  2 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 8e5d030ac..6c69f981b 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -4126,6 +4126,11 @@ xport_has_ip(const struct xport *xport)
>>  return n_in6 ? true : false;
>>  }
>>
>> +#define IS_VALID_NEIGHBOR_REPLY(flow, ctx) \
>> +((flow->dl_type == htons(ETH_TYPE_ARP) || \
>> +  flow->nw_proto == IPPROTO_ICMPV6) && \
>> + is_neighbor_reply_correct(ctx, flow))
>> +
>
>Although terminate_native_tunnel() would be the only user, I guess a
>static function could be ok here, instead.
>
>>  static bool
>>  terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport,
>>  struct flow *flow, struct flow_wildcards *wc,
>> @@ -4146,9 +4151,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const 
>> struct xport *xport,
>>  /* If no tunnel port was found and it's about an ARP or ICMPv6 
>>packet,
>>   * do tunnel neighbor snooping. */
>>  if (*tnl_port == ODPP_NONE &&
>> -    (flow->dl_type == htons(ETH_TYPE_ARP) ||
>> - flow->nw_proto == IPPROTO_ICMPV6) &&
>> - is_neighbor_reply_correct(ctx, flow)) {
>> +    (IS_VALID_NEIGHBOR_REPLY(flow, ctx) ||
>> + is_garp(flow, wc))) {
>
>AFAICT, this seems ok to me and the tests related to tunnel_push_pop
>succeed. There's probably some room for improvement in the code down to
>tnl_arp_snoop(), but I guess it's a bit out of scope of this patch.
>
>>  tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
>>  ctx->xin->allow_side_effects);
>>  } else if (*tnl_port != ODPP_NONE &&
>> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
>> index c63344196..0bac362f4 100644
>> --- a/tests/tunnel-push-pop.at
>> +++ b/tests/tunnel-push-pop.at
>> @@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], 
>> [0], [dnl
>>  1.1.2.92  f8:bc:12:44:34:b6   br0
>>  ])
>>
>> +dnl Receiving Gratuitous ARP request with correct VLAN id should alter 
>> tunnel neighbor cache
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 
>> 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))'])
>> +
>> +ovs-appctl time/warp 1000
>> +ovs-appctl time/warp 1000
>> +
>> +AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
>> +1.1.2.92  f8:bc:12:44:34:c8   br0
>> +])
>> +
>> +dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel 
>> neighbor cache
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 
>> 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:

[ovs-dev] [PATCH v1] ovs-save: Use right OpenFlow version for add-tlv-map

2022-07-01 Thread Han Ding 


When the bridge protocols is not included Openflow10, printing an error
message "version negotiation failed" when doing "Restoring saved flows".

Signed-off-by: Han Ding 
---
 utilities/ovs-save | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/ovs-save b/utilities/ovs-save
index a190902f4..67092ecf7 100755
--- a/utilities/ovs-save
+++ b/utilities/ovs-save
@@ -127,7 +127,7 @@ save_flows () {
 # Get the highest enabled OpenFlow version
 ofp_version=$(get_highest_ofp_version "$bridge")

-printf "%s" "ovs-ofctl add-tlv-map ${bridge} '"
+printf "%s" "ovs-ofctl -O $ofp_version add-tlv-map ${bridge} '"
 ovs-ofctl dump-tlv-map ${bridge} -O $ofp_version | \
 awk '/^  *0x/ {if (cnt != 0) printf ","; \
  cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}'
--
2.27.0




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

[ovs-dev] [PATCH v3] ofproto-dpif-xlate: Update tunnel neighbor when receive gratuitous arp.

2022-06-18 Thread Han Ding 
Commit ba07cf222a add the feature "Handle gratuitous ARP requests and
replies in tnl_arp_snoop()". But commit 83c2757bd1 just allow the ARP whitch
the destination address of the ARP is matched against the known xbridge 
addresses.
So the modification of commit ba07cf222a is not effective. When ovs receive the
gratuitous ARP from underlay gateway which the source address and destination
address are all gateway IP, tunnel neighbor will not be updated.

Signed-off-by: Han Ding 
---

Notes:
v3
Correct the spell mistake.

v2
Change author name.  

 ofproto/ofproto-dpif-xlate.c | 10 +++---
 tests/tunnel-push-pop.at | 20 
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 8e5d030ac..6c69f981b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4126,6 +4126,11 @@ xport_has_ip(const struct xport *xport)
 return n_in6 ? true : false;
 }

+#define IS_VALID_NEIGHBOR_REPLY(flow, ctx) \
+((flow->dl_type == htons(ETH_TYPE_ARP) || \
+  flow->nw_proto == IPPROTO_ICMPV6) && \
+ is_neighbor_reply_correct(ctx, flow))
+
 static bool
 terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport,
 struct flow *flow, struct flow_wildcards *wc,
@@ -4146,9 +4151,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const 
struct xport *xport,
 /* If no tunnel port was found and it's about an ARP or ICMPv6 packet,
  * do tunnel neighbor snooping. */
 if (*tnl_port == ODPP_NONE &&
-(flow->dl_type == htons(ETH_TYPE_ARP) ||
- flow->nw_proto == IPPROTO_ICMPV6) &&
- is_neighbor_reply_correct(ctx, flow)) {
+(IS_VALID_NEIGHBOR_REPLY(flow, ctx) ||
+ is_garp(flow, wc))) {
 tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
 ctx->xin->allow_side_effects);
 } else if (*tnl_port != ODPP_NONE &&
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index c63344196..0bac362f4 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], 
[0], [dnl
 1.1.2.92  f8:bc:12:44:34:b6   br0
 ])

+dnl Receiving Gratuitous ARP request with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:c8   br0
+])
+
+dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:34:b2,tha=f8:bc:12:44:34:b2))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:b2   br0
+])
+
 dnl Receive ARP reply without VLAN header
 AT_CHECK([ovs-vsctl set port br0 tag=0])
 AT_CHECK([ovs-appctl tnl/neigh/flush], [0], [OK
--
2.27.0




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

[ovs-dev] [PATCH v2] ofproto-dpif-xlate: Update tunnel neighbor when receive gratuitous arp.

2022-06-17 Thread Han Ding 


Commit ba07cf222a add the feature "Handle gratuitous ARP requests and
replies in tnl_arp_snoop()". But commit 83c2757bd1 just allow the ARP whitch
the destination address of the ARP is matched against the known xbridge 
addresses.
So the modification of commit ba07cf222a is not effective. When ovs receive the
gratuitous ARP from underlay gateway whitch the source address and destination
address are all gateway IP, tunnel neighbor will not be updated.

Signed-off-by: Han Ding 
---

Notes:
Change author name.

 ofproto/ofproto-dpif-xlate.c | 10 +++---
 tests/tunnel-push-pop.at | 20 
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 8e5d030ac..6c69f981b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4126,6 +4126,11 @@ xport_has_ip(const struct xport *xport)
 return n_in6 ? true : false;
 }

+#define IS_VALID_NEIGHBOR_REPLY(flow, ctx) \
+((flow->dl_type == htons(ETH_TYPE_ARP) || \
+  flow->nw_proto == IPPROTO_ICMPV6) && \
+ is_neighbor_reply_correct(ctx, flow))
+
 static bool
 terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport,
 struct flow *flow, struct flow_wildcards *wc,
@@ -4146,9 +4151,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const 
struct xport *xport,
 /* If no tunnel port was found and it's about an ARP or ICMPv6 packet,
  * do tunnel neighbor snooping. */
 if (*tnl_port == ODPP_NONE &&
-(flow->dl_type == htons(ETH_TYPE_ARP) ||
- flow->nw_proto == IPPROTO_ICMPV6) &&
- is_neighbor_reply_correct(ctx, flow)) {
+(IS_VALID_NEIGHBOR_REPLY(flow, ctx) ||
+ is_garp(flow, wc))) {
 tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
 ctx->xin->allow_side_effects);
 } else if (*tnl_port != ODPP_NONE &&
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index c63344196..0bac362f4 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], 
[0], [dnl
 1.1.2.92  f8:bc:12:44:34:b6   br0
 ])

+dnl Receiving Gratuitous ARP request with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:c8   br0
+])
+
+dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:34:b2,tha=f8:bc:12:44:34:b2))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:b2   br0
+])
+
 dnl Receive ARP reply without VLAN header
 AT_CHECK([ovs-vsctl set port br0 tag=0])
 AT_CHECK([ovs-appctl tnl/neigh/flush], [0], [OK
--
2.27.0




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

[ovs-dev] [PATCH v1] ofproto-dpif-xlate: Update tunnel neighbor when receive gratuitous arp.

2022-06-17 Thread Han Ding 


Commit ba07cf222a add the feature "Handle gratuitous ARP requests and
replies in tnl_arp_snoop()". But commit 83c2757bd1 just allow the ARP whitch
the destination address of the ARP is matched against the known xbridge 
addresses.
So the modification of commit ba07cf222a is not effective. When ovs receive the
gratuitous ARP from underlay gateway whitch the source address and destination
address are all gateway IP, tunnel neighbor will not be updated.

Signed-off-by: handing 
---
 ofproto/ofproto-dpif-xlate.c | 10 +++---
 tests/tunnel-push-pop.at | 20 
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 8e5d030ac..6c69f981b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4126,6 +4126,11 @@ xport_has_ip(const struct xport *xport)
 return n_in6 ? true : false;
 }

+#define IS_VALID_NEIGHBOR_REPLY(flow, ctx) \
+((flow->dl_type == htons(ETH_TYPE_ARP) || \
+  flow->nw_proto == IPPROTO_ICMPV6) && \
+ is_neighbor_reply_correct(ctx, flow))
+
 static bool
 terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport,
 struct flow *flow, struct flow_wildcards *wc,
@@ -4146,9 +4151,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const 
struct xport *xport,
 /* If no tunnel port was found and it's about an ARP or ICMPv6 packet,
  * do tunnel neighbor snooping. */
 if (*tnl_port == ODPP_NONE &&
-(flow->dl_type == htons(ETH_TYPE_ARP) ||
- flow->nw_proto == IPPROTO_ICMPV6) &&
- is_neighbor_reply_correct(ctx, flow)) {
+(IS_VALID_NEIGHBOR_REPLY(flow, ctx) ||
+ is_garp(flow, wc))) {
 tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
 ctx->xin->allow_side_effects);
 } else if (*tnl_port != ODPP_NONE &&
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index c63344196..0bac362f4 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], 
[0], [dnl
 1.1.2.92  f8:bc:12:44:34:b6   br0
 ])

+dnl Receiving Gratuitous ARP request with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:c8   br0
+])
+
+dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel 
neighbor cache
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 
'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:34:b2,tha=f8:bc:12:44:34:b2))'])
+
+ovs-appctl time/warp 1000
+ovs-appctl time/warp 1000
+
+AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl
+1.1.2.92  f8:bc:12:44:34:b2   br0
+])
+
 dnl Receive ARP reply without VLAN header
 AT_CHECK([ovs-vsctl set port br0 tag=0])
 AT_CHECK([ovs-appctl tnl/neigh/flush], [0], [OK
--
2.27.0




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

[ovs-dev] [PATCH] ovs-tcpdump: Fix error when stopping ovs-tcpdump.

2022-05-20 Thread Han Ding 


Sometimes we need to dump packets on more than two interfaces in a bridge
at the same time. Then when we stop dumping in order, ovs-tcpdump print
traceback and fail to delete mirror interface for some interface.

For example:
br-int has two interface tap1 and br-int. We use ovs-tcpdump dump tap1 first
and dump br-int next. Then stopping tap1 ovs-tcpdump first, and stopping
br-int second. When we stop ovs-tcpdump for br-int, the screen show the error
like this:
__main__.OVSDBException: Unable to delete Mirror m_br-int

Signed-off-by: Han Ding 
---
 utilities/ovs-tcpdump.in | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 82d1bedfa..6188be5c4 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -164,6 +164,9 @@ class OVSDB(object):
 schema.register_all()
 self._idl_conn = idl.Idl(db_sock, schema)
 OVSDB.wait_for_db_change(self._idl_conn)  # Initial Sync with DB
+
+def close_idl(self):
+self._idl_conn.close()

 def _get_schema(self):
 error, strm = Stream.open_block(Stream.open(self._db_sock))
@@ -500,6 +503,8 @@ def main():
 pass
 sys.exit(1)

+ovsdb.close_idl()
+
 pipes = _doexec(*([dump_cmd, '-i', mirror_interface] + tcpdargs))
 try:
 while pipes.poll() is None:
@@ -512,6 +517,7 @@ def main():
 if pipes.poll() is None:
 pipes.terminate()

+ovsdb = OVSDB(db_sock)
 ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
 ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
 if tap_created is True:
--
2.27.0




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

[ovs-dev] [PATCH v2] ovs-save: Get highest ofp version error.

2022-05-11 Thread Han Ding 
When setting just one ofp version to protocols of bridge, The function
get_highest_ofp_version in ovs-save parse it error.

For example:
$ ovs-vsctl get bridge br-int protocols
[OpenFlow15]

$ ovs-vsctl get bridge br-int protocols |
  sed 's/[][]//g' | sed 's/\ //g' | awk -F ',' '{ print (NF>1)? $(NF) : 
"OpenFlow14" }'
OpenFlow14

Signed-off-by: Han Ding 
Acked-by: Adrian Moreno 
---

Notes:
Change the author name to full name.

 utilities/ovs-save | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/ovs-save b/utilities/ovs-save
index fb2025b76..a190902f4 100755
--- a/utilities/ovs-save
+++ b/utilities/ovs-save
@@ -102,7 +102,7 @@ save_interfaces () {
 get_highest_ofp_version() {
 ovs-vsctl get bridge "$1" protocols | \
 sed 's/[][]//g' | sed 's/\ //g' | \
-awk -F ',' '{ print (NF>1)? $(NF) : "OpenFlow14" }'
+awk -F ',' '{ print (NF>0)? $(NF) : "OpenFlow14" }'
 }

 save_flows () {
--
2.27.0




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

Re: [ovs-dev] [PATCH] ovs-save: Get highest ofp version error.

2022-05-10 Thread Han Ding 



--



Han Ding
>
>
>On 5/6/22 11:04, Adrian Moreno wrote:
>> Hello,
>> 
>> On 4/18/22 10:01, handing wrote:
>>>
>>> When setting just one ofp version to protocols of bridge, The function
>>> get_highest_ofp_version in ovs-save parse it error.
>>>
>>> For example:
>>> $ ovs-vsctl get bridge br-int protocols
>>> [OpenFlow15]
>>>
>>> $ ovs-vsctl get bridge br-int protocols |
>>>    sed 's/[][]//g' | sed 's/\ //g' | awk -F ',' '{ print (NF>1)? $(NF) : 
>>> "OpenFlow14" }'
>>> OpenFlow14
>>>
>>> Signed-off-by: handing 
>> 
>> I think we need the full name of the author in the Signed-off-by tag as per
>
>Sorry: unfinished sentence. My intention was to reference this doc: 
>https://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/

Thanks for reviewing. I will modify it in next version.


>> 
>>> ---
>>>   utilities/ovs-save | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/utilities/ovs-save b/utilities/ovs-save
>>> index fb2025b76..a190902f4 100755
>>> --- a/utilities/ovs-save
>>> +++ b/utilities/ovs-save
>>> @@ -102,7 +102,7 @@ save_interfaces () {
>>>   get_highest_ofp_version() {
>>>   ovs-vsctl get bridge "$1" protocols | \
>>>   sed 's/[][]//g' | sed 's/\ //g' | \
>>> -    awk -F ',' '{ print (NF>1)? $(NF) : "OpenFlow14" }'
>>> +    awk -F ',' '{ print (NF>0)? $(NF) : "OpenFlow14" }'
>>>   }
>>>
>>>   save_flows () {
>>> -- 
>>> 2.27.0
>>>
>>>
>> 
>> The change itself looks good to me. Apart from the formal comment above:
>> 
>> Acked-by: Adrian Moreno 
>> 
>
>-- 
>Adrián Moreno
>


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