Opened 4 years ago

Closed 4 years ago

Last modified 2 years ago

#716 closed Bug / Defect (fixed)

ncp patch set breaks --mssfix (and possibly --fragment)

Reported by: Gert Döring Owned by:
Priority: blocker Milestone: alpha 2.4
Component: Generic / unclassified Version: OpenVPN git master branch (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc: Steffan Karger, Samuli Seppänen, David Sommerseth

Description

just so it's not lost: ncp patch set breaks --fragment, because a new frame is built and frame_finalize() sets

frame->link_mtu_dynamic = frame->link_mtu

so the --mssfix and most likely --fragment init needs to be re-done in tls_session_update_crypto_params()

Attachments (2)

0001-Fix-mssfix-when-using-NCP.patch (3.3 KB) - added by Steffan Karger 4 years ago.
0002-XXX-fix-fragment-seems-not-needed.patch (3.9 KB) - added by Steffan Karger 4 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by Gert Döring

Cc: Steffan Karger Samuli Seppänen added
Priority: majorblocker

comment:2 Changed 4 years ago by Gert Döring

Cc: David Sommerseth added

adding a bit more context

init.c has

  /* initialize MTU variables */
  do_init_frame (c);  /* calling frame_finalize_options() -> frame_finalize() internally */

  /* initialize TLS MTU variables */
  do_init_frame_tls (c);
...   
  do_init_fragment (c);
...
  /* initialize dynamic MTU variable */
  do_init_mssfix (c);

and these do_init_... functions modify variables in "struct frame" concerning maximum packet size (= to what value should mssfix reduce the MSS in SYN packets).

With the new code, we have ssl.c tls_session_update_crypto_params() which also calls frame_finalize() , overwriting the MTU values in there with the defaults. So we need to reinitialize do_init_mssfix()'ish after the frame_finalize() call, preferably without calling *back* into init.c, or without code duplication.

With just copying these two init functions, this is trivially solved (and I tested that it fixes mssfix, did not yet build a --fragment test suite) - but not pretty, and not good style...

Changed 4 years ago by Steffan Karger

Changed 4 years ago by Steffan Karger

comment:3 Changed 4 years ago by Steffan Karger

For --mssfix the proposed fix works, see 0001-Fix-mssfix-when-using-NCP.patch​ for a proposed patch.

For --fragment however, I think things are more complicated. do_init_fragment() does not touch c->c2.frame at all, but operates on a pre-frame-finalize copy of c->c2.frame: c->c2.frame_fragment. I didn't investigate yet, but get the feeling that if I change that more stuff will break... For completeness I did attach a patch that does what you proposed, since I wrote it anyway ;)

comment:4 Changed 4 years ago by Gert Döring

So, I finally came around to setting up a reference test server with "--fragment 500" (this cannot be pushed nor enabled dynamically from a ccd/ file, unfortunately). Here's the stanza for t_client.rc:

RUN_TITLE_6="udp / p2pm / top subnet / --fragment 500"
OPENVPN_CONF_6="$OPENVPN_BASE_P2MP --dev tun --proto udp --remote $REMOTE --port 51198 --fragment 500"
EXPECT_IFCONFIG4_6="10.194.6.2"
EXPECT_IFCONFIG6_6="fd00:abcd:194:6::1000"
PING4_HOSTS_6="10.194.6.1 10.194.0.1"
PING6_HOSTS_6="fd00:abcd:194:6::1 fd00:abcd:194:0::1"

(adapt EXPECT_IFCONFIG* addresses for what gets assigned to your client).

Good news - --fragment stuff works just fine, and it turns out I have misread the code. While do_init_fragment() does have a call to frame_set_mtu_dynamic() (which I assumed would interfere with the new frame), but this isn't operating on c2.fragment but on c2.frame_fragment - which is not affected by the new code.

So the "not-needed" patch is, indeed, not needed :-)

As far as --mssfix goes, the other patch is needed - and ACK from me. Tested, works, looks nice, and respects all the source hierarchy stuff plus "no code duplication". Thanks.

Can you please send to the list so I can formally ACK&merge?

comment:5 Changed 4 years ago by Gert Döring

Resolution: fixed
Status: newclosed

commit cbc3c5a9831b44ec7f59e8cb21e19ea364e6c0ee (master)
Author: Steffan Karger
Date: Sat Sep 10 08:11:12 2016 +0200

Fix --mssfix when using NCP

thanks :)

comment:6 Changed 2 years ago by stipa

What is the reason we re-initialize frame on NCP negotiation? Isn't it enough just to update frame_extra ?

Note: See TracTickets for help on using tickets.