[ovs-dev] [L3 In-Band 6/9] Add ability for the datapath to match IP address in ARPs

[Ben Pfaff]

Justin Pettit <jpettit at nicira.com> writes:

>  struct kmem_cache *flow_cache;
>  
> +
> +struct arp_eth_header
> +{
> +	__be16      ar_hrd;	/* format of hardware address   */
> +	__be16      ar_pro;	/* format of protocol address   */

This introduces a doubled blank line, would you mind making it
just a single blank line?

> +	} else if (key->dl_type == htons(ETH_P_ARP) && arphdr_ok(skb)) {
> +		struct arp_eth_header *arp;
> +
> +		arp = (struct arp_eth_header *)skb_network_header(skb);
> +
> +		/* We only match on the lower 8 bits of the opcode. */
> +		if (ntohs(arp->ar_op) <= 0xff) {
> +			key->nw_proto = ntohs(arp->ar_op);
> +		}
> +
> +		if ((key->nw_proto == ARPOP_REQUEST) 
> +				|| (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));
> +		}

Do you think that it is worthwhile checking that ar_hrd is
Ethernet, ar_pro is IP, ar_hln is 6, and ar_pln is 4?  Otherwise,
we could mistakenly match non-Ethernet media or non-IP protocols.
Those are uncommon but possible, e.g. wikipedia claims that
"ATMARP is used to resolve ATM NSAP addresses in the Classical IP
over ATM protocol."

Same question about lib/flow.c, of course.

Nit: kernel style mavens would probably complain about the extra
parentheses around the ||'s operator's clauses.

> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -31,6 +31,15 @@
>  #include "vlog.h"
>  #define THIS_MODULE VLM_flow
>  
> +static struct arp_eth_header *
> +pull_arp(struct ofpbuf *packet)
> +{
> +    if (packet->size >= ARP_ETH_HEADER_LEN) {
> +        return ofpbuf_pull(packet, ARP_ETH_HEADER_LEN);
> +    }
> +    return NULL;
> +}

I think that this function can be implemented as:
    return ofpbuf_try_pull(packet, ARP_ETH_HEADER_LEN);

> --- a/secchan/ofproto.c
> +++ b/secchan/ofproto.c
> @@ -2496,6 +2496,28 @@ struct flow_stats_ds_cbdata {
>      struct ds *results;
>  };
>  
> +/* The Open vSwitch datapath supports matching on ARP payloads, which 
> + * OpenFlow does not.  This function is identical to 'flow_to_match',
> + * but does not hide our ability to match on ARP. */
> +static void
> +flow_to_ovs_match(const flow_t *flow, uint32_t wildcards, 
> +                  struct ofp_match *match)
> +{

Given that it's a subset of flow_to_match(), could
flow_to_match() be re-implemented as a wrapper around
flow_to_ovs_match()?  In general I'm in favor of reducing
redundancy.