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.maxim...@ovn.org>
I didn't ACK this patch. Please, don't add tags not explicitly given.
>> Signed-off-by: Han Ding <hand...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
