On 17/10/15(Sat) 02:05, Alexander Bluhm wrote:
> On Fri, Oct 16, 2015 at 03:09:03PM +0200, Martin Pieuchot wrote:
> > - if (rt->rt_flags & (RTF_REJECT | RTF_BLACKHOLE) ||
> > - (rt->rt_flags & RTF_UP) == 0) {
> > + if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_REJECT|RTF_BLACKHOLE)) {
>
> Why change to ISSET()? I still don't know which variant I like
> more and the code is very inconsistent. So I don't care much.
Changed back.
> > - if (ro->ro_rt != NULL &&
> > - (ro->ro_rt->rt_flags & (RTF_UP | RTF_HOST)) &&
> > + if (rtisvalid(ro->ro_rt) &&
> > + ISSET(ro->ro_rt->rt_flags, RTF_HOST) &&
>
> This is not equivalent. Before RTF_UP or RTF_HOST was checked,
> now it is RTF_UP and RTF_HOST. Is this change intensional?
It is. This code is related to tcp_mtudisc() which only deals with
RTF_HOST routes. Since here we're updating the MTU of a route (note
the violation layer btw) we must be sure we're not modifying any route.
Only checking for RTF_UP would be wrong. I believe the original "fix"
should have been:
if ((rt->rt_flags & (RTF_UP|RTF_HOST)) == (RTF_UP|RTF_HOST))
Diff below also includes a routing table check in in_selectsrc() to
match your comment in the INET6 version of this diff.
ok?
Index: netinet/in_pcb.c
===================================================================
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.183
diff -u -p -r1.183 in_pcb.c
--- netinet/in_pcb.c 19 Oct 2015 08:49:13 -0000 1.183
+++ netinet/in_pcb.c 19 Oct 2015 09:32:19 -0000
@@ -764,7 +764,7 @@ in_pcbrtentry(struct inpcb *inp)
ro = &inp->inp_route;
/* check if route is still valid */
- if (ro->ro_rt && (ro->ro_rt->rt_flags & RTF_UP) == 0) {
+ if (!rtisvalid(ro->ro_rt)) {
rtfree(ro->ro_rt);
ro->ro_rt = NULL;
}
@@ -857,8 +857,8 @@ in_selectsrc(struct in_addr **insrc, str
* If route is known or can be allocated now,
* our src addr is taken from the i/f, else punt.
*/
- if (ro->ro_rt && ((ro->ro_rt->rt_flags & RTF_UP) == 0 ||
- (satosin(&ro->ro_dst)->sin_addr.s_addr != sin->sin_addr.s_addr))) {
+ if (!rtisvalid(ro->ro_rt) || (ro->ro_tableid != rtableid) ||
+ (satosin(&ro->ro_dst)->sin_addr.s_addr != sin->sin_addr.s_addr)) {
rtfree(ro->ro_rt);
ro->ro_rt = NULL;
}
Index: netinet/ip_icmp.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.141
diff -u -p -r1.141 ip_icmp.c
--- netinet/ip_icmp.c 23 Sep 2015 08:49:46 -0000 1.141
+++ netinet/ip_icmp.c 19 Oct 2015 09:34:15 -0000
@@ -936,18 +936,14 @@ icmp_mtudisc_clone(struct in_addr dst, u
sin.sin_addr = dst;
rt = rtalloc(sintosa(&sin), RT_REPORT|RT_RESOLVE, rtableid);
- if (rt == NULL)
- return (NULL);
/* Check if the route is actually usable */
- if (rt->rt_flags & (RTF_REJECT | RTF_BLACKHOLE) ||
- (rt->rt_flags & RTF_UP) == 0) {
+ if (!rtisvalid(rt) || (rt->rt_flags & (RTF_REJECT|RTF_BLACKHOLE))) {
rtfree(rt);
return (NULL);
}
/* If we didn't get a host route, allocate one */
-
if ((rt->rt_flags & RTF_HOST) == 0) {
struct rtentry *nrt;
struct rt_addrinfo info;
Index: netinet/ip_output.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.301
diff -u -p -r1.301 ip_output.c
--- netinet/ip_output.c 13 Oct 2015 10:16:17 -0000 1.301
+++ netinet/ip_output.c 15 Oct 2015 09:58:41 -0000
@@ -588,8 +588,8 @@ sendit:
* them, there is no way for one to update all its
* routes when the MTU is changed.
*/
- if (ro->ro_rt != NULL &&
- (ro->ro_rt->rt_flags & (RTF_UP | RTF_HOST)) &&
+ if (rtisvalid(ro->ro_rt) &&
+ ISSET(ro->ro_rt->rt_flags, RTF_HOST) &&
!(ro->ro_rt->rt_rmx.rmx_locks & RTV_MTU) &&
(ro->ro_rt->rt_rmx.rmx_mtu > ifp->if_mtu)) {
ro->ro_rt->rt_rmx.rmx_mtu = ifp->if_mtu;
