netdev - Re: [PATCH net-next 1/3] udp_tunnel: allow to turn off path mtu discovery on encap sockets

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: <20200715153547.77dbaf82@elisabeth>
Date:   Wed, 15 Jul 2020 15:35:47 +0200
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Florian Westphal <fw@...len.de>
Cc:     David Ahern <dsahern@...il.com>, netdev@...r.kernel.org,
        aconole@...hat.com
Subject: Re: [PATCH net-next 1/3] udp_tunnel: allow to turn off path mtu
 discovery on encap sockets

On Wed, 15 Jul 2020 14:42:58 +0200
Florian Westphal <fw@...len.de> wrote:

> Stefano Brivio <sbrivio@...hat.com> wrote:
> > I would still like the idea I proposed better (updating MTUs down the
> > chain), it's simpler and we don't have to duplicate existing
> > functionality (generating additional ICMP messages).  
> 
> It doesn't make this work though.

Yeah, not knowing exactly what needs to work, that just fixes the two
cases you describe.

I thought that would be enough for Open vSwitch, but apparently it's
not (you mentioned the problem appeared with MTUs already set to
correct values). And also your (bulletproof, I thought) ICMP errors
don't work with it. :/

Anyway, about the Linux bridge:

> With your skeleton patch, br0 updates MTU, but the sender still
> won't know that unless input traffic to br0 is routed (or locally
> generated).

To let the sender know, I still think it's a bit simpler with this
approach, we don't have to do all the peeling. In br_handle_frame(), we
would need to add *something like*:

	if (skb->len > p->br->dev->mtu) {
		memset(IPCB(skb), 0, sizeof(*IPCB(skb)));
		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
			  htonl(p->br->dev->mtu));
		goto drop;
	}

just like IP tunnels do, see tnl_update_pmtu().

Note that this doesn't work as it is because of a number of reasons
(skb doesn't have a dst, pkt_type is not PACKET_HOST), and perhaps we
shouldn't be using icmp_send(), but at a glance that looks simpler.

Another slight preference I have towards this idea is that the only
known way we can break PMTU discovery right now is by using a bridge,
so fixing the problem there looks more future-proof than addressing any
kind of tunnel with this problem. I think FoU and GUE would hit the
same problem, I don't know about IP tunnels, sticking that selftest
snippet to whatever other test in pmtu.sh should tell.

I might be wrong of course as I haven't tried to implement this bit,
and if this turns out to be just moving the problem without making it
simpler, then sure, I'd rather stick to your approach.

> Furthermore, such MTU reduction would require a mechanism to
> auto-reconfig every device in the same linklevel broadcast domain,
> and I am not aware of any such mechanism.

You mean for other ports connected to the same bridge? They would then
get ICMP errors as well, no?

If you refer to other drivers that need to adjust the MTU, instead,
that's why I would use skb_tunnel_check_pmtu() for that, to avoid
implementing the same logic in every driver.

-- 
Stefano