netdev - Re: [PATCHv4 net 6/8] vti: do not confirm neighbor when do pmtu update

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]

Date:   Sat, 21 Dec 2019 19:30:24 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, Julian Anastasov <ja@....bg>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        David Ahern <dsahern@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        Pablo Neira <pablo@...filter.org>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Alexey Kodanev <alexey.kodanev@...cle.com>
Subject: Re: [PATCHv4 net 6/8] vti: do not confirm neighbor when do pmtu
 update

On Fri, Dec 20, 2019 at 11:25:23AM +0800, Hangbin Liu wrote:
> Although ip vti is not affected as __ip_rt_update_pmtu() does not call
> dst_confirm_neigh(), we still not do neigh confirm to keep consistency with
> IPv6 code.
> 
As with the GRE case, the problem is not the IP version used on the
underlay. The problem happens when the tunnel transports IPv6 packets
(which vti can do).

However, it's true that vti is immune to this problem, but that's for
another reason: it's an IFF_NOARP interface (which means vti6 is immune
too).

Patch is good (we really have no reason to confirm neighbour here), but
let's not have a misleading commit message.

> v4: No change.
> v3: Do not remove dst_confirm_neigh, but add a new bool parameter in
>     dst_ops.update_pmtu to control whether we should do neighbor confirm.
>     Also split the big patch to small ones for each area.
> v2: Remove dst_confirm_neigh in __ip6_rt_update_pmtu.
> 
> Reviewed-by: Guillaume Nault <gnault@...hat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>  net/ipv4/ip_vti.c  | 2 +-
>  net/ipv6/ip6_vti.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
> index cfb025606793..fb9f6d60c27c 100644
> --- a/net/ipv4/ip_vti.c
> +++ b/net/ipv4/ip_vti.c
> @@ -214,7 +214,7 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev,
>  
>  	mtu = dst_mtu(dst);
>  	if (skb->len > mtu) {
> -		skb_dst_update_pmtu(skb, mtu);
> +		skb_dst_update_pmtu_no_confirm(skb, mtu);
>  		if (skb->protocol == htons(ETH_P_IP)) {
>  			icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
>  				  htonl(mtu));
> diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
> index 024db17386d2..6f08b760c2a7 100644
> --- a/net/ipv6/ip6_vti.c
> +++ b/net/ipv6/ip6_vti.c
> @@ -479,7 +479,7 @@ vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
>  
>  	mtu = dst_mtu(dst);
>  	if (skb->len > mtu) {
> -		skb_dst_update_pmtu(skb, mtu);
> +		skb_dst_update_pmtu_no_confirm(skb, mtu);
>  
>  		if (skb->protocol == htons(ETH_P_IPV6)) {
>  			if (mtu < IPV6_MIN_MTU)
> -- 
> 2.19.2
>