On 26/11/14(Wed) 13:32, Martin Pieuchot wrote:
> On 25/11/14(Tue) 15:16, Todd C. Miller wrote:
> > On Tue, 25 Nov 2014 16:43:16 +0100, Martin Pieuchot wrote:
> >
> > > Diff below removes the non-needed usages of "struct route" & friends in
> > > pf.c, any comment or ok?
> >
> > You are missing some initializations of rt to NULL, comments inline.
>
> Thanks for the reviews todd! This is much appreciated now that network
> hacker is a critically endangered specie :)
>
> You'll find a corrected diff below. Since I was asked to give more
> information about this change here's the story:
>
> We have currently two structures used to cache a route entry in order to
> avoid supplementary route lookups: "struct route" and "struct route_in6".
>
> These structures store a pointer to a rtentry and the destination of
> this route. Yes this is duplicated information. There are mainly two
> problems with this design. First of all the fact that we have a
> structure per AF, when a rtentry is AF agnostic, which results in a lot
> of #ifdefs & cast. But the main reason to directly use a "struct rtentry"
> instead of these structures is to strengthen & unify the code checking
> for the validity of a route. I'd like at least to stop checking against
> a cached destination, I'd like to add a check for stall ifa and I'd like
> to add a check for reference counters.
>
> So the first move is to stop using such structure when we don't want to
> keep a reference on a route entry, then I'll modify the APIs keeping a
> reference and finally add more checks.
And know with the correct diff...
Index: net/pf.c
===================================================================
RCS file: /home/ncvs/src/sys/net/pf.c,v
retrieving revision 1.896
diff -u -p -r1.896 pf.c
--- net/pf.c 20 Nov 2014 13:54:24 -0000 1.896
+++ net/pf.c 26 Nov 2014 12:03:13 -0000
@@ -2952,42 +2952,36 @@ pf_calc_mss(struct pf_addr *addr, sa_fam
{
#ifdef INET
struct sockaddr_in *dst;
- struct route ro;
#endif /* INET */
#ifdef INET6
struct sockaddr_in6 *dst6;
- struct route_in6 ro6;
#endif /* INET6 */
struct rtentry *rt = NULL;
+ struct sockaddr_storage ss;
int hlen;
u_int16_t mss = tcp_mssdflt;
+ memset(&ss, 0, sizeof(ss));
+
switch (af) {
#ifdef INET
case AF_INET:
hlen = sizeof(struct ip);
- bzero(&ro, sizeof(ro));
- dst = (struct sockaddr_in *)&ro.ro_dst;
+ dst = (struct sockaddr_in *)&ss;
dst->sin_family = AF_INET;
dst->sin_len = sizeof(*dst);
dst->sin_addr = addr->v4;
- ro.ro_tableid = rtableid;
- ro.ro_rt = rtalloc(&ro.ro_dst, RT_REPORT, ro.ro_tableid);
- rt = ro.ro_rt;
+ rt = rtalloc(sintosa(dst), RT_REPORT, rtableid);
break;
#endif /* INET */
#ifdef INET6
case AF_INET6:
hlen = sizeof(struct ip6_hdr);
- bzero(&ro6, sizeof(ro6));
- dst6 = (struct sockaddr_in6 *)&ro6.ro_dst;
+ dst6 = (struct sockaddr_in6 *)&ss;
dst6->sin6_family = AF_INET6;
dst6->sin6_len = sizeof(*dst6);
dst6->sin6_addr = addr->v6;
- ro6.ro_tableid = rtableid;
- ro6.ro_rt = rtalloc(sin6tosa(&ro6.ro_dst), RT_REPORT,
- ro6.ro_tableid);
- rt = ro6.ro_rt;
+ rt = rtalloc(sin6tosa(dst6), RT_REPORT, rtableid);
break;
#endif /* INET6 */
}
@@ -5396,25 +5390,22 @@ int
pf_routable(struct pf_addr *addr, sa_family_t af, struct pfi_kif *kif,
int rtableid)
{
+ struct sockaddr_storage ss;
struct sockaddr_in *dst;
int ret = 1;
int check_mpath;
#ifdef INET6
struct sockaddr_in6 *dst6;
- struct route_in6 ro;
-#else
- struct route ro;
#endif
- struct rtentry *rt;
+ struct rtentry *rt = NULL;
struct ifnet *ifp;
check_mpath = 0;
- bzero(&ro, sizeof(ro));
- ro.ro_tableid = rtableid;
+ memset(&ss, 0, sizeof(ss));
switch (af) {
#ifdef INET
case AF_INET:
- dst = (struct sockaddr_in *)&ro.ro_dst;
+ dst = (struct sockaddr_in *)&ss;
dst->sin_family = AF_INET;
dst->sin_len = sizeof(*dst);
dst->sin_addr = addr->v4;
@@ -5430,7 +5421,7 @@ pf_routable(struct pf_addr *addr, sa_fam
*/
if (IN6_IS_SCOPE_EMBED(&addr->v6))
goto out;
- dst6 = &ro.ro_dst;
+ dst6 = (struct sockaddr_in6 *)&ss;
dst6->sin6_family = AF_INET6;
dst6->sin6_len = sizeof(*dst6);
dst6->sin6_addr = addr->v6;
@@ -5444,10 +5435,8 @@ pf_routable(struct pf_addr *addr, sa_fam
if (kif != NULL && kif->pfik_ifp->if_type == IFT_ENC)
goto out;
- ro.ro_rt = rtalloc((struct sockaddr *)&ro.ro_dst, RT_REPORT,
- ro.ro_tableid);
-
- if (ro.ro_rt != NULL) {
+ rt = rtalloc((struct sockaddr *)&ss, RT_REPORT, rtableid);
+ if (rt != NULL) {
/* No interface given, this is a no-route check */
if (kif == NULL)
goto out;
@@ -5459,7 +5448,6 @@ pf_routable(struct pf_addr *addr, sa_fam
/* Perform uRPF check if passed input interface */
ret = 0;
- rt = ro.ro_rt;
do {
if (rt->rt_ifp->if_type == IFT_CARP)
ifp = rt->rt_ifp->if_carpdev;
@@ -5473,8 +5461,8 @@ pf_routable(struct pf_addr *addr, sa_fam
} else
ret = 0;
out:
- if (ro.ro_rt != NULL)
- rtfree(ro.ro_rt);
+ if (rt != NULL)
+ rtfree(rt);
return (ret);
}
@@ -5482,21 +5470,19 @@ int
pf_rtlabel_match(struct pf_addr *addr, sa_family_t af, struct pf_addr_wrap *aw,
int rtableid)
{
+ struct sockaddr_storage ss;
struct sockaddr_in *dst;
#ifdef INET6
struct sockaddr_in6 *dst6;
- struct route_in6 ro;
-#else
- struct route ro;
#endif
+ struct rtentry *rt;
int ret = 0;
- bzero(&ro, sizeof(ro));
- ro.ro_tableid = rtableid;
+ memset(&ss, 0, sizeof(ss));
switch (af) {
#ifdef INET
case AF_INET:
- dst = (struct sockaddr_in *)(&ro.ro_dst);
+ dst = (struct sockaddr_in *)&ss;
dst->sin_family = AF_INET;
dst->sin_len = sizeof(*dst);
dst->sin_addr = addr->v4;
@@ -5504,7 +5490,7 @@ pf_rtlabel_match(struct pf_addr *addr, s
#endif /* INET */
#ifdef INET6
case AF_INET6:
- dst6 = &ro.ro_dst;
+ dst6 = (struct sockaddr_in6 *)&ss;
dst6->sin6_family = AF_INET6;
dst6->sin6_len = sizeof(*dst6);
dst6->sin6_addr = addr->v6;
@@ -5512,13 +5498,11 @@ pf_rtlabel_match(struct pf_addr *addr, s
#endif /* INET6 */
}
- ro.ro_rt = rtalloc((struct sockaddr *)&ro.ro_dst, RT_REPORT,
- ro.ro_tableid);
-
- if (ro.ro_rt != NULL) {
- if (ro.ro_rt->rt_labelid == aw->v.rtlabel)
+ rt = rtalloc((struct sockaddr *)&ss, RT_REPORT|RT_RESOLVE, rtableid);
+ if (rt != NULL) {
+ if (rt->rt_labelid == aw->v.rtlabel)
ret = 1;
- rtfree(ro.ro_rt);
+ rtfree(rt);
}
return (ret);
@@ -5530,14 +5514,14 @@ pf_route(struct mbuf **m, struct pf_rule
struct pf_state *s)
{
struct mbuf *m0, *m1;
- struct route iproute;
- struct route *ro = NULL;
- struct sockaddr_in *dst;
+ struct sockaddr_in *dst, sin;
+ struct rtentry *rt = NULL;
struct ip *ip;
struct ifnet *ifp = NULL;
struct pf_addr naddr;
struct pf_src_node *sns[PF_SN_MAX];
int error = 0;
+ unsigned int rtableid;
#ifdef IPSEC
struct m_tag *mtag;
#endif /* IPSEC */
@@ -5569,27 +5553,25 @@ pf_route(struct mbuf **m, struct pf_rule
ip = mtod(m0, struct ip *);
- ro = &iproute;
- bzero((caddr_t)ro, sizeof(*ro));
- dst = satosin(&ro->ro_dst);
+ memset(&sin, 0, sizeof(sin));
+ dst = &sin;
dst->sin_family = AF_INET;
dst->sin_len = sizeof(*dst);
dst->sin_addr = ip->ip_dst;
- ro->ro_tableid = m0->m_pkthdr.ph_rtableid;
+ rtableid = m0->m_pkthdr.ph_rtableid;
if (!r->rt) {
- ro->ro_rt = rtalloc(&ro->ro_dst, RT_REPORT|RT_RESOLVE,
- ro->ro_tableid);
- if (ro->ro_rt == 0) {
+ rt = rtalloc(sintosa(dst), RT_REPORT|RT_RESOLVE, rtableid);
+ if (rt == NULL) {
ipstat.ips_noroute++;
goto bad;
}
- ifp = ro->ro_rt->rt_ifp;
- ro->ro_rt->rt_use++;
+ ifp = rt->rt_ifp;
+ rt->rt_use++;
- if (ro->ro_rt->rt_flags & RTF_GATEWAY)
- dst = satosin(ro->ro_rt->rt_gateway);
+ if (rt->rt_flags & RTF_GATEWAY)
+ dst = satosin(rt->rt_gateway);
m0->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
} else {
@@ -5695,8 +5677,8 @@ pf_route(struct mbuf **m, struct pf_rule
done:
if (r->rt != PF_DUPTO)
*m = NULL;
- if (ro == &iproute && ro->ro_rt)
- rtfree(ro->ro_rt);
+ if (rt != NULL)
+ rtfree(rt);
return;
bad:
@@ -5711,13 +5693,12 @@ pf_route6(struct mbuf **m, struct pf_rul
struct pf_state *s)
{
struct mbuf *m0;
- struct route_in6 ip6route;
- struct route_in6 *ro;
- struct sockaddr_in6 *dst;
+ struct sockaddr_in6 *dst, sin6;
struct ip6_hdr *ip6;
struct ifnet *ifp = NULL;
struct pf_addr naddr;
struct pf_src_node *sns[PF_SN_MAX];
+ unsigned int rtableid;
if (m == NULL || *m == NULL || r == NULL ||
(dir != PF_IN && dir != PF_OUT) || oifp == NULL)
@@ -5745,13 +5726,12 @@ pf_route6(struct mbuf **m, struct pf_rul
}
ip6 = mtod(m0, struct ip6_hdr *);
- ro = &ip6route;
- bzero((caddr_t)ro, sizeof(*ro));
- dst = (struct sockaddr_in6 *)&ro->ro_dst;
+ memset(&sin6, 0, sizeof(sin6));
+ dst = &sin6;
dst->sin6_family = AF_INET6;
dst->sin6_len = sizeof(*dst);
dst->sin6_addr = ip6->ip6_dst;
- ro->ro_tableid = m0->m_pkthdr.ph_rtableid;
+ rtableid = m0->m_pkthdr.ph_rtableid;
if (!r->rt) {
m0->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
