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);
> }
>
> /*
