On 07/08/16 23:35, Alexander Bluhm wrote:
> On Tue, Jun 28, 2016 at 08:30:16AM +0200, Martin Pieuchot wrote:
>> With this diff if your next hop becomes invalid after being cached you'll
>> also need two ICMP6_PACKET_TOO_BIG to restore the MTU, is this wanted?
>
> No, a single ICMP6_PACKET_TOO_BIG packet is enough. I have written
> a test that checks all cases.
>
> https://github.com/bluhm/pmtu-regress
>
> After the issue has been fixed, I would like to commit it to
> /usr/src/regress/sys/netinet/pmtu .
ok mpi@
>> I would prefer if we could cache the gw route in a single place. So
>> I'm wondering if the check for reseting PMTUD makes sense (where it is).
>
> After testing TCP, TCP6, UDP6 I still think my diff is correct. It
> makes sense to reset the PMTU discovery after the gateway route is
> gone.
>
> I the current implementation rtalloc() returns a route with a valid
> gateway, while rtrequest(RTM_ADD) returns a route with an invalid
> gateway. I think both methods should provide a similar route, so
> rt_setgwroute() should fix the gateway route in both functions.
I'm convinced, ok mpi@
>>> Index: net/if_spppsubr.c
>>> ===================================================================
>>> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_spppsubr.c,v
>>> retrieving revision 1.154
>>> diff -u -p -r1.154 if_spppsubr.c
>>> --- net/if_spppsubr.c 14 Jun 2016 20:44:43 -0000 1.154
>>> +++ net/if_spppsubr.c 24 Jun 2016 22:32:28 -0000
>>> @@ -4161,7 +4161,7 @@ sppp_update_gw_walker(struct rtentry *rt
>>> rt->rt_gateway->sa_family ||
>>> !ISSET(rt->rt_flags, RTF_GATEWAY))
>>> return (0); /* do not modify non-gateway routes */
>>> - rt_setgate(rt, rt->rt_ifa->ifa_dstaddr);
>>> + rt_setgate(rt, rt->rt_ifa->ifa_dstaddr, ifp->if_rdomain);
>>> }
>>> return (0);
>>> }
>>> Index: net/route.c
>>> ===================================================================
>>> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
>>> retrieving revision 1.309
>>> diff -u -p -r1.309 route.c
>>> --- net/route.c 14 Jun 2016 09:48:52 -0000 1.309
>>> +++ net/route.c 24 Jun 2016 22:37:45 -0000
>>> @@ -154,6 +154,7 @@ struct pool rttimer_pool; /* pool for r
>>>
>>> void rt_timer_init(void);
>>> int rt_setaddr(struct rtentry *, struct sockaddr *);
>>> +void rt_setgwroute(struct rtentry *, u_int);
>>> int rtflushclone1(struct rtentry *, void *, u_int);
>>> void rtflushclone(unsigned int, struct rtentry *);
>>> int rt_if_remove_rtdelete(struct rtentry *, void *, u_int);
>>> @@ -369,7 +370,7 @@ rtalloc(struct sockaddr *dst, int flags,
>>> struct rtentry *
>>> _rtalloc(struct sockaddr *dst, uint32_t *src, int flags, unsigned int
>>> rtableid)
>>> {
>>> - struct rtentry *rt, *nhrt;
>>> + struct rtentry *rt;
>>>
>>> rt = rt_match(dst, src, flags, rtableid);
>>>
>>> @@ -381,6 +382,16 @@ _rtalloc(struct sockaddr *dst, uint32_t
>>> if (rtisvalid(rt->rt_gwroute))
>>> return (rt);
>>>
>>> + rt_setgwroute(rt, rtableid);
>>> +
>>> + return (rt);
>>> +}
>>> +
>>> +void
>>> +rt_setgwroute(struct rtentry *rt, u_int rtableid)
>>> +{
>>> + struct rtentry *nhrt;
>>> +
>>> rtfree(rt->rt_gwroute);
>>> rt->rt_gwroute = NULL;
>>>
>>> @@ -392,10 +403,9 @@ _rtalloc(struct sockaddr *dst, uint32_t
>>> * this behavior. But it is safe since rt_checkgate() wont
>>> * allow us to us this route later on.
>>> */
>>> - nhrt = rt_match(rt->rt_gateway, NULL, flags | RT_RESOLVE,
>>> - rtable_l2(rtableid));
>>> + nhrt = rt_match(rt->rt_gateway, NULL, RT_RESOLVE, rtable_l2(rtableid));
>>> if (nhrt == NULL)
>>> - return (rt);
>>> + return;
>>>
>>> /*
>>> * Next hop must be reachable, this also prevents rtentry
>>> @@ -403,13 +413,13 @@ _rtalloc(struct sockaddr *dst, uint32_t
>>> */
>>> if (ISSET(nhrt->rt_flags, RTF_CLONING|RTF_GATEWAY)) {
>>> rtfree(nhrt);
>>> - return (rt);
>>> + return;
>>> }
>>>
>>> /* Next hop entry must be UP and on the same interface. */
>>> if (!ISSET(nhrt->rt_flags, RTF_UP) || nhrt->rt_ifidx != rt->rt_ifidx) {
>>> rtfree(nhrt);
>>> - return (rt);
>>> + return;
>>> }
>>>
>>> /*
>>> @@ -424,8 +434,6 @@ _rtalloc(struct sockaddr *dst, uint32_t
>>> * do the magic for us.
>>> */
>>> rt->rt_gwroute = nhrt;
>>> -
>>> - return (rt);
>>> }
>>>
>>> void
>>> @@ -595,7 +603,7 @@ create:
>>> flags |= RTF_MODIFIED;
>>> prio = rt->rt_priority;
>>> stat = &rtstat.rts_newgateway;
>>> - rt_setgate(rt, gateway);
>>> + rt_setgate(rt, gateway, rdomain);
>>> }
>>> } else
>>> error = EHOSTUNREACH;
>>> @@ -1089,7 +1097,8 @@ rtrequest(int req, struct rt_addrinfo *i
>>> * it to (re)order routes.
>>> */
>>> if ((error = rt_setaddr(rt, ifa->ifa_addr)) ||
>>> - (error = rt_setgate(rt, info->rti_info[RTAX_GATEWAY]))) {
>>> + (error = rt_setgate(rt, info->rti_info[RTAX_GATEWAY],
>>> + tableid))) {
>>> ifafree(ifa);
>>> rtfree(rt->rt_parent);
>>> rtfree(rt->rt_gwroute);
>>> @@ -1169,7 +1178,7 @@ rt_setaddr(struct rtentry *rt, struct so
>>> }
>>>
>>> int
>>> -rt_setgate(struct rtentry *rt, struct sockaddr *gate)
>>> +rt_setgate(struct rtentry *rt, struct sockaddr *gate, u_int rtableid)
>>> {
>>> int glen = ROUNDUP(gate->sa_len);
>>> struct sockaddr *sa;
>>> @@ -1183,8 +1192,8 @@ rt_setgate(struct rtentry *rt, struct so
>>> }
>>> memmove(rt->rt_gateway, gate, glen);
>>>
>>> - rtfree(rt->rt_gwroute);
>>> - rt->rt_gwroute = NULL;
>>> + if (ISSET(rt->rt_flags, RTF_GATEWAY))
>>> + rt_setgwroute(rt, rtableid);
>>>
>>> return (0);
>>> }
>>> Index: net/route.h
>>> ===================================================================
>>> RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.h,v
>>> retrieving revision 1.138
>>> diff -u -p -r1.138 route.h
>>> --- net/route.h 14 Jun 2016 09:48:52 -0000 1.138
>>> +++ net/route.h 24 Jun 2016 22:29:45 -0000
>>> @@ -363,7 +363,7 @@ struct sockaddr *rt_plen2mask(struct rte
>>> void rt_sendmsg(struct rtentry *, int, u_int);
>>> void rt_sendaddrmsg(struct rtentry *, int, struct ifaddr *);
>>> void rt_missmsg(int, struct rt_addrinfo *, int, uint8_t, u_int,
>>> int, u_int);
>>> -int rt_setgate(struct rtentry *, struct sockaddr *);
>>> +int rt_setgate(struct rtentry *, struct sockaddr *, u_int);
>>> int rt_checkgate(struct rtentry *, struct rtentry **);
>>> void rt_setmetrics(u_long, const struct rt_metrics *, struct
>>> rt_kmetrics *);
>>> void rt_getmetrics(const struct rt_kmetrics *, struct rt_metrics *);
>>> Index: net/rtsock.c
>>> ===================================================================
>>> RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtsock.c,v
>>> retrieving revision 1.192
>>> diff -u -p -r1.192 rtsock.c
>>> --- net/rtsock.c 14 Jun 2016 09:48:52 -0000 1.192
>>> +++ net/rtsock.c 24 Jun 2016 22:33:04 -0000
>>> @@ -748,7 +748,8 @@ report:
>>> ifa = info.rti_ifa;
>>> }
>>> if (info.rti_info[RTAX_GATEWAY] != NULL && (error =
>>> - rt_setgate(rt, info.rti_info[RTAX_GATEWAY])))
>>> + rt_setgate(rt, info.rti_info[RTAX_GATEWAY],
>>> + tableid)))
>>> goto flush;
>>> if (ifa) {
>>> if (rt->rt_ifa != ifa) {
>>>
>
