| Message ID | 202301051415026716003@chinatelecom.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] ofproto-dpif-xlate: Remove repeated function for judge garp | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Hi Han Ding, On Thu, Jan 05, 2023 at 02:15:03PM +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. I agree it is nice to consolidate things. But, curiously, the logic of is_garp() and is_gratuitous_arp() are somewhat different. I wonder about the risk of regressions when consolidating them. > Acked-by: Ilya Maximets<i.maximets@ovn.org> > Signed-off-by: Han Ding <handing@chinatelecom.cn> > --- > 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))) { Are any users relying on is_garp() not enforcing the eth_addr_is_broadcast() check? > 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(&ctx->xin->flow.vlans, &old_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(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst); > - if (!eth_addr_is_broadcast(flow->dl_dst)) { > - return false; > - } > - > - memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); > - if (flow->nw_proto == ARP_OP_REPLY) { > - return true; This behaviour, which I assume correlates to the Citrix portion of the comment above, does not seem to be present in is_garp(). Is that a problem? > - } else if (flow->nw_proto == ARP_OP_REQUEST) { > - memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); > - memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst); > - > - return flow->nw_src == flow->nw_dst; > - } else { This behaviour does not seem to be present in is_garp(). Is that a problem? > - 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. ...
On 1/20/23 10:49, Simon Horman wrote: > Hi Han Ding, > > On Thu, Jan 05, 2023 at 02:15:03PM +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. > > I agree it is nice to consolidate things. > > But, curiously, the logic of is_garp() and is_gratuitous_arp() > are somewhat different. I wonder about the risk of regressions > when consolidating them. Yeah, I had the same concern for v1. > >> Acked-by: Ilya Maximets<i.maximets@ovn.org> I didn't ACK this patch. Please, don't add tags not explicitly given. >> Signed-off-by: Han Ding <handing@chinatelecom.cn> >> --- >> 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))) { > > Are any users relying on is_garp() not enforcing the > eth_addr_is_broadcast() check? > >> 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(&ctx->xin->flow.vlans, &old_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(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst); >> - if (!eth_addr_is_broadcast(flow->dl_dst)) { >> - return false; >> - } >> - >> - memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); >> - if (flow->nw_proto == ARP_OP_REPLY) { >> - return true; > > This behaviour, which I assume correlates to the Citrix portion > of the comment above, does not seem to be present in is_garp(). > > Is that a problem? > >> - } else if (flow->nw_proto == ARP_OP_REQUEST) { >> - memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); >> - memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst); >> - >> - return flow->nw_src == flow->nw_dst; >> - } else { > > This behaviour does not seem to be present in is_garp(). > > Is that a problem? 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. 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. 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. Thoughts? Some fact-checking of above would be great. Best regards, Ilya Maximets.
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.camel@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. > Thoughts? Some fact-checking of above would be great. I'd be included to accept this patch. But it is not without risk.
>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.camel@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
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(&ctx->xin->flow.vlans, &old_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(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst); - if (!eth_addr_is_broadcast(flow->dl_dst)) { - return false; - } - - memset(&wc->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(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); - memset(&wc->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(&xbridge->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