On 25/06/16(Sat) 01:03, Alexander Bluhm wrote:
> Hi,
>
> Path MTU discovery in IPv6 is slightly broken. I takes two
> ICMP6_PACKET_TOO_BIG packets to create and change the dynamic route.
>
> When the first ICMP6 packet arrives, a dynamic route gets created
> in icmp6_mtudisc_clone() and icmp6_mtudisc_update() sets its MTU
> value. During notify tcp_mtudisc() removes the old route from the
> inp and allocates the dynamic route. As the gateway route is not
> set, the MTU is reset to 0 in _rtalloc().
>
> The second packet will not allocate a dynamic route as it already
> exists. Just the MTU is updated. Then tcp_mtudisc() does not
> change the route at the inp as it already the correct one. So now
> PMTU discovery is successful.
>
> This behavior was introduced in net/route.c rev 1.269 when the
> gateway route allocation was moved from rt_setgateway() to _rtalloc().
> So now rtrequest() can return a route without a valid gateway route.
> To fix this, I call rt_setgwroute() from _rtalloc() and rt_setgateway().
> As a result icmp6_mtudisc_clone() will create a route with a gateway
> route and tcp_mtudisc() will not reset the MTU to 0 during rtalloc().
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?
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).
> 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) {
>
