Message-ID: <20200717142743.6d05d3ae@elisabeth>
Date:   Fri, 17 Jul 2020 14:27:43 +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 16:33:56 +0200
Florian Westphal <fw@...len.de> wrote:

> Stefano Brivio <sbrivio@...hat.com> wrote:
> > On Wed, 15 Jul 2020 14:42:58 +0200
> > Florian Westphal <fw@...len.de> wrote:  
> > > 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().  
> 
> Yes, but the caveat here is that a bridge might be transporting
> non-IP protocol too.
> 
> So, MTU-reduction+ICMP won't help for them.

Well, it doesn't need to, PMTU discovery is only implemented for IP,
so, to handle this, we can just check if the frame contains an IP
packet. The kind of check (skb->protocol == htons(ETH_P_...)) is
already there on all sorts of paths in the bridge.

However,

> I would try to avoid mixing IP functionality into the bridge,

if we stick to the fact the bridge is a L2 device, sure, we should drop
packets silently. The problem is that bridging an UDP tunnel forces the
combination to become a router.

So, either we forbid that, or I guess it's acceptable to have (even
further) L3 functionality implemented in the bridge.

> its a slippery slope (look at bridge netfilter for an example).

Oops, I was taking it as a positive example :)

> I agree that for a 'ip only' bridge that might work indeed.
> 
> > 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.  
> 
> Yes, it also requires that the bridge has IP connectivity
> to reach the inner ip, which might not be the case.

If the VXLAN endpoint is a port of the bridge, that needs to be the
case, right? Otherwise the VXLAN endpoint can't be reached.

> > 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.  
> 
> Every type of bridge port that needs to add additional header on egress
> has this problem in the bridge scenario once the peer of the IP tunnel
> signals a PMTU event.

Yes :(

> I agree that excess copy&paste should be avoided, but at this point
> I don't see an easy solution.

I think what you mention below is way more acceptable.

> > 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?  
> 
> Yes, if you don't do that then we have devices with MTU X hooked to
> a bridge with MTU Y, where X > Y.  I don't see how this could work.

Yes, I see, and my point is: they would get ICMP errors from the
bridge, and that would create route exceptions.

> > 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.  
> 
> Yes, it might be possible to move the proposed icmp inject into
> skb_tunnel_check_pmtu() -- it gets the needed headroom passed as arg,
> it could detect when device driver is in a bridge and it already knows
> when skb has no dst entry that it a pmtu change could be propagated to.

I didn't mean to move the ICMP injection there, I meant we could
override the MTU there.

Moving the ICMP injection there: I think that would be totally
reasonable, it comes with none of the issues of the solution I proposed
and with almost none of the issues I raised about your idea.

> Given the affected setups all use ovs I think it makes sense to make
> sure the intended solution would work for ovs too, bridge seems more
> like a nice-to-have thing at the moment.

Yes, agreed.

As I see it, we don't know what the problem is there, we might even
have to do absolutely nothing in kernel. I guess we should know before
trying to hack up something.

-- 
Stefano

Powered by blists - more mailing lists