On 26/10/15(Mon) 10:45, Vincent Gross wrote:
> regress/sys/net/rdomains still passes with this diff.
MP is hard! Let me comment on your diff, you're taking the right
direction.
> Index: net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.398
> diff -u -p -r1.398 if.c
> --- net/if.c 25 Oct 2015 21:58:04 -0000 1.398
> +++ net/if.c 26 Oct 2015 09:44:10 -0000
> @@ -1143,31 +1143,19 @@ if_congested(void)
You're forgetting the most interesting part of this function! Its
comment:
/*
* Locate an interface based on a complete address.
*/
Historically this function was returning an ``ifp'' and in most of the
cases that's exactly what we want. So the code is dereferencing an
``ifa'' just to get ``ifp''. The diff below changes some of this
call just to give you an idea.
> struct ifaddr *
> ifa_ifwithaddr(struct sockaddr *addr, u_int rtableid)
> {
> - struct ifnet *ifp;
> struct ifaddr *ifa;
> + struct rtentry *rt;
> u_int rdomain;
>
> + /*
> + * Local routes corresponding to ifas are in rdomain's
> + * default rtable.
> + */
> rdomain = rtable_l2(rtableid);
> - TAILQ_FOREACH(ifp, &ifnet, if_list) {
> - if (ifp->if_rdomain != rdomain)
> - continue;
> -
> - TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
> - if (ifa->ifa_addr->sa_family != addr->sa_family)
> - continue;
> -
> - if (equal(addr, ifa->ifa_addr))
> - return (ifa);
> -
> - /* IPv6 doesn't have broadcast */
> - if ((ifp->if_flags & IFF_BROADCAST) &&
> - ifa->ifa_broadaddr &&
> - ifa->ifa_broadaddr->sa_len != 0 &&
> - equal(ifa->ifa_broadaddr, addr))
> - return (ifa);
> - }
> - }
> - return (NULL);
> + rt = rtalloc(addr, 0, rdomain);
> + ifa = rt && (rt->rt_flags & RTF_LOCAL) ? rt->rt_ifa : NULL;
Here you're forgetting RTF_BROADCAST and you should probably call
rtisvalid(9) instead of checking for (rt != NULL).
> + rtfree(rt);
> + return ifa;
This is currently correct but that's what we do not want. As soon as
rtfree(9) is called ``ifa'' might be freed.
But let's take a step back. All the ifa_if*() functions are a good hint
that some work has to be done for MP-safeness. Accessing "&ifnet" MUST
not be done in the hot path (only in ioctl path).
There's two types of codes calling ifa_ifwithaddr(). Those that want
an ifp index (like divert) should probably use rt->rt_ifidx. Those that
want a destination should use rt_key(rt) on a RTF_LOCAL|RTF_BROADCAST
route.
I'd better see an audit of all the functions calling ifa_if*() to see
if they are called in hot path and need to be rewritten or if we can
deal with them later.
The diff below is just a hint, I don't want to introduce a new
interface.
Index: net/if.c
===================================================================
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.393
diff -u -p -r1.393 if.c
--- net/if.c 22 Oct 2015 17:48:34 -0000 1.393
+++ net/if.c 23 Oct 2015 13:19:50 -0000
@@ -1138,12 +1138,38 @@ if_congested(void)
/*
* Locate an interface based on a complete address.
*/
+struct ifnet *
+if_withaddr(struct sockaddr *addr, unsigned int rtableid)
+{
+ struct ifnet *ifp;
+ struct ifaddr *ifa;
+ unsigned int rdomain;
+
+ rdomain = rtable_l2(rtableid);
+ TAILQ_FOREACH(ifp, &ifnet, if_list) {
+ if (ifp->if_rdomain != rdomain)
+ continue;
+
+ TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
+ if (ifa->ifa_addr->sa_family != addr->sa_family)
+ continue;
+
+ if (equal(addr, ifa->ifa_addr))
+ return (ifp);
+ }
+ }
+ return (NULL);
+}
+
+/*
+ * Locate an ``ifa'' based on a unicast or broadcast address.
+ */
struct ifaddr *
-ifa_ifwithaddr(struct sockaddr *addr, u_int rtableid)
+ifa_ifwithaddr(struct sockaddr *addr, unsigned int rtableid)
{
struct ifnet *ifp;
struct ifaddr *ifa;
- u_int rdomain;
+ unsigned int rdomain;
rdomain = rtable_l2(rtableid);
TAILQ_FOREACH(ifp, &ifnet, if_list) {
Index: net/route.c
===================================================================
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.258
diff -u -p -r1.258 route.c
--- net/route.c 22 Oct 2015 17:19:38 -0000 1.258
+++ net/route.c 23 Oct 2015 13:27:31 -0000
@@ -443,7 +443,7 @@ rtredirect(struct sockaddr *dst, struct
if (!(flags & RTF_DONE) && rt &&
(!equal(src, rt->rt_gateway) || rt->rt_ifa != ifa))
error = EINVAL;
- else if (ifa_ifwithaddr(gateway, rdomain) != NULL)
+ else if (if_withaddr(gateway, rdomain) != NULL)
error = EHOSTUNREACH;
if (error)
goto done;
Index: net/if_var.h
===================================================================
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.49
diff -u -p -r1.49 if_var.h
--- net/if_var.h 22 Oct 2015 17:48:34 -0000 1.49
+++ net/if_var.h 23 Oct 2015 13:20:14 -0000
@@ -408,6 +408,7 @@ void if_start(struct ifnet *);
int if_enqueue(struct ifnet *, struct mbuf *);
void if_input(struct ifnet *, struct mbuf_list *);
int if_input_local(struct ifnet *, struct mbuf *, sa_family_t);
+struct ifnet *if_withaddr(struct sockaddr *, unsigned int);
void ether_ifattach(struct ifnet *);
void ether_ifdetach(struct ifnet *);
Index: net/if_vxlan.c
===================================================================
RCS file: /cvs/src/sys/net/if_vxlan.c,v
retrieving revision 1.32
diff -u -p -r1.32 if_vxlan.c
--- net/if_vxlan.c 23 Oct 2015 01:19:04 -0000 1.32
+++ net/if_vxlan.c 23 Oct 2015 13:23:41 -0000
@@ -210,7 +210,6 @@ vxlan_multicast_join(struct ifnet *ifp,
{
struct vxlan_softc *sc = ifp->if_softc;
struct ip_moptions *imo = &sc->sc_imo;
- struct ifaddr *ifa;
struct ifnet *mifp;
if (!IN_MULTICAST(dst->sin_addr.s_addr))
@@ -219,8 +218,7 @@ vxlan_multicast_join(struct ifnet *ifp,
if (src->sin_addr.s_addr == INADDR_ANY ||
IN_MULTICAST(src->sin_addr.s_addr))
return (EINVAL);
- if ((ifa = ifa_ifwithaddr(sintosa(src), sc->sc_rdomain)) == NULL ||
- (mifp = ifa->ifa_ifp) == NULL ||
+ if ((mifp = if_withaddr(sintosa(src), sc->sc_rdomain)) == NULL ||
(mifp->if_flags & IFF_MULTICAST) == 0)
return (EADDRNOTAVAIL);
Index: netinet/ip_input.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.258
diff -u -p -r1.258 ip_input.c
--- netinet/ip_input.c 19 Oct 2015 11:59:26 -0000 1.258
+++ netinet/ip_input.c 23 Oct 2015 13:23:24 -0000
@@ -1051,9 +1051,8 @@ ip_dooptions(struct mbuf *m, struct ifne
ipaddr.sin_family = AF_INET;
ipaddr.sin_len = sizeof(ipaddr);
ipaddr.sin_addr = ip->ip_dst;
- ia = ifatoia(ifa_ifwithaddr(sintosa(&ipaddr),
- m->m_pkthdr.ph_rtableid));
- if (ia == NULL) {
+ if (if_withaddr(sintosa(&ipaddr),
+ m->m_pkthdr.ph_rtableid) == NULL) {
if (opt == IPOPT_SSRR) {
type = ICMP_UNREACH;
code = ICMP_UNREACH_SRCFAIL;
@@ -1187,7 +1186,7 @@ ip_dooptions(struct mbuf *m, struct ifne
ipaddr.sin_family = AF_INET;
ipaddr.sin_len = sizeof(ipaddr);
ipaddr.sin_addr = sin;
- if (ifa_ifwithaddr(sintosa(&ipaddr),
+ if (if_withaddr(sintosa(&ipaddr),
m->m_pkthdr.ph_rtableid) == NULL)
continue;
ipt.ipt_ptr += sizeof(struct in_addr);
Index: netinet/ip_divert.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_divert.c,v
retrieving revision 1.38
diff -u -p -r1.38 ip_divert.c
--- netinet/ip_divert.c 9 Sep 2015 20:15:52 -0000 1.38
+++ netinet/ip_divert.c 23 Oct 2015 13:22:10 -0000
@@ -84,7 +84,7 @@ divert_output(struct inpcb *inp, struct
{
struct sockaddr_in *sin;
struct socket *so;
- struct ifaddr *ifa;
+ struct ifnet *ifp;
int error = 0, min_hdrlen = 0, dir;
struct ip *ip;
u_int16_t off;
@@ -140,12 +140,12 @@ divert_output(struct inpcb *inp, struct
if (dir == PF_IN) {
ipaddr.sin_addr = sin->sin_addr;
- ifa = ifa_ifwithaddr(sintosa(&ipaddr), m->m_pkthdr.ph_rtableid);
- if (ifa == NULL) {
+ ifp = if_withaddr(sintosa(&ipaddr), m->m_pkthdr.ph_rtableid);
+ if (ifp == NULL) {
error = EADDRNOTAVAIL;
goto fail;
}
- m->m_pkthdr.ph_ifidx = ifa->ifa_ifp->if_index;
+ m->m_pkthdr.ph_ifidx = ifp->if_index;
/*
* Recalculate IP and protocol checksums for the inbound packet
Index: netinet/ip_output.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.303
diff -u -p -r1.303 ip_output.c
--- netinet/ip_output.c 20 Oct 2015 20:22:42 -0000 1.303
+++ netinet/ip_output.c 23 Oct 2015 13:23:01 -0000
@@ -1324,7 +1324,6 @@ ip_setmoptions(int optname, struct ip_mo
u_int rtableid)
{
struct in_addr addr;
- struct in_ifaddr *ia;
struct ip_mreq *mreq;
struct ifnet *ifp = NULL;
struct ip_moptions *imo = *imop;
@@ -1381,9 +1380,7 @@ ip_setmoptions(int optname, struct ip_mo
sin.sin_len = sizeof(sin);
sin.sin_family = AF_INET;
sin.sin_addr = addr;
- ia = ifatoia(ifa_ifwithaddr(sintosa(&sin), rtableid));
- if (ia && in_hosteq(sin.sin_addr, ia->ia_addr.sin_addr))
- ifp = ia->ia_ifp;
+ ifp = if_withaddr(sintosa(&sin), rtableid);
if (ifp == NULL || (ifp->if_flags & IFF_MULTICAST) == 0) {
error = EADDRNOTAVAIL;
break;
@@ -1451,9 +1448,7 @@ ip_setmoptions(int optname, struct ip_mo
sin.sin_len = sizeof(sin);
sin.sin_family = AF_INET;
sin.sin_addr = mreq->imr_interface;
- ia = ifatoia(ifa_ifwithaddr(sintosa(&sin), rtableid));
- if (ia && in_hosteq(sin.sin_addr, ia->ia_addr.sin_addr))
- ifp = ia->ia_ifp;
+ ifp = if_withaddr(sintosa(&sin), rtableid);
}
/*
* See if we found an interface, and confirm that it
@@ -1546,10 +1541,8 @@ ip_setmoptions(int optname, struct ip_mo
sin.sin_len = sizeof(sin);
sin.sin_family = AF_INET;
sin.sin_addr = mreq->imr_interface;
- ia = ifatoia(ifa_ifwithaddr(sintosa(&sin), rtableid));
- if (ia && in_hosteq(sin.sin_addr, ia->ia_addr.sin_addr))
- ifp = ia->ia_ifp;
- else {
+ ifp = if_withaddr(sintosa(&sin), rtableid);
+ if (ifp == NULL) {
error = EADDRNOTAVAIL;
break;
}
Index: netinet/ip_mroute.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
retrieving revision 1.82
diff -u -p -r1.82 ip_mroute.c
--- netinet/ip_mroute.c 12 Sep 2015 13:34:12 -0000 1.82
+++ netinet/ip_mroute.c 23 Oct 2015 13:09:38 -0000
@@ -827,7 +827,6 @@ add_vif(struct mbuf *m)
{
struct vifctl *vifcp;
struct vif *vifp;
- struct ifaddr *ifa;
struct ifnet *ifp;
struct ifreq ifr;
int error, s;
@@ -857,8 +856,9 @@ add_vif(struct mbuf *m)
#endif
{
sin.sin_addr = vifcp->vifc_lcl_addr;
- ifa = ifa_ifwithaddr(sintosa(&sin), /* XXX */ 0);
- if (ifa == NULL)
+ /* Use the physical interface associated with the address. */
+ ifp = if_withaddr(sintosa(&sin), /* XXX */ 0);
+ if (ifp == NULL)
return (EADDRNOTAVAIL);
}
@@ -881,9 +881,6 @@ add_vif(struct mbuf *m)
}
#endif
} else {
- /* Use the physical interface associated with the address. */
- ifp = ifa->ifa_ifp;
-
/* Make sure the interface supports multicast. */
if ((ifp->if_flags & IFF_MULTICAST) == 0)
return (EOPNOTSUPP);
Index: netinet6/ip6_divert.c
===================================================================
RCS file: /cvs/src/sys/netinet6/ip6_divert.c,v
retrieving revision 1.38
diff -u -p -r1.38 ip6_divert.c
--- netinet6/ip6_divert.c 11 Sep 2015 08:17:06 -0000 1.38
+++ netinet6/ip6_divert.c 23 Oct 2015 13:22:34 -0000
@@ -88,7 +88,7 @@ divert6_output(struct inpcb *inp, struct
{
struct sockaddr_in6 *sin6;
struct socket *so;
- struct ifaddr *ifa;
+ struct ifnet *ifp;
int error = 0, min_hdrlen = 0, nxt = 0, off, dir;
struct ip6_hdr *ip6;
@@ -149,13 +149,12 @@ divert6_output(struct inpcb *inp, struct
if (dir == PF_IN) {
ip6addr.sin6_addr = sin6->sin6_addr;
- ifa = ifa_ifwithaddr(sin6tosa(&ip6addr),
- m->m_pkthdr.ph_rtableid);
- if (ifa == NULL) {
+ ifp = if_withaddr(sin6tosa(&ip6addr), m->m_pkthdr.ph_rtableid);
+ if (ifp == NULL) {
error = EADDRNOTAVAIL;
goto fail;
}
- m->m_pkthdr.ph_ifidx = ifa->ifa_ifp->if_index;
+ m->m_pkthdr.ph_ifidx = ifp->if_index;
/*
* Recalculate the protocol checksum for the inbound packet
