From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: struct rtentry reorder
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Sat, 15 Mar 2025 18:57:44 +0300

> On 13 Mar 2025, at 23:19, Alexander Bluhm <bluhm@openbsd.org> wrote:
> 
> Hi,
> 
> I see races in our routing code.  The union of rt_gwroute and
> rt_cachecnt makes debugging harder.  In theory the RTF_GATEWAY flag
> distinguishes which field is relevant, but that does not help when
> hunting bugs in this area.  The benefit of saving 4 bytes in struct
> rtentry is minimal.
> 
> Break up the RT_gw union.  To keep memory compact, sort pointers
> to the beginning and integres to the end of struct rtentry.
> 
> ok?
> 

Yes please. This RT_gw union potentially is very dangerous.

> bluhm
> 
> Index: net/route.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v
> diff -u -p -r1.215 route.h
> --- net/route.h	7 Mar 2025 10:18:15 -0000	1.215
> +++ net/route.h	13 Mar 2025 20:07:28 -0000
> @@ -116,25 +116,21 @@ struct rttimer;
>  */
> 
> struct rtentry {
> -	struct mutex	 rt_mtx;
> 	struct sockaddr	*rt_dest;	/* [I] destination */
> 	SRPL_ENTRY(rtentry) rt_next;	/* [R] next mpath entry to our dst */
> 	struct sockaddr	*rt_gateway;	/* [X] gateway address */
> 	struct ifaddr	*rt_ifa;	/* [N] interface addr to use */
> 	caddr_t		 rt_llinfo;	/* [L] pointer to link level info or
> 					   an MPLS structure */
> -	union {
> -		struct rtentry	*_nh;	/* [X] rtentry for rt_gateway */
> -		unsigned int	 _ref;	/* [r] # gateway rtentry refs */
> -	} RT_gw;
> -#define rt_gwroute	 RT_gw._nh
> -#define rt_cachecnt	 RT_gw._ref
> +	struct rtentry	*rt_gwroute;	/* [X] rtentry for rt_gateway */
> 	struct rtentry	*rt_parent;	/* [N] if cloned, parent rtentry */
> 	LIST_HEAD(, rttimer) rt_timer;  /* queue of timeouts for misc funcs */
> +	struct mutex	 rt_mtx;	/* lock members of this struct */
> +	struct refcnt	 rt_refcnt;	/* # held references */
> 	struct rt_kmetrics rt_rmx;	/* metrics used by rx'ing protocols */
> +	unsigned int	 rt_cachecnt;	/* [r] # gateway rtentry refs */
> 	unsigned int	 rt_ifidx;	/* [N] interface to use */
> 	unsigned int	 rt_flags;	/* [X] up/down?, host/net */
> -	struct refcnt	 rt_refcnt;	/* # held references */
> 	int		 rt_plen;	/* [I] prefix length */
> 	uint16_t	 rt_labelid;	/* [N] route label ID */
> 	uint8_t		 rt_priority;	/* [N] routing priority to use */
>