On Mon, Oct 26, 2015 at 07:49:01PM +0100, Martin Pieuchot wrote: > This diff does two things. > > First it changes ip_ours() to no longer rely on ``rt_ifa''. The problem > here is that the route entry reference acts as a proxy for ``ia''. So > you cannot dereference ``ia'' *after* calling rtfree(9). I find this > very error prone, so I rewrote the function to not use ``rt_ifa'' at > all. > > Second it puts a KERNEL_LOCK() around the ``ifp->if_addrlist'' > iteration. This is a hint that accessing this list isn't MP-safe. I > don't want to bother turning this list into a SRPLIST at least for the > moment because it isn't use frequently. Remember that this case is > only execute when you don't have a pf state nor an existing route entry. > There's two others code path accessing this list in ip_output() but > *not* when a route entry is provided, so we will still be running under > KERNEL_LOCK() anyway. > > Ok?
OK bluhm@ > > Index: netinet/ip_input.c > =================================================================== > RCS file: /cvs/src/sys/netinet/ip_input.c,v > retrieving revision 1.259 > diff -u -p -r1.259 ip_input.c > --- netinet/ip_input.c 26 Oct 2015 15:49:13 -0000 1.259 > +++ netinet/ip_input.c 26 Oct 2015 18:27:15 -0000 > @@ -646,9 +646,9 @@ bad: > int > in_ouraddr(struct mbuf *m, struct ifnet *ifp, struct in_addr ina) > { > - struct in_ifaddr *ia = NULL; > struct rtentry *rt; > struct sockaddr_in sin; > + int match = 0; > #if NPF > 0 > struct pf_state_key *key; > > @@ -671,11 +671,25 @@ in_ouraddr(struct mbuf *m, struct ifnet > sin.sin_family = AF_INET; > sin.sin_addr = ina; > rt = rtalloc(sintosa(&sin), 0, m->m_pkthdr.ph_rtableid); > - if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL|RTF_BROADCAST)) > - ia = ifatoia(rt->rt_ifa); > + if (rtisvalid(rt)) { > + if (ISSET(rt->rt_flags, RTF_LOCAL)) > + match = 1; > + > + /* > + * If directedbcast is enabled we only consider it local > + * if it is received on the interface with that address. > + */ > + if (ISSET(rt->rt_flags, RTF_BROADCAST) && > + (!ip_directedbcast || rt->rt_ifidx == ifp->if_index)) { > + match = 1; > + > + /* Make sure M_BCAST is set */ > + m->m_flags |= M_BCAST; > + } > + } > rtfree(rt); > > - if (ia == NULL) { > + if (!match) { > struct ifaddr *ifa; > > /* > @@ -695,33 +709,21 @@ in_ouraddr(struct mbuf *m, struct ifnet > * interface, and that M_BCAST will only be set on a BROADCAST > * interface. > */ > + KERNEL_LOCK(); > TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) { > if (ifa->ifa_addr->sa_family != AF_INET) > continue; > > if (IN_CLASSFULBROADCAST(ina.s_addr, > - ifatoia(ifa)->ia_addr.sin_addr.s_addr)) > - return (1); > + ifatoia(ifa)->ia_addr.sin_addr.s_addr)) { > + match = 1; > + break; > + } > } > - > - return (0); > - } > - > - if (ina.s_addr != ia->ia_addr.sin_addr.s_addr) { > - /* > - * This matches a broadcast address on one of our interfaces. > - * If directedbcast is enabled we only consider it local if it > - * is received on the interface with that address. > - */ > - if (ip_directedbcast && ia->ia_ifp != ifp) > - return (0); > - > - /* Make sure M_BCAST is set */ > - if (m) > - m->m_flags |= M_BCAST; > + KERNEL_UNLOCK(); > } > > - return (1); > + return (match); > } > > /*