All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hangbin Liu <liuhangbin@gmail.com>
To: David Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org, ja@ssi.bg, marcelo.leitner@gmail.com,
	dsahern@gmail.com, edumazet@google.com
Subject: Re: [PATCH net] ipv6/route: only update neigh confirm time if pmtu changed
Date: Tue, 26 Nov 2019 17:17:27 +0800	[thread overview]
Message-ID: <20191126091726.GH18865@dhcp-12-139.nay.redhat.com> (raw)
In-Reply-To: <20191122.100438.416583996458136747.davem@davemloft.net>

Hi David,

Sorry for the late reply. I'm not sure why your reply went to spam list and
I didn't receive it timely.
On Fri, Nov 22, 2019 at 10:04:38AM -0800, David Miller wrote:
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Fri, 22 Nov 2019 14:19:19 +0800
> 
> > The reason is when we ping6 remote via gretap, we will call like
> > 
> > gre_tap_xmit()
> >  - ip_tunnel_xmit()
> >    - tnl_update_pmtu()
> >      - skb_dst_update_pmtu()
> >        - ip6_rt_update_pmtu()
> >          - __ip6_rt_update_pmtu()
> >            - dst_confirm_neigh()
> >              - ip6_confirm_neigh()
> >                - __ipv6_confirm_neigh()
> >                  - n->confirmed = now
> 
> This whole callchain violates the assumptions of the MTU update
> machinery.
> 
> In this case it's just the tunneling code accounting for the
> encapsulation it is creating, and checking the MTU just in case.
> 
> But the MTU update code is supposed to be invoked in response to real
> networking events that update the PMTU.
> 
> So for this ip_tunnel_xmit() case, _EVEN_ if the MTU is changed, we
> should not be invoking dst_confirm_neigh() as we have no evidence
> of successful two-way communication at this point.

Thanks for the explanation. When I fixed the code, I was also wondering
if we need this neighbor confirmation. So I just moved the dst_confirm_neigh()
a little down to make sure pmtu changed. Your explanation make me clear that
we should not have this neighbor confirmation as PMTU is not a two-way
communication.
> 
> We have to stop papering over the tunneling code's abuse of the PMTU
> update framework and do this properly.

Should I do other works than just remove dst_confirm_neigh() in
__ip6_rt_update_pmtu().

Thanks
Hangbin
> 
> Sorry, I'm not applying this.

  reply	other threads:[~2019-11-26  9:17 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-22  6:19 [PATCH net] ipv6/route: only update neigh confirm time if pmtu changed Hangbin Liu
2019-11-22 18:04 ` David Miller
2019-11-26  9:17   ` Hangbin Liu [this message]
2019-12-03  2:11 ` [PATCHv2 net] ipv6/route: should not update neigh confirm time during PMTU update Hangbin Liu
2019-12-03  2:47   ` David Miller
2019-12-03 10:15     ` Hangbin Liu
2019-12-03 10:25       ` Hangbin Liu
2019-12-03 19:58         ` David Miller
2019-12-10  3:36           ` Hangbin Liu
2019-12-10 17:00             ` Guillaume Nault
2019-12-18 11:53   ` [PATCH net-next 0/8] disable neigh update for tunnels during pmtu update Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 1/8] net: add bool confirm_neigh parameter for dst_ops.update_pmtu Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 2/8] ip6_gre: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 3/8] gtp: " Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 4/8] net/dst: add new function skb_dst_update_pmtu_no_confirm Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 5/8] tunnel: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-19 17:47       ` Guillaume Nault
2019-12-20  2:36         ` Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 6/8] vti: " Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 7/8] sit: " Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 8/8] net/dst: do not confirm neighbor for vxlan and geneve " Hangbin Liu
2019-12-19 17:49       ` Guillaume Nault
2019-12-18 12:01     ` [PATCH net-next 0/8] disable neigh update for tunnels during " Hangbin Liu
2019-12-19 17:57       ` Guillaume Nault
2019-12-20  2:48         ` Hangbin Liu
2019-12-19 17:53     ` Guillaume Nault
2019-12-20  3:25     ` [PATCHv4 net " Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 1/8] net: add bool confirm_neigh parameter for dst_ops.update_pmtu Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 2/8] ip6_gre: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 3/8] gtp: " Hangbin Liu
2019-12-21 18:35         ` Guillaume Nault
2019-12-20  3:25       ` [PATCHv4 net 4/8] net/dst: add new function skb_dst_update_pmtu_no_confirm Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 5/8] tunnel: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-21 18:43         ` Guillaume Nault
2019-12-20  3:25       ` [PATCHv4 net 6/8] vti: " Hangbin Liu
2019-12-21 18:30         ` Guillaume Nault
2019-12-20  3:25       ` [PATCHv4 net 7/8] sit: " Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 8/8] net/dst: do not confirm neighbor for vxlan and geneve " Hangbin Liu
2019-12-21 18:38         ` Guillaume Nault
2019-12-20 16:14       ` [PATCHv4 net 0/8] disable neigh update for tunnels during " David Ahern
2019-12-22  2:51       ` [PATCHv5 " Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 1/8] net: add bool confirm_neigh parameter for dst_ops.update_pmtu Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 2/8] ip6_gre: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 3/8] gtp: " Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 4/8] net/dst: add new function skb_dst_update_pmtu_no_confirm Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 5/8] tunnel: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 6/8] vti: " Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 7/8] sit: " Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 8/8] net/dst: do not confirm neighbor for vxlan and geneve " Hangbin Liu
2019-12-22 22:10         ` [PATCHv5 net 0/8] disable neigh update for tunnels during " Guillaume Nault
2019-12-25  6:30         ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191126091726.GH18865@dhcp-12-139.nay.redhat.com \
    --to=liuhangbin@gmail.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@gmail.com \
    --cc=edumazet@google.com \
    --cc=ja@ssi.bg \
    --cc=marcelo.leitner@gmail.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.