Re: kern/54150: COMPAT_50 vs NET_RT_IFLIST

[Paul Goyette <paul%whooppee.com@localhost>]

The following reply was made to PR kern/54150; it has been noted by GNATS.

From: Paul Goyette <paul%whooppee.com@localhost>
To: Robert Elz <kre%munnari.OZ.AU@localhost>
Cc: gnats-bugs%netbsd.org@localhost, Roy Marples <roy%marples.name@localhost>, 
    Matthew Green <mrg%netbsd.org@localhost>
Subject: Re: kern/54150: COMPAT_50 vs NET_RT_IFLIST
Date: Fri, 3 May 2019 13:16:14 +0800 (PST)

 I think that the new hook function compat_50_iflist_addr() could be 
 declared static, and thus you wouldn't need to add it to net/if.h
 
 But otherwise it looks reasonable to me.
 
 
 On Fri, 3 May 2019, Robert Elz wrote:
 
 > OK ... the patch below seems to fix this one - for the specific case
 > of NetBSD 5 binaries running on a -current kernel (a patch for -8 would
 > be quite different, but along the same lines).
 >
 > Whether this correctly handles older (older than 1.4?) binaries, or even
 > NetBSD 7 (or 8) binaries I have not tested.   But it should not be worse
 > than it was, I think.
 >
 > The primary problems were the incorrect RTM_VERSION in the messages,
 > and that struct ifa_msghdr has a different layout (field order changed)
 > in NetBSD 5 & NetBSD 7 - so returning a NetBSD 7 format struct to a
 > NetBSD 5 client does not work well.
 >
 > I'd appreciate it if everyone who has looked at this problem could
 > take a look at this, and send comments before it gets committed.
 > (Hence I have added a few cc's to this message, so people can get
 > the patch in a non-gnats-mangled format ... anyone else who wants
 > that send me a message off list).
 >
 > I am particularly wary of the RTS_CTASSERT() stuff in the new
 > rt_getvers() function - that function is more or less a clone of
 > rt_getlen() (which has these same RTS_CTASSERTs) and seems to
 > work ok, on amd64 anyway.   Whether this will work on i386 (and
 > other 32 bit systems) I have no idea.
 >
 > The companion PR (for "route monitor") I have not yet investigated
 > (except to discover that it is not as simple as I thought it might be.)
 >
 > kre
 >
 > ps: IMNSHO, way too many files need to be touched to add a new hook.
 >
 > I would suggest that we need a better way, where it just needs to be
 > defined (one place) and then used (wherever needed) and nothing else
 > needs to be manually edited - all the bookkeeping should be handled
 > automatically.
 >
 >
 > Index: compat/common/rtsock_50.c
 > ===================================================================
 > RCS file: /cvsroot/src/sys/compat/common/rtsock_50.c,v
 > retrieving revision 1.13
 > diff -u -r1.13 rtsock_50.c
 > --- compat/common/rtsock_50.c	29 Apr 2019 16:12:30 -0000	1.13
 > +++ compat/common/rtsock_50.c	3 May 2019 04:26:38 -0000
 > @@ -151,6 +151,28 @@
 > 	return 0;
 > }
 >
 > +int
 > +compat_50_iflist_addr(struct rt_walkarg *w, struct ifaddr *ifa,
 > +     struct rt_addrinfo *info)
 > +{
 > +	int len, error;
 > +
 > +	if ((error = rt_msg3(RTM_ONEWADDR, info, 0, w, &len)))
 > +		return error;
 > +	if (w->w_where && w->w_tmem && w->w_needed <= 0) {
 > +		struct ifa_msghdr50 *ifam;
 > +
 > +		ifam = (struct ifa_msghdr50 *)w->w_tmem;
 > +		ifam->ifam_index = ifa->ifa_ifp->if_index;
 > +		ifam->ifam_flags = ifa->ifa_flags;
 > +		ifam->ifam_metric = ifa->ifa_metric;
 > +		ifam->ifam_addrs = info->rti_addrs;
 > +		if ((error = copyout(w->w_tmem, w->w_where, len)) == 0)
 > +			w->w_where = (char *)w->w_where + len;
 > +	}
 > +	return error;
 > +}
 > +
 > void
 > rtsock_50_init(void)
 > {
 > @@ -170,6 +192,8 @@
 > 	    compat_50_rt_ifannouncemsg);
 > 	MODULE_HOOK_SET(rtsock_rt_ieee80211msg_50_hook, "rts_50",
 > 	    compat_50_rt_ieee80211msg);
 > +	MODULE_HOOK_SET(rtsock_iflist_addr_50_hook, "rts_50",
 > +	    compat_50_iflist_addr);
 > 	sysctl_net_route_setup(&clog, PF_OROUTE, "ortable");
 > }
 >
 > @@ -187,4 +211,5 @@
 > 	MODULE_HOOK_UNSET(rtsock_rt_addrmsg_50_hook);
 > 	MODULE_HOOK_UNSET(rtsock_rt_ifannouncemsg_50_hook);
 > 	MODULE_HOOK_UNSET(rtsock_rt_ieee80211msg_50_hook);
 > +	MODULE_HOOK_UNSET(rtsock_iflist_addr_50_hook);
 > }
 > Index: compat/net/if.h
 > ===================================================================
 > RCS file: /cvsroot/src/sys/compat/net/if.h,v
 > retrieving revision 1.4
 > diff -u -r1.4 if.h
 > --- compat/net/if.h	21 Sep 2016 10:50:23 -0000	1.4
 > +++ compat/net/if.h	3 May 2019 04:26:38 -0000
 > @@ -171,6 +171,9 @@
 > 	int	ifam_metric;	/* value of ifa_metric */
 > };
 >
 > +int compat_50_iflist_addr(struct rt_walkarg *, struct ifaddr *,
 > +    struct rt_addrinfo *);
 > +
 > /*
 >  * Message format announcing the arrival or departure of a network interface.
 >  */
 > Index: kern/compat_stub.c
 > ===================================================================
 > RCS file: /cvsroot/src/sys/kern/compat_stub.c,v
 > retrieving revision 1.11
 > diff -u -r1.11 compat_stub.c
 > --- kern/compat_stub.c	29 Apr 2019 16:12:30 -0000	1.11
 > +++ kern/compat_stub.c	3 May 2019 04:26:46 -0000
 > @@ -207,6 +207,7 @@
 > struct rtsock_rt_addrmsg_src_50_hook_t rtsock_rt_addrmsg_src_50_hook;
 > struct rtsock_rt_addrmsg_50_hook_t rtsock_rt_addrmsg_50_hook;
 > struct rtsock_rt_ieee80211msg_50_hook_t rtsock_rt_ieee80211msg_50_hook;
 > +struct rtsock_iflist_addr_50_hook_t rtsock_iflist_addr_50_hook;
 >
 > /*
 >  * rtsock 70 compatability
 > Index: net/rtsock.c
 > ===================================================================
 > RCS file: /cvsroot/src/sys/net/rtsock.c,v
 > retrieving revision 1.249
 > diff -u -r1.249 rtsock.c
 > --- net/rtsock.c	29 Apr 2019 05:42:09 -0000	1.249
 > +++ net/rtsock.c	3 May 2019 04:26:49 -0000
 > @@ -364,9 +364,12 @@
 > 				error = sysctl_iflist_addr(w, ifa, &info);
 > 				break;
 > 			case NET_RT_OIFLIST:
 > +				MODULE_HOOK_CALL(rtsock_iflist_70_hook,
 > +				    (w, ifa, &info), enosys(), error);
 > +				break;
 > 			case NET_RT_OOIFLIST:
 > 			case NET_RT_OOOIFLIST:
 > -				MODULE_HOOK_CALL(rtsock_iflist_70_hook,
 > +				MODULE_HOOK_CALL(rtsock_iflist_addr_50_hook,
 > 				    (w, ifa, &info), enosys(), error);
 > 				break;
 > 			default:
 > Index: net/rtsock_shared.c
 > ===================================================================
 > RCS file: /cvsroot/src/sys/net/rtsock_shared.c,v
 > retrieving revision 1.8
 > diff -u -r1.8 rtsock_shared.c
 > --- net/rtsock_shared.c	29 Apr 2019 16:12:30 -0000	1.8
 > +++ net/rtsock_shared.c	3 May 2019 04:26:50 -0000
 > @@ -1159,6 +1159,67 @@
 > }
 >
 >
 > +static int
 > +rt_getvers(int type)
 > +{
 > +	RTS_CTASSERT(__alignof(struct ifa_msghdr) >= sizeof(uint64_t));
 > +	RTS_CTASSERT(__alignof(struct if_msghdr) >= sizeof(uint64_t));
 > +	RTS_CTASSERT(__alignof(struct if_announcemsghdr) >= sizeof(uint64_t));
 > +	RTS_CTASSERT(__alignof(struct rt_msghdr) >= sizeof(uint64_t));
 > +
 > +	switch (type) {
 > +	case RTM_ODELADDR:
 > +	case RTM_ONEWADDR:
 > +	case RTM_OCHGADDR:
 > +		if (rtsock_iflist_70_hook.hooked)
 > +			return RTM_OVERSION;
 > +		else {
 > +#ifdef RTSOCK_DEBUG
 > +			printf("%s: unsupported RTM type %d\n", __func__, type);
 > +#endif
 > +			return -1;
 > +		}
 > +
 > +	case RTM_DELADDR:
 > +	case RTM_NEWADDR:
 > +	case RTM_CHGADDR:
 > +		return RTM_VERSION;
 > +
 > +	case RTM_OOIFINFO:
 > +		if (rtsock_iflist_14_hook.hooked)
 > +			return RTM_OVERSION;
 > +		else {
 > +#ifdef RTSOCK_DEBUG
 > +			printf("%s: unsupported RTM type RTM_OOIFINFO\n",
 > +			    __func__);
 > +#endif
 > +			return -1;
 > +		}
 > +
 > +	case RTM_OIFINFO:
 > +		if (rtsock_iflist_50_hook.hooked)
 > +			return RTM_OVERSION;
 > +		else {
 > +#ifdef RTSOCK_DEBUG
 > +			printf("%s: unsupported RTM type RTM_OIFINFO\n",
 > +			    __func__);
 > +#endif
 > +			return -1;
 > +		}
 > +
 > +	case RTM_IFINFO:
 > +		return RTM_VERSION;
 > +
 > +	case RTM_IFANNOUNCE:
 > +	case RTM_IEEE80211:
 > +		return RTM_VERSION;
 > +
 > +	default:
 > +		return RTM_XVERSION;
 > +	}
 > +}
 > +
 > +
 > struct mbuf *
 > COMPATNAME(rt_msg1)(int type, struct rt_addrinfo *rtinfo, void *data, int datalen)
 > {
 > @@ -1212,7 +1273,7 @@
 > 	if (m->m_pkthdr.len != len)
 > 		goto out;
 > 	rtm->rtm_msglen = len;
 > -	rtm->rtm_version = RTM_XVERSION;
 > +	rtm->rtm_version = rt_getvers(type);
 > 	rtm->rtm_type = type;
 > 	return m;
 > out:
 > @@ -1290,7 +1351,7 @@
 > 	if (cp) {
 > 		struct rt_xmsghdr *rtm = (struct rt_xmsghdr *)cp0;
 >
 > -		rtm->rtm_version = RTM_XVERSION;
 > +		rtm->rtm_version = rt_getvers(type);
 > 		rtm->rtm_type = type;
 > 		rtm->rtm_msglen = len;
 > 	}
 > Index: sys/compat_stub.h
 > ===================================================================
 > RCS file: /cvsroot/src/sys/sys/compat_stub.h,v
 > retrieving revision 1.15
 > diff -u -r1.15 compat_stub.h
 > --- sys/compat_stub.h	29 Apr 2019 16:12:30 -0000	1.15
 > +++ sys/compat_stub.h	3 May 2019 04:26:51 -0000
 > @@ -269,6 +269,8 @@
 > MODULE_HOOK(rtsock_rt_ifannouncemsg_50_hook, void, (struct ifnet *, int));
 > MODULE_HOOK(rtsock_rt_ieee80211msg_50_hook, void,
 >     (struct ifnet *, int, void *, size_t));
 > +MODULE_HOOK(rtsock_iflist_addr_50_hook, int,
 > +    (struct rt_walkarg *, struct ifaddr *, struct rt_addrinfo *));
 >
 > /*
 >  * Hooks for rtsock_70
 >
 >
 >
 > !DSPAM:5ccbc7c0112531254785785!
 >
 >
 
 +--------------------+--------------------------+-----------------------+
 | Paul Goyette       | PGP Key fingerprint:     | E-mail addresses:     |
 | (Retired)          | FA29 0E3B 35AF E8AE 6651 | paul%whooppee.com@localhost     |
 | Software Developer | 0786 F758 55DE 53BA 7731 | pgoyette%netbsd.org@localhost   |
 +--------------------+--------------------------+-----------------------+