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. > 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
