Index | Thread | Search

From:
Claudio Jeker <claudio@openbsd.org>
Subject:
Re: struct route inet
To:
Alexander Bluhm <alexander.bluhm@gmx.net>
Cc:
tech@openbsd.org
Date:
Wed, 7 Feb 2024 11:24:22 +0100

Download raw body.

Thread
On Fri, Feb 02, 2024 at 10:11:00PM +0100, Alexander Bluhm wrote:
> Hi,
> 
> Claudio complained about generic struct route usage without enough
> storage to hold IPv6 address.  This diff splits definitions in
> struct route_in and route_in6 with correct types.
> 
> Is that what you want?

In an ideal world I would like to have one struct route object that can
handle both IPv4 and IPv6. This would remove the union in inpcb and makes
the struct route object more general.

If we have one struct route object it is possible to extend it and include
the source address into the cache so that rtalloc_mpath() can use that
instead of the uint32_t * used for the source right now.

I think this would help clean up the route lookups a fair bit more.

-- 
:wq Claudio



> Index: net/route.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
> diff -u -p -r1.427 route.c
> --- net/route.c	31 Jan 2024 14:56:42 -0000	1.427
> +++ net/route.c	2 Feb 2024 20:41:04 -0000
> @@ -202,7 +202,7 @@ route_init(void)
>  }
>  
>  void
> -route_cache(struct route *ro, struct in_addr addr, u_int rtableid)
> +route_cache_in(struct route_in *ro, struct in_addr addr, u_int rtableid)
>  {
>  	u_long gen;
>  
> @@ -212,8 +212,8 @@ route_cache(struct route *ro, struct in_
>  	if (rtisvalid(ro->ro_rt) &&
>  	    ro->ro_generation == gen &&
>  	    ro->ro_tableid == rtableid &&
> -	    ro->ro_dst.sa_family == AF_INET &&
> -	    satosin(&ro->ro_dst)->sin_addr.s_addr == addr.s_addr) {
> +	    ro->ro_dst.sin_family == AF_INET &&
> +	    ro->ro_dst.sin_addr.s_addr == addr.s_addr) {
>  		return;
>  	}
>  
> @@ -223,9 +223,9 @@ route_cache(struct route *ro, struct in_
>  	ro->ro_tableid = rtableid;
>  
>  	memset(&ro->ro_dst, 0, sizeof(ro->ro_dst));
> -	satosin(&ro->ro_dst)->sin_family = AF_INET;
> -	satosin(&ro->ro_dst)->sin_len = sizeof(struct sockaddr_in);
> -	satosin(&ro->ro_dst)->sin_addr = addr;
> +	ro->ro_dst.sin_family = AF_INET;
> +	ro->ro_dst.sin_len = sizeof(struct sockaddr_in);
> +	ro->ro_dst.sin_addr = addr;
>  }
>  
>  /*
> Index: net/route.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v
> diff -u -p -r1.204 route.h
> --- net/route.h	31 Jan 2024 14:56:42 -0000	1.204
> +++ net/route.h	2 Feb 2024 20:52:50 -0000
> @@ -448,9 +448,10 @@ struct in_addr;
>  struct sockaddr_in6;
>  struct if_ieee80211_data;
>  struct bfd_config;
> +struct route_in;
>  
>  void	 route_init(void);
> -void	 route_cache(struct route *, struct in_addr, u_int);
> +void	 route_cache_in(struct route_in *, struct in_addr, u_int);
>  void	 rtm_ifchg(struct ifnet *);
>  void	 rtm_ifannounce(struct ifnet *, int);
>  void	 rtm_bfd(struct bfd_config *);
> Index: netinet/in.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.h,v
> diff -u -p -r1.145 in.h
> --- netinet/in.h	10 Nov 2023 20:05:22 -0000	1.145
> +++ netinet/in.h	2 Feb 2024 20:48:14 -0000
> @@ -322,6 +322,16 @@ struct ip_opts {
>  
>  #if __BSD_VISIBLE
>  /*
> + * IPv4 route structure, keep fields in sync with struct route.
> + */
> +struct route_in {
> +	struct	rtentry *ro_rt;
> +	u_long		 ro_generation;
> +	u_long		 ro_tableid;	/* padded to long for alignment */
> +	struct	sockaddr_in ro_dst;
> +};
> +
> +/*
>   * Security levels - IPsec, not IPSO
>   */
>  
> Index: netinet/in_pcb.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v
> diff -u -p -r1.289 in_pcb.c
> --- netinet/in_pcb.c	2 Feb 2024 15:39:23 -0000	1.289
> +++ netinet/in_pcb.c	2 Feb 2024 20:54:19 -0000
> @@ -907,7 +907,7 @@ in_pcblookup_local_lock(struct inpcbtabl
>  struct rtentry *
>  in_pcbrtentry(struct inpcb *inp)
>  {
> -	struct route *ro;
> +	struct route_in *ro;
>  
>  #ifdef INET6
>  	if (ISSET(inp->inp_flags, INP_IPV6))
> @@ -926,15 +926,15 @@ in_pcbrtentry(struct inpcb *inp)
>  	 * No route yet, so try to acquire one.
>  	 */
>  	if (ro->ro_rt == NULL) {
> -		memset(ro, 0, sizeof(struct route));
> +		memset(ro, 0, sizeof(struct route_in));
>  
>  		if (inp->inp_faddr.s_addr == INADDR_ANY)
>  			return (NULL);
> -		ro->ro_dst.sa_family = AF_INET;
> -		ro->ro_dst.sa_len = sizeof(struct sockaddr_in);
> -		satosin(&ro->ro_dst)->sin_addr = inp->inp_faddr;
> +		ro->ro_dst.sin_family = AF_INET;
> +		ro->ro_dst.sin_len = sizeof(struct sockaddr_in);
> +		ro->ro_dst.sin_addr = inp->inp_faddr;
>  		ro->ro_tableid = inp->inp_rtableid;
> -		ro->ro_rt = rtalloc_mpath(&ro->ro_dst,
> +		ro->ro_rt = rtalloc_mpath(sintosa(&ro->ro_dst),
>  		    &inp->inp_laddr.s_addr, ro->ro_tableid);
>  	}
>  	return (ro->ro_rt);
> @@ -951,7 +951,7 @@ in_pcbselsrc(struct in_addr *insrc, stru
>      struct inpcb *inp)
>  {
>  	struct ip_moptions *mopts = inp->inp_moptions;
> -	struct route *ro = &inp->inp_route;
> +	struct route_in *ro = &inp->inp_route;
>  	const struct in_addr *laddr = &inp->inp_laddr;
>  	u_int rtableid = inp->inp_rtableid;
>  	struct sockaddr	*ip4_source = NULL;
> @@ -999,23 +999,24 @@ in_pcbselsrc(struct in_addr *insrc, stru
>  	 * our src addr is taken from the i/f, else punt.
>  	 */
>  	if (!rtisvalid(ro->ro_rt) || (ro->ro_tableid != rtableid) ||
> -	    (satosin(&ro->ro_dst)->sin_addr.s_addr != sin->sin_addr.s_addr)) {
> +	    (ro->ro_dst.sin_addr.s_addr != sin->sin_addr.s_addr)) {
>  		rtfree(ro->ro_rt);
>  		ro->ro_rt = NULL;
>  	}
>  	if (ro->ro_rt == NULL) {
>  		/* No route yet, so try to acquire one */
> -		ro->ro_dst.sa_family = AF_INET;
> -		ro->ro_dst.sa_len = sizeof(struct sockaddr_in);
> -		satosin(&ro->ro_dst)->sin_addr = sin->sin_addr;
> +		ro->ro_dst.sin_family = AF_INET;
> +		ro->ro_dst.sin_len = sizeof(struct sockaddr_in);
> +		ro->ro_dst.sin_addr = sin->sin_addr;
>  		ro->ro_tableid = rtableid;
> -		ro->ro_rt = rtalloc_mpath(&ro->ro_dst, NULL, ro->ro_tableid);
> +		ro->ro_rt = rtalloc_mpath(sintosa(&ro->ro_dst), NULL,
> +		    ro->ro_tableid);
>  
>  		/*
>  		 * It is important to zero out the rest of the
>  		 * struct sockaddr_in when mixing v6 & v4!
>  		 */
> -		sin2 = satosin(&ro->ro_dst);
> +		sin2 = &ro->ro_dst;
>  		memset(sin2->sin_zero, 0, sizeof(sin2->sin_zero));
>  	}
>  
> Index: netinet/in_pcb.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.h,v
> diff -u -p -r1.150 in_pcb.h
> --- netinet/in_pcb.h	31 Jan 2024 12:27:57 -0000	1.150
> +++ netinet/in_pcb.h	2 Feb 2024 20:27:59 -0000
> @@ -152,11 +152,11 @@ struct inpcb {
>  	struct	  socket *inp_socket;	/* [I] back pointer to socket */
>  	caddr_t	  inp_ppcb;		/* pointer to per-protocol pcb */
>  	union {				/* Route (notice increased size). */
> -		struct route ru_route;
> -		struct route_in6 ru_route6;
> +		struct route_in iru_route;
> +		struct route_in6 iru_route6;
>  	} inp_ru;
> -#define	inp_route	inp_ru.ru_route
> -#define	inp_route6	inp_ru.ru_route6
> +#define inp_route	inp_ru.iru_route
> +#define inp_route6	inp_ru.iru_route6
>  	struct    refcnt inp_refcnt;	/* refcount PCB, delay memory free */
>  	struct	  mutex inp_mtx;	/* protect PCB and socket members */
>  	int	  inp_flags;		/* generic IP/datagram flags */
> Index: netinet/ip_input.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v
> diff -u -p -r1.388 ip_input.c
> --- netinet/ip_input.c	31 Jan 2024 14:56:42 -0000	1.388
> +++ netinet/ip_input.c	2 Feb 2024 20:41:57 -0000
> @@ -1475,7 +1475,7 @@ ip_forward(struct mbuf *m, struct ifnet 
>  {
>  	struct mbuf mfake, *mcopy = NULL;
>  	struct ip *ip = mtod(m, struct ip *);
> -	struct route ro;
> +	struct route_in ro;
>  	int error = 0, type = 0, code = 0, destmtu = 0, fake = 0, len;
>  	u_int32_t dest;
>  
> @@ -1491,10 +1491,10 @@ ip_forward(struct mbuf *m, struct ifnet 
>  	}
>  
>  	ro.ro_rt = NULL;
> -	route_cache(&ro, ip->ip_dst, m->m_pkthdr.ph_rtableid);
> +	route_cache_in(&ro, ip->ip_dst, m->m_pkthdr.ph_rtableid);
>  	if (!rtisvalid(rt)) {
>  		rtfree(rt);
> -		rt = rtalloc_mpath(&ro.ro_dst, &ip->ip_src.s_addr,
> +		rt = rtalloc_mpath(sintosa(&ro.ro_dst), &ip->ip_src.s_addr,
>  		    m->m_pkthdr.ph_rtableid);
>  		if (rt == NULL) {
>  			ipstat_inc(ips_noroute);
> Index: netinet/ip_output.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v
> diff -u -p -r1.394 ip_output.c
> --- netinet/ip_output.c	31 Jan 2024 14:56:43 -0000	1.394
> +++ netinet/ip_output.c	2 Feb 2024 20:43:22 -0000
> @@ -86,9 +86,9 @@ void in_delayed_cksum(struct mbuf *);
>  
>  int ip_output_ipsec_lookup(struct mbuf *m, int hlen, const u_char seclevel[],
>      struct tdb **, int ipsecflowinfo);
> -void ip_output_ipsec_pmtu_update(struct tdb *, struct route *, struct in_addr,
> -    int, int);
> -int ip_output_ipsec_send(struct tdb *, struct mbuf *, struct route *, int);
> +void ip_output_ipsec_pmtu_update(struct tdb *, struct route_in *,
> +    struct in_addr, int, int);
> +int ip_output_ipsec_send(struct tdb *, struct mbuf *, struct route_in *, int);
>  
>  /*
>   * IP output.  The packet in mbuf chain m contains a skeletal IP
> @@ -97,7 +97,7 @@ int ip_output_ipsec_send(struct tdb *, s
>   * The mbuf opt, if present, will not be freed.
>   */
>  int
> -ip_output(struct mbuf *m, struct mbuf *opt, struct route *ro, int flags,
> +ip_output(struct mbuf *m, struct mbuf *opt, struct route_in *ro, int flags,
>      struct ip_moptions *imo, const u_char seclevel[], u_int32_t ipsecflowinfo)
>  {
>  	struct ip *ip;
> @@ -105,7 +105,7 @@ ip_output(struct mbuf *m, struct mbuf *o
>  	struct mbuf_list ml;
>  	int hlen = sizeof (struct ip);
>  	int error = 0;
> -	struct route iproute;
> +	struct route_in iproute;
>  	struct sockaddr_in *dst;
>  	struct tdb *tdb = NULL;
>  	u_long mtu;
> @@ -166,8 +166,8 @@ reroute:
>  	 * If there is a cached route, check that it is to the same
>  	 * destination and is still up.  If not, free it and try again.
>  	 */
> -	route_cache(ro, ip->ip_dst, m->m_pkthdr.ph_rtableid);
> -	dst = satosin(&ro->ro_dst);
> +	route_cache_in(ro, ip->ip_dst, m->m_pkthdr.ph_rtableid);
> +	dst = &ro->ro_dst;
>  
>  	if ((IN_MULTICAST(ip->ip_dst.s_addr) ||
>  	    (ip->ip_dst.s_addr == INADDR_BROADCAST)) &&
> @@ -185,7 +185,7 @@ reroute:
>  		struct in_ifaddr *ia;
>  
>  		if (ro->ro_rt == NULL)
> -			ro->ro_rt = rtalloc_mpath(&ro->ro_dst,
> +			ro->ro_rt = rtalloc_mpath(sintosa(&ro->ro_dst),
>  			    &ip->ip_src.s_addr, ro->ro_tableid);
>  
>  		if (ro->ro_rt == NULL) {
> @@ -253,7 +253,7 @@ reroute:
>  		 * still points to the address in "ro".  (It may have been
>  		 * changed to point to a gateway address, above.)
>  		 */
> -		dst = satosin(&ro->ro_dst);
> +		dst = &ro->ro_dst;
>  
>  		/*
>  		 * See if the caller provided any multicast options
> @@ -454,8 +454,8 @@ sendit:
>  		if (ro->ro_tableid != orig_rtableid) {
>  			rtfree(ro->ro_rt);
>  			ro->ro_tableid = orig_rtableid;
> -			ro->ro_rt = icmp_mtudisc_clone(
> -			    satosin(&ro->ro_dst)->sin_addr, ro->ro_tableid, 0);
> +			ro->ro_rt = icmp_mtudisc_clone(ro->ro_dst.sin_addr,
> +			    ro->ro_tableid, 0);
>  		}
>  #endif
>  		/*
> @@ -536,7 +536,7 @@ ip_output_ipsec_lookup(struct mbuf *m, i
>  }
>  
>  void
> -ip_output_ipsec_pmtu_update(struct tdb *tdb, struct route *ro,
> +ip_output_ipsec_pmtu_update(struct tdb *tdb, struct route_in *ro,
>      struct in_addr dst, int rtableid, int transportmode)
>  {
>  	struct rtentry *rt = NULL;
> @@ -558,7 +558,8 @@ ip_output_ipsec_pmtu_update(struct tdb *
>  		rt->rt_mtu = tdb->tdb_mtu;
>  		if (ro != NULL && ro->ro_rt != NULL) {
>  			rtfree(ro->ro_rt);
> -			ro->ro_rt = rtalloc(&ro->ro_dst, RT_RESOLVE, rtableid);
> +			ro->ro_rt = rtalloc(sintosa(&ro->ro_dst),
> +			    RT_RESOLVE, rtableid);
>  		}
>  		if (rt_mtucloned)
>  			rtfree(rt);
> @@ -566,7 +567,8 @@ ip_output_ipsec_pmtu_update(struct tdb *
>  }
>  
>  int
> -ip_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route *ro, int fwd)
> +ip_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route_in *ro,
> +    int fwd)
>  {
>  	struct mbuf_list ml;
>  	struct ifnet *encif = NULL;
> Index: netinet/ip_var.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_var.h,v
> diff -u -p -r1.110 ip_var.h
> --- netinet/ip_var.h	26 Nov 2023 22:08:10 -0000	1.110
> +++ netinet/ip_var.h	2 Feb 2024 20:40:13 -0000
> @@ -223,7 +223,7 @@ extern const struct pr_usrreqs rip_usrre
>  
>  extern struct rttimer_queue ip_mtudisc_timeout_q;
>  extern struct pool ipqent_pool;
> -struct route;
> +struct route_in;
>  struct inpcb;
>  
>  int	 ip_ctloutput(int, struct socket *, int, int, struct mbuf *);
> @@ -235,7 +235,7 @@ struct mbuf*
>  	 ip_insertoptions(struct mbuf *, struct mbuf *, int *);
>  int	 ip_mforward(struct mbuf *, struct ifnet *);
>  int	 ip_optcopy(struct ip *, struct ip *);
> -int	 ip_output(struct mbuf *, struct mbuf *, struct route *, int,
> +int	 ip_output(struct mbuf *, struct mbuf *, struct route_in *, int,
>  	    struct ip_moptions *, const u_char[], u_int32_t);
>  u_int16_t
>  	 ip_randomid(void);
> Index: netinet/tcp_input.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
> diff -u -p -r1.399 tcp_input.c
> --- netinet/tcp_input.c	27 Jan 2024 21:13:46 -0000	1.399
> +++ netinet/tcp_input.c	2 Feb 2024 20:59:38 -0000
> @@ -3165,7 +3165,7 @@ syn_cache_put(struct syn_cache *sc)
>  
>  	/* Dealing with last reference, no lock needed. */
>  	m_free(sc->sc_ipopts);
> -	rtfree(sc->sc_route4.ro_rt);
> +	rtfree(sc->sc_route.ro_rt);
>  
>  	pool_put(&syn_cache_pool, sc);
>  }
> @@ -3578,12 +3578,12 @@ syn_cache_get(struct sockaddr *src, stru
>  	 * Give the new socket our cached route reference.
>  	 */
>  	if (src->sa_family == AF_INET)
> -		inp->inp_route = sc->sc_route4;         /* struct assignment */
> +		inp->inp_route = sc->sc_route;		/* struct assignment */
>  #ifdef INET6
>  	else
>  		inp->inp_route6 = sc->sc_route6;
>  #endif
> -	sc->sc_route4.ro_rt = NULL;
> +	sc->sc_route.ro_rt = NULL;
>  
>  	am = m_get(M_DONTWAIT, MT_SONAME);	/* XXX */
>  	if (am == NULL)
> @@ -4151,7 +4151,7 @@ syn_cache_respond(struct syn_cache *sc, 
>  		if (inp != NULL)
>  			ip->ip_tos = inp->inp_ip.ip_tos;
>  
> -		error = ip_output(m, sc->sc_ipopts, &sc->sc_route4,
> +		error = ip_output(m, sc->sc_ipopts, &sc->sc_route,
>  		    (ip_mtudisc ? IP_MTUDISC : 0),  NULL,
>  		    inp ? inp->inp_seclevel : NULL, 0);
>  		break;
> Index: netinet/tcp_var.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_var.h,v
> diff -u -p -r1.175 tcp_var.h
> --- netinet/tcp_var.h	27 Jan 2024 21:13:46 -0000	1.175
> +++ netinet/tcp_var.h	2 Feb 2024 20:45:17 -0000
> @@ -248,15 +248,11 @@ struct syn_cache {
>  	struct refcnt sc_refcnt;		/* ref count list and timer */
>  	struct timeout sc_timer;		/* rexmt timer */
>  	union {					/* cached route */
> -		struct route route4;
> -#ifdef INET6
> -		struct route_in6 route6;
> -#endif
> -	} sc_route_u;
> -#define sc_route4	sc_route_u.route4	/* [N] */
> -#ifdef INET6
> -#define sc_route6	sc_route_u.route6	/* [N] */
> -#endif
> +		struct route_in sru_route;
> +		struct route_in6 sru_route6;
> +	} sc_ru;
> +#define sc_route	sc_ru.sru_route		/* [N] */
> +#define sc_route6	sc_ru.sru_route6	/* [N] */
>  	long sc_win;				/* [I] advertised window */
>  	struct syn_cache_head *sc_buckethead;	/* [S] our bucket index */
>  	struct syn_cache_set *sc_set;		/* [S] our syn cache set */
> Index: netinet6/in6.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6.h,v
> diff -u -p -r1.113 in6.h
> --- netinet6/in6.h	31 Jan 2024 14:56:43 -0000	1.113
> +++ netinet6/in6.h	2 Feb 2024 20:23:09 -0000
> @@ -145,7 +145,7 @@ extern const struct in6_addr in6addr_lin
>  
>  #if __BSD_VISIBLE
>  /*
> - * IPv6 route structure, keep fields in sync with struct route
> + * IPv6 route structure, keep fields in sync with struct route.
>   */
>  struct route_in6 {
>  	struct	rtentry *ro_rt;
> Index: netinet6/in6_pcb.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6_pcb.c,v
> diff -u -p -r1.134 in6_pcb.c
> --- netinet6/in6_pcb.c	31 Jan 2024 12:27:57 -0000	1.134
> +++ netinet6/in6_pcb.c	2 Feb 2024 20:53:48 -0000
> @@ -519,7 +519,7 @@ in6_pcbnotify(struct inpcbtable *table, 
>  		    !(inp->inp_route.ro_rt->rt_flags & RTF_HOST)) {
>  			struct sockaddr_in6 *dst6;
>  
> -			dst6 = satosin6(&inp->inp_route.ro_dst);
> +			dst6 = &inp->inp_route6.ro_dst;
>  			if (IN6_ARE_ADDR_EQUAL(&dst6->sin6_addr,
>  			    &dst->sin6_addr))
>  				goto do_notify;