[ovs-dev] [IPv6 3/7] nicira-ext: Support matching ARP source and target hardware addresses.

[Ben Pfaff]

On Fri, Jan 21, 2011 at 04:27:39AM -0800, Justin Pettit wrote:
> OpenFlow 1.0 doesn't allow matching on the ARP source and target
> hardware address.  This has caused us to introduce hacks such as the
> Drop Spoofed ARP action.  Now that we have extensible match, we can
> match on more fields within ARP:
> 
>     - Source Hardware Address (arp_sha)
>     - Target Hardware Address (arp_tha)
> 
> Signed-off-by: Justin Pettit <jpettit at nicira.com>

I have some comments but you need to get feedback from Jesse too of
course.

> diff --git a/datapath/flow.c b/datapath/flow.c
> index 0f73c66..237b8bf 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -376,6 +376,8 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct odp_flow_key *key,
>  					|| key->nw_proto == ARPOP_REPLY) {
>  				memcpy(&key->nw_src, arp->ar_sip, sizeof(key->nw_src));
>  				memcpy(&key->nw_dst, arp->ar_tip, sizeof(key->nw_dst));
> +				memcpy(&key->arp_sha, arp->ar_sha, ETH_ALEN);
> +				memcpy(&key->arp_tha, arp->ar_tha, ETH_ALEN);

The & should be omitted for the destinations here since these are
arrays.  (There's no semantic difference in this case but it's odd to
pass a uint8_t** to memcpy.)

> +    if (!eth_addr_is_zero(flow->arp_sha) || !eth_addr_is_zero(flow->arp_tha)) {
> +        ds_put_format(ds, " ha"ETH_ADDR_FMT"->"ETH_ADDR_FMT,
> +                ETH_ADDR_ARGS(flow->arp_sha),
> +                ETH_ADDR_ARGS(flow->arp_tha));
> +    }

Could we change the lead-in from "ha" to, say, "arp_ha" to make it clear
what protocol header this is?

> +    if (!eth_addr_is_zero(key->arp_sha) || !eth_addr_is_zero(key->arp_tha)) {
> +        ds_put_format(ds, " ha"ETH_ADDR_FMT"->"ETH_ADDR_FMT,
> +                ETH_ADDR_ARGS(key->arp_sha),
> +                ETH_ADDR_ARGS(key->arp_tha));
> +    }

Here too?

Thanks for adding to the tests.  Could you also add tests for
NXM_NX_ARP_SHA and NXM_NX_ARP_THA to the "ovs-ofctl parse-nx-match" test
that starts near line 120 of tests/ovs-ofctl.at?  Thanks.

> @@ -506,10 +512,10 @@ Stops processing further actions, if the packet being processed is an
>  Ethernet+IPv4 ARP packet for which the source Ethernet address inside
>  the ARP packet differs from the source Ethernet address in the
>  Ethernet header.
> -.
> -This is useful because OpenFlow does not provide a way to match on the
> -Ethernet addresses inside ARP packets, so there is no other way to
> -drop spoofed ARPs other than sending every ARP packet to a controller.
> +.IP
> +\fBNOTE:\fR This action is deprecated in favor of defining flows using
> +the \fBarp_sha\fR match field described earlier and will likely be
> +removed in a future version of Open vSwitch.

I'm not sure why, but I hate it when documents start out sentences with
"Note:" and the like.  Unless you feel strongly the other way, would you
mind just dropping that introductory word?

Thanks,

Ben.