On Fri, Oct 23, 2015 at 05:45:31PM +0200, Martin Pieuchot wrote:
> This diff adds a small define to be able to convert code dereferencing
> "rt_ifp" to if_get()+if_put(). This will allows us to guarantee that
> ``ifp'' got from route entries are used with a proper reference count.
>
> If the direction is ok I'll commit the .h.
OK with the direction.
> I also accept reviews on the chunks below.
One concern: You call if_get()/if_put() from db_show_rtentry().
When dropped into ddb, the kernel may be in a fragile state. So I
would not recommend to call SRP locking code. Could we just print
the interface index?
otherwise OK bluhm@
>
> Index: net/route.h
> ===================================================================
> RCS file: /cvs/src/sys/net/route.h,v
> retrieving revision 1.115
> diff -u -p -r1.115 route.h
> --- net/route.h 7 Oct 2015 10:50:35 -0000 1.115
> +++ net/route.h 23 Oct 2015 15:13:33 -0000
> @@ -103,6 +103,7 @@ struct rtentry {
> #endif
> struct sockaddr *rt_gateway; /* value */
> struct ifnet *rt_ifp; /* the answer: interface to use */
> +#define rt_ifidx rt_ifp->if_index
> struct ifaddr *rt_ifa; /* the answer: interface addr to use */
> caddr_t rt_llinfo; /* pointer to link level info cache or
> to an MPLS structure */
> Index: net/if_spppsubr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_spppsubr.c,v
> retrieving revision 1.141
> diff -u -p -r1.141 if_spppsubr.c
> --- net/if_spppsubr.c 5 Oct 2015 19:05:09 -0000 1.141
> +++ net/if_spppsubr.c 23 Oct 2015 14:53:10 -0000
> @@ -4263,7 +4263,7 @@ sppp_update_gw_walker(struct rtentry *rt
> {
> struct ifnet *ifp = arg;
>
> - if (rt->rt_ifp == ifp) {
> + if (rt->rt_ifidx == ifp->if_index) {
> if (rt->rt_ifa->ifa_dstaddr->sa_family !=
> rt->rt_gateway->sa_family ||
> (rt->rt_flags & RTF_GATEWAY) == 0)
> Index: net/route.c
> ===================================================================
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.259
> diff -u -p -r1.259 route.c
> --- net/route.c 23 Oct 2015 14:48:22 -0000 1.259
> +++ net/route.c 23 Oct 2015 15:00:42 -0000
> @@ -383,20 +383,23 @@ rtfree(struct rtentry *rt)
> void
> rt_sendmsg(struct rtentry *rt, int cmd, u_int rtableid)
> {
> - struct rt_addrinfo info;
> - struct sockaddr_rtlabel sa_rl;
> + struct rt_addrinfo info;
> + struct ifnet *ifp;
> + struct sockaddr_rtlabel sa_rl;
>
> memset(&info, 0, sizeof(info));
> info.rti_info[RTAX_DST] = rt_key(rt);
> info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
> info.rti_info[RTAX_NETMASK] = rt_mask(rt);
> info.rti_info[RTAX_LABEL] = rtlabel_id2sa(rt->rt_labelid, &sa_rl);
> - if (rt->rt_ifp != NULL) {
> - info.rti_info[RTAX_IFP] = sdltosa(rt->rt_ifp->if_sadl);
> + ifp = if_get(rt->rt_ifidx);
> + if (ifp != NULL) {
> + info.rti_info[RTAX_IFP] = sdltosa(ifp->if_sadl);
> info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
> }
>
> - rt_missmsg(cmd, &info, rt->rt_flags, rt->rt_ifp, 0, rtableid);
> + rt_missmsg(cmd, &info, rt->rt_flags, ifp, 0, rtableid);
> + if_put(ifp);
> }
>
> void
> @@ -541,11 +544,12 @@ rtdeletemsg(struct rtentry *rt, u_int ta
> info.rti_info[RTAX_NETMASK] = rt_mask(rt);
> info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
> info.rti_flags = rt->rt_flags;
> - ifp = rt->rt_ifp;
> + ifp = if_get(rt->rt_ifidx);
> error = rtrequest1(RTM_DELETE, &info, rt->rt_priority, &rt, tableid);
> rt_missmsg(RTM_DELETE, &info, info.rti_flags, ifp, error, tableid);
> if (error == 0)
> rtfree(rt);
> + if_put(ifp);
> return (error);
> }
>
> @@ -1061,7 +1065,7 @@ rt_checkgate(struct ifnet *ifp, struct r
> * loops, for example when rt->rt_gwroute points to rt.
> */
> if (((rt->rt_gwroute->rt_flags & (RTF_UP|RTF_GATEWAY)) !=
> - RTF_UP) || (rt->rt_gwroute->rt_ifp != ifp)) {
> + RTF_UP) || (rt->rt_gwroute->rt_ifidx != ifp->if_index)) {
> rtfree(rt->rt_gwroute);
> rt->rt_gwroute = NULL;
> return (EHOSTUNREACH);
> @@ -1616,7 +1620,7 @@ rt_if_remove_rtdelete(struct rtentry *rt
> {
> struct ifnet *ifp = vifp;
>
> - if (rt->rt_ifp == ifp) {
> + if (rt->rt_ifidx == ifp->if_index) {
> int cloning = (rt->rt_flags & RTF_CLONING);
>
> if (rtdeletemsg(rt, id) == 0 && cloning)
> @@ -1658,7 +1662,7 @@ rt_if_linkstate_change(struct rtentry *r
> {
> struct ifnet *ifp = arg;
>
> - if (rt->rt_ifp != ifp)
> + if (rt->rt_ifidx != ifp->if_index)
> return (0);
>
> /* Local routes are always usable. */
> @@ -1745,6 +1749,8 @@ db_print_ifa(struct ifaddr *ifa)
> int
> db_show_rtentry(struct rtentry *rt, void *w, unsigned int id)
> {
> + struct ifnet *ifp;
> +
> db_printf("rtentry=%p", rt);
>
> db_printf(" flags=0x%x refcnt=%d use=%llu expire=%lld rtableid=%u\n",
> @@ -1754,11 +1760,13 @@ db_show_rtentry(struct rtentry *rt, void
> db_printf(" mask="); db_print_sa(rt_mask(rt));
> db_printf(" gw="); db_print_sa(rt->rt_gateway);
>
> - db_printf(" ifp=%p ", rt->rt_ifp);
> - if (rt->rt_ifp)
> - db_printf("(%s)", rt->rt_ifp->if_xname);
> + db_printf(" ifidx=%u ", rt->rt_ifidx);
> + ifp = if_get(rt->rt_ifidx);
> + if (ifp != NULL)
> + db_printf("(%s)", ifp->if_xname);
> else
> db_printf("(NULL)");
> + if_put(ifp);
>
> db_printf(" ifa=%p\n", rt->rt_ifa);
> db_print_ifa(rt->rt_ifa);
> Index: net/rtsock.c
> ===================================================================
> RCS file: /cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.175
> diff -u -p -r1.175 rtsock.c
> --- net/rtsock.c 23 Oct 2015 13:41:41 -0000 1.175
> +++ net/rtsock.c 23 Oct 2015 15:00:26 -0000
> @@ -603,7 +603,7 @@ route_output(struct mbuf *m, ...)
> &saved_nrt->rt_rmx);
> /* write back the priority the kernel used */
> rtm->rtm_priority = saved_nrt->rt_priority & RTP_MASK;
> - rtm->rtm_index = saved_nrt->rt_ifp->if_index;
> + rtm->rtm_index = saved_nrt->rt_ifidx;
> rtm->rtm_flags = saved_nrt->rt_flags;
> rtfree(saved_nrt);
> }
> @@ -705,8 +705,8 @@ report:
> #endif
> info.rti_info[RTAX_IFP] = NULL;
> info.rti_info[RTAX_IFA] = NULL;
> - if (rtm->rtm_addrs & (RTA_IFP | RTA_IFA) &&
> - (ifp = rt->rt_ifp) != NULL) {
> + ifp = if_get(rt->rt_ifidx);
> + if (ifp != NULL && rtm->rtm_addrs & (RTA_IFP|RTA_IFA)) {
> info.rti_info[RTAX_IFP] = sdltosa(ifp->if_sadl);
> info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
> if (ifp->if_flags & IFF_POINTOPOINT)
> @@ -716,6 +716,7 @@ report:
> info.rti_info[RTAX_BRD] = NULL;
> rtm->rtm_index = ifp->if_index;
> }
> + if_put(ifp);
> len = rt_msg2(rtm->rtm_type, RTM_VERSION, &info, NULL,
> NULL);
> if (len > rtm->rtm_msglen) {
> @@ -828,7 +829,7 @@ report:
>
> rt_setmetrics(rtm->rtm_inits, &rtm->rtm_rmx,
> &rt->rt_rmx);
> - rtm->rtm_index = rt->rt_ifp->if_index;
> + rtm->rtm_index = rt->rt_ifidx;
> rtm->rtm_priority = rt->rt_priority & RTP_MASK;
> rtm->rtm_flags = rt->rt_flags;
> if (rt->rt_ifa && rt->rt_ifa->ifa_rtrequest)
> @@ -1199,6 +1200,7 @@ sysctl_dumpentry(struct rtentry *rt, voi
> struct walkarg *w = v;
> int error = 0, size;
> struct rt_addrinfo info;
> + struct ifnet *ifp;
> #ifdef MPLS
> struct sockaddr_mpls sa_mpls;
> #endif
> @@ -1223,12 +1225,14 @@ sysctl_dumpentry(struct rtentry *rt, voi
> info.rti_info[RTAX_DST] = rt_key(rt);
> info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
> info.rti_info[RTAX_NETMASK] = rt_mask(rt);
> - if (rt->rt_ifp) {
> - info.rti_info[RTAX_IFP] = sdltosa(rt->rt_ifp->if_sadl);
> + ifp = if_get(rt->rt_ifidx);
> + if (ifp != NULL) {
> + info.rti_info[RTAX_IFP] = sdltosa(ifp->if_sadl);
> info.rti_info[RTAX_IFA] = rt->rt_ifa->ifa_addr;
> - if (rt->rt_ifp->if_flags & IFF_POINTOPOINT)
> + if (ifp->if_flags & IFF_POINTOPOINT)
> info.rti_info[RTAX_BRD] = rt->rt_ifa->ifa_dstaddr;
> }
> + if_put(ifp);
> info.rti_info[RTAX_LABEL] = rtlabel_id2sa(rt->rt_labelid, &sa_rl);
> #ifdef MPLS
> if (rt->rt_flags & RTF_MPLS) {
> @@ -1252,7 +1256,7 @@ sysctl_dumpentry(struct rtentry *rt, voi
> rt_getmetrics(&rt->rt_rmx, &rtm->rtm_rmx);
> /* Do not account the routing table's reference. */
> rtm->rtm_rmx.rmx_refcnt = rt->rt_refcnt - 1;
> - rtm->rtm_index = rt->rt_ifp->if_index;
> + rtm->rtm_index = rt->rt_ifidx;
> rtm->rtm_addrs = info.rti_addrs;
> rtm->rtm_tableid = id;
> #ifdef MPLS
