netdev - Re: [PATCH net] ipv6/route: only update neigh confirm time if pmtu changed

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191126091726.GH18865@dhcp-12-139.nay.redhat.com>
Date:   Tue, 26 Nov 2019 17:17:27 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, ja@....bg, marcelo.leitner@...il.com,
        dsahern@...il.com, edumazet@...gle.com
Subject: Re: [PATCH net] ipv6/route: only update neigh confirm time if pmtu
 changed

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@...il.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.