lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <540DE380.1000302@fb.com> Date: Mon, 8 Sep 2014 10:12:32 -0700 From: Alex Gartrell <agartrell@...com> To: Eric Dumazet <eric.dumazet@...il.com> CC: <davem@...emloft.net>, <edumazet@...gle.com>, <netdev@...r.kernel.org>, <kernel-team@...com>, <ps@...com> Subject: Re: [RFC PATCH net-next] ip6: Do not expire uncached routes for mtu invalidation Thank you for taking a look, Eric. I'll admit that I have a distinct lack of confidence that I've got the right solution to the problem here, but I've made it about as far as I can without getting your collective comments, so it's much appreciated. On 9/8/14 3:30 AM, Eric Dumazet wrote: > On Mon, 2014-09-08 at 01:34 -0700, Alex Gartrell wrote: >> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h >> index 9bcb220..2f0d4d0 100644 >> --- a/include/net/ip6_fib.h >> +++ b/include/net/ip6_fib.h >> @@ -184,7 +184,8 @@ static inline void rt6_update_expires(struct rt6_info *rt0, int timeout) >> rt0->dst.expires = rt->dst.expires; >> >> dst_set_expires(&rt0->dst, timeout); >> - rt0->rt6i_flags |= RTF_EXPIRES; >> + if (rt0->rt6i_flags & (RTF_CACHE | RTF_EXPIRES)) >> + rt0->rt6i_flags |= RTF_EXPIRES; > > This looks wrong. What could be the point of settinf RTF_EXPIRES if its > already set ? > This is a good point. It was clearer to me at the time to include it (more similar to the old implementation which set the bit unconditionally), but I don't really care. >> } >> >> static inline void rt6_set_from(struct rt6_info *rt, struct rt6_info *from) >> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >> index f74b041..a509a06 100644 >> --- a/net/ipv6/route.c >> +++ b/net/ipv6/route.c >> @@ -947,8 +947,19 @@ restart: >> nrt = rt6_alloc_cow(rt, &fl6->daddr, &fl6->saddr); >> else if (!(rt->dst.flags & DST_HOST)) >> nrt = rt6_alloc_clone(rt, &fl6->daddr); >> - else >> + else { >> + if (!(rt->rt6i_flags & RTF_EXPIRES) && rt->dst.expires && >> + time_after(jiffies, rt->dst.expires)) { >> + /* Uncached routes may have expires set if we >> + * intend to expire the MTU but not the dest >> + * itself. In that case, we should reset the mtu >> + * before handing it back */ >> + dst_metric_set(&rt->dst, RTAX_MTU, 0); >> + rt6_clean_expires(rt); >> + rt->rt6i_flags &= ~RTF_MODIFIED; > > Many cpus can perform this at the same time on same route, this looks > racy. Initially I was just going to agree with you here, but taking another look at ip_vs_xmit at least, there doesn't appear to be any special locking before invoking ->update_pmtu, which is playing with rt6i_flags and dst.expires as well. Is that racy as well or is there something else I'm missing here? There are other ways to skin this particular cat though, and I've got no specific attachment to any of them. The most logical thing to do IMO is clone the route when it may be necessary to do so, but given the fact that that was very deliberately undone in 7343ff3 "ipv6: Don't create clones of host routes," I'm not sure that it's the right thing to do or that it won't require major surgery. Thanks again, -- Alex Gartrell <agartrell@...com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists