Re: [PATCH 3/6] ndp: Split out helpers for sending specific NDP message types

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

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

On Wed, Nov 13, 2024 at 02:14:12AM +0100, Stefano Brivio wrote:
> Nits only:
> 
> On Tue, 12 Nov 2024 15:06:15 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently the large ndp() function responds to all NDP messages we handle,
> > both parsing the message as necessary and sending the response.  Split out
> > the code to construct and send specific message types into ndp_na() (to
> > send NA messages) and ndp_ra() (to send RA messages).
> > 
> > As well as breaking up an excessively large function, this is a first step
> > to being able to send unsolicited NDP messages.
> > 
> > While we're there, remove a slighty ugly goto.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  ndp.c | 136 +++++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 78 insertions(+), 58 deletions(-)
> > 
> > diff --git a/ndp.c b/ndp.c
> > index fa1b67a..e876c34 100644
> > --- a/ndp.c
> > +++ b/ndp.c
> > @@ -186,16 +186,13 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
> >  }
> >  
> >  /**
> > - * ndp() - Check for NDP solicitations, reply as needed
> > + * ndp_na() - Send an NDP Neighbour Advertisement (NA) message
> >   * @c:		Execution context
> > - * @ih:		ICMPv6 header
> > - * @saddr:	Source IPv6 address
> > - * @p:		Packet pool
> > - *
> > - * Return: 0 if not handled here, 1 if handled, -1 on failure
> > + * @dst:	IPv6 address to send the NA to
> > + * @addr:	IPv6 address to advertise
> >   */
> > -int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> > -	const struct in6_addr *saddr, const struct pool *p)
> > +static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
> > +		   const void *addr)
> >  {
> >  	struct ndp_na na = {
> >  		.ih = {
> > @@ -212,6 +209,20 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  			},
> >  		}
> >  	};
> > +
> > +	memcpy(&na.target_addr, addr, sizeof(na.target_addr));
> > +	memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
> > +
> > +	ndp_send(c, dst, &na, sizeof(na));
> > +}
> > +
> > +/**
> > + * ndp_ra() - Send an NDP Router Advertisement (RA) message
> > + * @c:		Execution context
> > + * @dst:	IPv6 address to send the RA to
> > + */
> > +static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
> > +{
> >  	struct ndp_ra ra = {
> >  		.ih = {
> >  			.icmp6_type		= RA,
> > @@ -238,58 +249,28 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  			},
> >  		},
> >  	};
> > +	unsigned char *ptr = NULL;
> >  
> > -	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
> > -		return 0;
> > -
> > -	if (c->no_ndp)
> > -		return 1;
> > -
> > -	if (ih->icmp6_type == NS) {
> > -		const struct ndp_ns *ns =
> > -			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
> > +	memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
> >  
> > -		if (!ns)
> > -			return -1;
> > +	ptr = &ra.var[0];
> >  
> > -		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
> > -			return 1;
> > -
> > -		info("NDP: received NS, sending NA");
> > -
> > -		memcpy(&na.target_addr, &ns->target_addr,
> > -		       sizeof(na.target_addr));
> > -		memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
> > +	if (c->mtu != -1) {
> > +		struct opt_mtu *mtu = (struct opt_mtu *)ptr;
> > +		*mtu = (struct opt_mtu) {
> > +			.header = {
> > +				.type		= OPT_MTU,
> > +				.len		= 1,
> > +			},
> > +			.value			= htonl(c->mtu),
> > +		};
> > +		ptr += sizeof(struct opt_mtu);
> > +	}
> >  
> > -		ndp_send(c, saddr, &na, sizeof(struct ndp_na));
> > -	} else if (ih->icmp6_type == RS) {
> > -		unsigned char *ptr = NULL;
> > +	if (!c->no_dhcp_dns) {
> >  		size_t dns_s_len = 0;
> >  		int i, n;
> >  
> > -		if (c->no_ra)
> > -			return 1;
> > -
> > -		info("NDP: received RS, sending RA");
> > -		memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
> > -
> > -		ptr = &ra.var[0];
> > -
> > -		if (c->mtu != -1) {
> > -			struct opt_mtu *mtu = (struct opt_mtu *)ptr;
> > -			*mtu = (struct opt_mtu) {
> > -				.header = {
> > -					.type		= OPT_MTU,
> > -					.len		= 1,
> > -				},
> > -				.value			= htonl(c->mtu),
> > -			};
> > -			ptr += sizeof(struct opt_mtu);
> > -		}
> > -
> > -		if (c->no_dhcp_dns)
> > -			goto dns_done;
> > -
> >  		for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++);
> >  		if (n) {
> >  			struct opt_rdnss *rdnss = (struct opt_rdnss *)ptr;
> > @@ -305,7 +286,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  				       sizeof(rdnss->dns[i]));
> >  			}
> >  			ptr += offsetof(struct opt_rdnss, dns) +
> > -			       i * sizeof(rdnss->dns[0]);
> > +				i * sizeof(rdnss->dns[0]);
> 
> Unrelated change, less readable, probably from your new editor / clangd?

Curiously, no, I wasn't using Zed at the time.  That just happened
because I reindented the block a couple of times, and apparently my
emacs indent settings aren't *quite* the same as passt standard.

Anyway, fixed.

> >  
> >  			for (n = 0; *c->dns_search[n].n; n++)
> >  				dns_s_len += strlen(c->dns_search[n].n) + 2;
> > @@ -329,7 +310,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  				*(ptr++) = '.';
> >  
> >  				len = sizeof(dnssl->domains) -
> > -				      (ptr - dnssl->domains);
> > +					(ptr - dnssl->domains);
> 
> Same here.
> 
> >  
> >  				strncpy((char *)ptr, c->dns_search[i].n, len);
> >  				for (dot = (char *)ptr - 1; *dot; dot++) {
> > @@ -343,11 +324,50 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> >  			memset(ptr, 0, 8 - dns_s_len % 8);	/* padding */
> >  			ptr += 8 - dns_s_len % 8;
> >  		}
> > +	}
> >  
> > -dns_done:
> > -		memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
> > +	memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
> >  
> > -		ndp_send(c, saddr, &ra, ptr - (unsigned char *)&ra);
> > +	ndp_send(c, dst, &ra, ptr - (unsigned char *)&ra);
> > +}
> > +
> > +/**
> > + * ndp() - Check for NDP solicitations, reply as needed
> > + * @c:		Execution context
> > + * @ih:		ICMPv6 header
> > + * @saddr:	Source IPv6 address
> > + * @p:		Packet pool
> > + *
> > + * Return: 0 if not handled here, 1 if handled, -1 on failure
> > + */
> > +int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> > +	const struct in6_addr *saddr, const struct pool *p)
> > +{
> > +	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
> > +		return 0;
> > +
> > +	if (c->no_ndp)
> > +		return 1;
> > +
> > +	if (ih->icmp6_type == NS) {
> > +		const struct ndp_ns *ns =
> > +			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
> 
> The assignment could go on its line here? We don't save any line this
> way.

Good point, done.

> > +
> > +		if (!ns)
> > +			return -1;
> > +
> > +		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
> > +			return 1;
> > +
> > +		info("NDP: received NS, sending NA");
> > +
> > +		ndp_na(c, saddr, &ns->target_addr);
> > +	} else if (ih->icmp6_type == RS) {
> > +		if (c->no_ra)
> > +			return 1;
> > +
> > +		info("NDP: received RS, sending RA");
> > +		ndp_ra(c, saddr);
> >  	}
> >  
> >  	return 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 --]