Re: [PATCH v6 01/30] arp: Don't mix incoming and outgoing buffers

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  4 Jun 2025 15:08:05 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Don't use the memory of the incoming packet to build the outgoing buffer
> as it can be memory of the TX queue in the case of vhost-user.
> 
> Moreover with vhost-user, the packet can be split across several
> iovec and it's easier to rebuild it in a buffer than updating an
> existing iovec array.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  arp.c | 84 ++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 55 insertions(+), 29 deletions(-)
> 
> diff --git a/arp.c b/arp.c
> index fc482bbd9938..721ff2d529b5 100644
> --- a/arp.c
> +++ b/arp.c
> @@ -31,56 +31,82 @@
>  #include "tap.h"
>  
>  /**
> - * arp() - Check if this is a supported ARP message, reply as needed
> + * accept_arp() - Check if we should accept this ARP message
>   * @c:		Execution context
> - * @p:		Packet pool, single packet with Ethernet buffer
> + * @ah:		ARP header
> + * @am:		ARP message
>   *
> - * Return: 1 if handled, -1 on failure
> + * Return: true if the message is supported, false otherwise.

...wait, the function name is reversed now, but this isn't. You
actually return true if the message should be discarded, not if the
message "is supported".

On v5, I mentioned I'd change the name of the function to accept_arp()
rather than reversing the return value... but I missed this part,
sorry. I guess if it's accept_arp(), it should return true *if we want
to do something with the message*. If it's ignore_arp(), the opposite.

I don't have a particular preference, as long as the function doesn't
return the opposite of what the verb in its name says. :)

>   */
> -int arp(const struct ctx *c, const struct pool *p)
> +static bool accept_arp(const struct ctx *c,
> +		       const struct arphdr *ah, const struct arpmsg *am)
>  {
> -	unsigned char swap[4];
> -	struct ethhdr *eh;
> -	struct arphdr *ah;
> -	struct arpmsg *am;
> -	size_t l2len;
> -
> -	eh = packet_get(p, 0, 0,			 sizeof(*eh), NULL);
> -	ah = packet_get(p, 0, sizeof(*eh),		 sizeof(*ah), NULL);
> -	am = packet_get(p, 0, sizeof(*eh) + sizeof(*ah), sizeof(*am), NULL);
> -
> -	if (!eh || !ah || !am)
> -		return -1;
> -
>  	if (ah->ar_hrd != htons(ARPHRD_ETHER)	||
>  	    ah->ar_pro != htons(ETH_P_IP)	||
>  	    ah->ar_hln != ETH_ALEN		||
>  	    ah->ar_pln != 4			||
>  	    ah->ar_op  != htons(ARPOP_REQUEST))
> -		return 1;
> +		return true;
>  
>  	/* Discard announcements, but not 0.0.0.0 "probes" */
>  	if (memcmp(am->sip, &in4addr_any, sizeof(am->sip)) &&
>  	    !memcmp(am->sip, am->tip, sizeof(am->sip)))
> -		return 1;
> +		return true;
>  
>  	/* Don't resolve the guest's assigned address, either. */
>  	if (!memcmp(am->tip, &c->ip4.addr, sizeof(am->tip)))
> +		return true;
> +
> +	return false;
> +}
> +
> +/**
> + * arp() - Check if this is a supported ARP message, reply as needed
> + * @c:		Execution context
> + * @p:		Packet pool, single packet with Ethernet buffer
> + *
> + * Return: 1 if handled, -1 on failure
> + */
> +int arp(const struct ctx *c, const struct pool *p)
> +{
> +	struct {
> +		struct ethhdr eh;
> +		struct arphdr ah;
> +		struct arpmsg am;
> +	} __attribute__((__packed__)) resp;
> +	const struct ethhdr *eh;
> +	const struct arphdr *ah;
> +	const struct arpmsg *am;
> +
> +	eh = packet_get(p, 0, 0,			 sizeof(*eh), NULL);
> +	ah = packet_get(p, 0, sizeof(*eh),		 sizeof(*ah), NULL);
> +	am = packet_get(p, 0, sizeof(*eh) + sizeof(*ah), sizeof(*am), NULL);
> +
> +	if (!eh || !ah || !am)
> +		return -1;
> +
> +	if (accept_arp(c, ah, am))
>  		return 1;
>  
> -	ah->ar_op = htons(ARPOP_REPLY);
> -	memcpy(am->tha,		am->sha,	sizeof(am->tha));
> -	memcpy(am->sha,		c->our_tap_mac,	sizeof(am->sha));
> +	/* Ethernet header */
> +	resp.eh.h_proto = htons(ETH_P_ARP);
> +	memcpy(resp.eh.h_dest, eh->h_source, sizeof(resp.eh.h_dest));
> +	memcpy(resp.eh.h_source, c->our_tap_mac, sizeof(resp.eh.h_source));
>  
> -	memcpy(swap,		am->tip,	sizeof(am->tip));
> -	memcpy(am->tip,		am->sip,	sizeof(am->tip));
> -	memcpy(am->sip,		swap,		sizeof(am->sip));
> +	/* ARP header */
> +	resp.ah.ar_op = htons(ARPOP_REPLY);
> +	resp.ah.ar_hrd = ah->ar_hrd;
> +	resp.ah.ar_pro = ah->ar_pro;
> +	resp.ah.ar_hln = ah->ar_hln;
> +	resp.ah.ar_pln = ah->ar_pln;
>  
> -	l2len = sizeof(*eh) + sizeof(*ah) + sizeof(*am);
> -	memcpy(eh->h_dest,	eh->h_source,	sizeof(eh->h_dest));
> -	memcpy(eh->h_source,	c->our_tap_mac,	sizeof(eh->h_source));
> +	/* ARP message */
> +	memcpy(resp.am.sha,		c->our_tap_mac,	sizeof(resp.am.sha));
> +	memcpy(resp.am.sip,		am->tip,	sizeof(resp.am.sip));
> +	memcpy(resp.am.tha,		am->sha,	sizeof(resp.am.tha));
> +	memcpy(resp.am.tip,		am->sip,	sizeof(resp.am.tip));
>  
> -	tap_send_single(c, eh, l2len);
> +	tap_send_single(c, &resp, sizeof(resp));
>  
>  	return 1;
>  }

-- 
Stefano