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

[Stefano Brivio <sbrivio@redhat.com>]

Here's my review, finally. Apologies for the very substantial delay.

On Thu, 17 Apr 2025 18:51:08 +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 splitted accross 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..9d68d7c3b602 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
> + * ignore_arp() - Check if this is a supported 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.

I find this, in combination with the name, a bit confusing. You now
have ignore_arp() returning true if the ARP message should *not* be
ignored.

As this function is essentially a filter, rather than reversing the
return value, I'd rather reverse the name, say:

 * accept_arp() - Check if we should accept this ARP message

>   */
> -int arp(const struct ctx *c, const struct pool *p)
> +static bool ignore_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 (ignore_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 */

Nit: Ethernet

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