>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/[email protected]/
>
>> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
