Re: [PATCH v9 9/9] arp/ndp: send gratuitous ARP / unsolicitated NA when MAC cache entry added

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6373 bytes --]

On Tue, Sep 23, 2025 at 09:13:30PM -0400, Jon Maloy wrote:
> Gratuitious ARP and unsolicitated NA should be handled with caution
> because of the risk of malignant users emitting them to disturb
> network communication.
> 
> There is however one case we where we know it is legitimate
> and safe for us to send out such messages: The one time we switch
> from using ctx->own_tap_mac to a MAC address received via the
> recently added neigbour subscription function. Later changes to
> the MAC address of a host in an existing entry cannot be fully
> trusted, so we abstain from doing it in such cases.

So, I think you're right that the gratuitous ARP is safe in this case.

But it concerns me that (other that some edge cases) we're sending
data to the guest under own_tap_mac before we get the real MAC.  At
the point we send data from a flow to the guest, I would have expected
to already have an entry in the host neighbour table (because by
definition the host is talking to the peer), therefore in our cache,
by the subscriber.

I'm wondering if it could be as simple as both the neighbour update
and the actual data coming in the same epoll batch, and we could avoid
the temporary use of own_tap_mac by prioritising processing of the
neighbour events.

> When sending this type of messages, we notice that the guest accepts
> the update, but also asks for a confirmation in the form of a regular
> ARP/NS request. This is responded to with the new value, and we have
> exactly the effect we wanted.
> 
> This commit adds this functionality.
> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  arp.c | 39 +++++++++++++++++++++++++++++++++++++++
>  arp.h |  2 ++
>  fwd.c | 11 +++++++++++
>  ndp.c | 10 ++++++++++
>  ndp.h |  1 +
>  5 files changed, 63 insertions(+)
> 
> diff --git a/arp.c b/arp.c
> index 442faff..259f736 100644
> --- a/arp.c
> +++ b/arp.c
> @@ -151,3 +151,42 @@ void arp_send_init_req(const struct ctx *c)
>  	debug("Sending initial ARP request for guest MAC address");
>  	tap_send_single(c, &req, sizeof(req));
>  }
> +
> +/**
> + * arp_send_gratuitous() - Send a gratuitous ARP announcement for an IPv4 host
> + * @c:		Execution context
> + * @ip:	IPv4 address we announce as owned by @mac
> + * @mac:	MAC address to advertise for @ip
> + */
> +void arp_send_gratuitous(const struct ctx *c, struct in_addr ip,
> +			 const unsigned char *mac)
> +{
> +	char ip_str[INET_ADDRSTRLEN];
> +	struct {
> +		struct ethhdr eh;
> +		struct arphdr ah;
> +		struct arpmsg am;
> +	} __attribute__((__packed__)) req;

'req' is not a great name, since this is an ARP response, not a
request (but see below).

> +	/* Ethernet header */
> +	req.eh.h_proto = htons(ETH_P_ARP);
> +	memcpy(req.eh.h_dest, MAC_BROADCAST, sizeof(req.eh.h_dest));
> +	memcpy(req.eh.h_source, c->our_tap_mac, sizeof(req.eh.h_source));
> +
> +	/* ARP header */
> +	req.ah.ar_op = htons(ARPOP_REPLY);
> +	req.ah.ar_hrd = htons(ARPHRD_ETHER);
> +	req.ah.ar_pro = htons(ETH_P_IP);
> +	req.ah.ar_hln = ETH_ALEN;
> +	req.ah.ar_pln = 4;
> +
> +	/* ARP message */
> +	memcpy(req.am.sha, mac, sizeof(req.am.sha));
> +	memcpy(req.am.sip, &ip, sizeof(req.am.sip));
> +	memcpy(req.am.tha, MAC_BROADCAST, sizeof(req.am.tha));
> +	memcpy(req.am.tip, &ip, sizeof(req.am.tip));

So, I was trying to check if it made sense to use the same IP for both
source and target here, and came across
    https://www.rfc-editor.org/rfc/rfc5227#section-3

Which suggests we should (counter intuitively) be using ARP requests,
not ARP replies for announcements.

> +	inet_ntop(AF_INET, &ip, ip_str, sizeof(ip_str));
> +	debug("Sending gratuitous ARP for %s", ip_str);
> +	tap_send_single(c, &req, sizeof(req));
> +}
> diff --git a/arp.h b/arp.h
> index d5ad0e1..b0dbb56 100644
> --- a/arp.h
> +++ b/arp.h
> @@ -22,5 +22,7 @@ struct arpmsg {
>  
>  int arp(const struct ctx *c, struct iov_tail *data);
>  void arp_send_init_req(const struct ctx *c);
> +void arp_send_gratuitous(const struct ctx *c, struct in_addr ip,
> +			 const unsigned char *mac);
>  
>  #endif /* ARP_H */
> diff --git a/fwd.c b/fwd.c
> index c6348ab..879a351 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -26,6 +26,8 @@
>  #include "passt.h"
>  #include "lineread.h"
>  #include "flow_table.h"
> +#include "arp.h"
> +#include "ndp.h"
>  
>  /* Empheral port range: values from RFC 6335 */
>  static in_port_t fwd_ephemeral_min = (1 << 15) + (1 << 14);
> @@ -129,6 +131,15 @@ void fwd_neigh_mac_cache_alloc(const struct ctx *c,
>  
>  	memcpy(&e->addr, addr, sizeof(*addr));
>  	memcpy(e->mac, mac, ETH_ALEN);
> +
> +	/* Send gratuitous ARP / unsolicited NA for the new mapping */

AFAICT this doesn't actually implement what the commit message
describes - it seems to always send an ARP/NA when the neighbour table
is updated.

> +	if (inany_v4(addr)) {
> +		struct in_addr ip4 = *inany_v4(addr);
> +
> +		arp_send_gratuitous(c, ip4, e->mac);
> +	} else {
> +		ndp_send_unsolicited_na(c, &addr->a6);
> +	}
>  }
>  
>  /**
> diff --git a/ndp.c b/ndp.c
> index 70b68aa..8914f31 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -226,6 +226,16 @@ static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
>  	ndp_send(c, dst, &na, sizeof(na));
>  }
>  
> +/**
> + * ndp_send_unsolicited_na() - Send unsolicited NA
> + * @c:		Execution context
> + * @addr:	IPv6 address to advertise
> + */
> +void ndp_send_unsolicited_na(const struct ctx *c, const struct in6_addr *addr)
> +{
> +	ndp_na(c, &in6addr_ll_all_nodes, addr);
> +}
> +
>  /**
>   * ndp_ra() - Send an NDP Router Advertisement (RA) message
>   * @c:		Execution context
> diff --git a/ndp.h b/ndp.h
> index 781ea86..320009c 100644
> --- a/ndp.h
> +++ b/ndp.h
> @@ -12,5 +12,6 @@ int ndp(const struct ctx *c, const struct in6_addr *saddr,
>  	struct iov_tail *data);
>  void ndp_timer(const struct ctx *c, const struct timespec *now);
>  void ndp_send_init_req(const struct ctx *c);
> +void ndp_send_unsolicited_na(const struct ctx *c, const struct in6_addr *addr);
>  
>  #endif /* NDP_H */
> -- 
> 2.50.1
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]