[PATCH 3/3] Drop packets that are too large without dropping connection

[Daniel Lenski]

On Thu, May 11, 2017 at 9:09 AM, Nikolay Martynov <mar.kolya at gmail.com> wrote:
> 2017-05-11 12:00 GMT-04:00 Daniel Lenski <dlenski at gmail.com>:
>> On Wed, May 10, 2017 at 8:03 PM, Nikolay Martynov <mar.kolya at gmail.com> wrote:
>>> This improves connection stability.
>>
>> How so? What is the downside to accepting an unexpectedly large packet
>> which nevertheless managed to make it across the VPN tunnel?
>
>   This patch goes from current state which is 'drop connection if
> there is packet larger than MTU in the tunnel' to the state 'drop
> packet if it is larger than MTU, but keep connection' - this improves
> connection stability.
>
> Somehow with tunnels that I've gotten with my
> employer I often get packets in that are larger than MTU (and it looks
> like they have DF flag set). I do not know exact cause of it - and I
> do not have access to VPN equipment, unfortunately. But this patch
> improves connection stability with this setup. Higher stacks figure
> out MTU value and continue normally after first dropped packet.
> 'Official' client (pulse) seems to set up MTU to 1400 on interfaces it
> creates - same as does openconnect.
>
>   Obviously we can just inject large packets higher up the stack, but
> I'm not exactly sure what sort of side effects would happen if we do
> that. So I sort of tried to play safe here.

Interesting, thanks for explaining. I have access to a couple Juniper
VPNs as well (older version, not Pulse), but I haven't observed this
behavior from them.

>   I guess this code can be improved for cases when MTU is not
> known/not known exactly - but unfortunately I do not have equipment to
> test such changes. Change I've sent improves connection stability with
> pulse vpn server.
>
>   Please let me know if you would like me to improve that patch.

Yeah, that's really my only concern here. It might be safer simply to
pass through packets which are larger than the negotiated or estimated
MTU, as long as they're otherwise well-formed. I am not sure, however,
if this could impede MTU detection by higher levels of the protocol
stack.

What happens if you keep only this part of your patch but don't drop
the packets?

diff --git a/oncp.c b/oncp.c
index 3c7cfa1..40ac1de 100644
--- a/oncp.c
+++ b/oncp.c
@@ -1011,7 +1011,7 @@ int oncp_mainloop(struct openconnect_info
*vpninfo, int *timeout)
                                goto unknown_pkt;
                        }

-                       if (!iplen || iplen > vpninfo->ip_info.mtu ||
iplen > kmplen)
+                       if (!iplen || iplen > kmplen)
                                goto badiplen;

                        if (iplen > vpninfo->cstp_pkt->len - 20)


Thanks,
Dan