Message ID | 1456946361-9889-1-git-send-email-tracywwnj@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
From: Wei Wang <weiwan@google.com> Date: Wed, 2 Mar 2016 11:19:21 -0800 > @@ -566,7 +567,16 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt, > if (type == ICMPV6_PKT_TOOBIG) { > if (!ip6_sk_accept_pmtu(sk)) > goto out; > - ip6_sk_update_pmtu(skb, sk, info); > + bh_lock_sock(sk); > + if (sk->sk_state == TCP_ESTABLISHED && > + !sock_owned_by_user(sk) && > + ipv6_addr_equal(saddr, &sk->sk_v6_rcv_saddr) && > + ipv6_addr_equal(daddr, &sk->sk_v6_daddr) && > + uh->dest == sk->sk_dport) > + inet6_csk_update_pmtu(sk, ntohl(info)); If I apply this patch it will hide a bug. Why isn't ip6_sk_update_pmtu() matching the same route as the one attached to the socket? I'd prefer you figure out what part of the lookup key used is wrong, and fix that instead. Thanks.
On Sat, Mar 5, 2016 at 9:55 PM, David Miller <davem@davemloft.net> wrote: > From: Wei Wang <weiwan@google.com> > Date: Wed, 2 Mar 2016 11:19:21 -0800 > >> @@ -566,7 +567,16 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt, >> if (type == ICMPV6_PKT_TOOBIG) { >> if (!ip6_sk_accept_pmtu(sk)) >> goto out; >> - ip6_sk_update_pmtu(skb, sk, info); >> + bh_lock_sock(sk); >> + if (sk->sk_state == TCP_ESTABLISHED && >> + !sock_owned_by_user(sk) && >> + ipv6_addr_equal(saddr, &sk->sk_v6_rcv_saddr) && >> + ipv6_addr_equal(daddr, &sk->sk_v6_daddr) && >> + uh->dest == sk->sk_dport) >> + inet6_csk_update_pmtu(sk, ntohl(info)); > > If I apply this patch it will hide a bug. > > Why isn't ip6_sk_update_pmtu() matching the same route as the > one attached to the socket? > > I'd prefer you figure out what part of the lookup key used is > wrong, and fix that instead. > The dst itself is the same than the socket sk_dst_cache, but __ip6_rt_update_pmtu() sees rt6_cache_allowed_for_pmtu() We endup doing : nrt6 = ip6_rt_cache_alloc(rt6, daddr, saddr); if (nrt6) { rt6_do_update_pmtu(nrt6, mtu); /* ip6_ins_rt(nrt6) will bump the * rt6->rt6i_node->fn_sernum * which will fail the next rt6_check() and * invalidate the sk->sk_dst_cache. */ ip6_ins_rt(nrt6); } But apparently the sk->sk_dst_cache is _not_ invalidated, even if the comment loudly claims so.
On Wed, Mar 02, 2016 at 11:19:21AM -0800, Wei Wang wrote: > From: Wei Wang <weiwan@google.com> > > When ICMPV6_PKT_TOOBIG message is received by a connected UDP socket, > the new mtu value is not properly updated in the dst_entry associated > with the socket. > This leads to the issue that the mtu value returned by getsockopt(sockfd, > IPPROTO_IPV6, IPV6_MTU, ...) is wrong. > The fix is to call the corresponding pmtu related function for connected > socket so that the dst_entry associated with the socket will get updated > with the new mtu value. Would it be a better fix if ip6_sk_update_pmtu() does a dst_check() and updates sk->sk_dst_cache (if needed) before it returns? It seems ipv4_sk_update_pmtu() is also doing it. or sk->sk_dst_cache is still valid after ip6_sk_update_pmtu() returns?
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index 0711f8f..5e6ba54 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -49,6 +49,7 @@ #include <net/inet6_hashtables.h> #include <net/busy_poll.h> #include <net/sock_reuseport.h> +#include <net/inet6_connection_sock.h> #include <linux/proc_fs.h> #include <linux/seq_file.h> @@ -566,7 +567,16 @@ void __udp6_lib_err(struct sk_buff *skb, struct inet6_skb_parm *opt, if (type == ICMPV6_PKT_TOOBIG) { if (!ip6_sk_accept_pmtu(sk)) goto out; - ip6_sk_update_pmtu(skb, sk, info); + bh_lock_sock(sk); + if (sk->sk_state == TCP_ESTABLISHED && + !sock_owned_by_user(sk) && + ipv6_addr_equal(saddr, &sk->sk_v6_rcv_saddr) && + ipv6_addr_equal(daddr, &sk->sk_v6_daddr) && + uh->dest == sk->sk_dport) + inet6_csk_update_pmtu(sk, ntohl(info)); + else + ip6_sk_update_pmtu(skb, sk, info); + bh_unlock_sock(sk); if (np->pmtudisc != IPV6_PMTUDISC_DONT) harderr = 1; }