Date: Mon, 15 Mar 2021 09:46:52 -0600 From: Scott Long <scottl@samsco.org> To: Kyle Evans <kevans@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: 74ae3f3e33b8 - main - if_wg: import latest fixup work from the wireguard-freebsd project Message-ID: <1D0606F6-73C2-42B9-9CF5-E3EBE01CE4EE@samsco.org> In-Reply-To: <CACNAnaFi_wsZQzHz=wc5_s=8kAptpucyTP96gYYCVt_sxSj6hA@mail.gmail.com> References: <202103150452.12F4qxjV047368@gitrepo.freebsd.org> <13F91280-2246-4A7B-BAC2-B9ABA07B561F@samsco.org> <CACNAnaFi_wsZQzHz=wc5_s=8kAptpucyTP96gYYCVt_sxSj6hA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I=E2=80=99m sorry that you feel that I was =E2=80=9Cimmediately = aggressive=E2=80=9D. I have the IRC logs to back up my statement that I was supportive but concerned, and what I objected to. If you feel that I was immediately aggressive or = that I deserved the scorn you hurled, then I really have nothing more to add to this conversation, other than I=E2=80=99ll be looking for other ways for = Netgate to operate that don=E2=80=99t overlap with Wireguard. Scott > On Mar 15, 2021, at 9:27 AM, Kyle Evans <kevans@freebsd.org> wrote: >=20 > On Mon, Mar 15, 2021 at 9:57 AM Scott Long <scottl@samsco.org> wrote: >>=20 >> Here is the response I sent to you and Donenfeld in private. I = won=E2=80=99t include >> my direct conversation with you from Slack/IRC, but I made my = concerns >> and objections pretty clear. This commit is quite disappointing. = See below: >>=20 >=20 > I'm sorry that you feel that way, but I've been pretty vocal about my > intentions to merge this work as soon as we were done with internal > review due to the pre-existing state of the driver. >=20 >> The good: >> [... snip ...] >>=20 >> The bad: >> - I want to be pragmatic about code APIs. Maybe iflib isn=E2=80=99t = ready for >> pseudo interfaces yet, and fixing it is non-trivial and out of scope. >> Going back to ifnet puts it back in line with openbsd and likely does >> fix the vnet problems. However, it=E2=80=99s a radical change to the = driver, and >> not something that I can support as a last-minute action for FreeBSD >> 13.0 or for pfSense. Therefore, I=E2=80=99m advising against its = immediate >> inclusion. The final choice is not mine to make for FreeBSD, but >> that=E2=80=99s my recommendation. For pfSense, I=E2=80=99ll be = discussing this with >> the rest of the engineering staff on Monday. >>=20 >=20 > iflib is definitely not ready for pseudo interfaces, and I'd like to > see the technical justification for using iflib here in the first > place. if_wg used exactly 0 of its real features because the queueing > system was bypassed by setting if_transmit in its IFDI_ATTACH_POST > implementation. In other words, we gained all of its complexity and > pseudo interfaces immaturity without any benefit that we found while > exploring the possibilities. >=20 >> - The LKML wouldn=E2=80=99t accept this kind of submission, they=E2=80=99= d insist that >> it be broken down into consumable pieces, and that bug fixes be >> considered and provided that don=E2=80=99t rely on massive re-writes. = I=E2=80=99ve >> been dealing with linux for 20+ years and BSD for almost 30 years, >> and I=E2=80=99ve got to say that despite my distaste for how the LKML = is run, >> they get results. Does fixing a segfault or packet drop/reorder >> require the removal of iflib? >>=20 >=20 > The review process on FreeBSD breaks down, as you yourself have noted. > mmacy has not been involved in if_wg since dropping it in the tree > AFAICT, and grehan claimed to only be involved because it was passed > to him at Netgate and that he didn't mind where it goes. Thus, I used > developer discretion and sought review from the person that wrote the > OpenBSD implementation and the person that designed the protocol. We > iterated on this for days to fix the numerous issues we were presented > with. >=20 >> The Ugly: >> - An accusation was made, tonight, to me, that the code Netgate >> sponsored was not reviewed and was shoved into the tree at the last >> minute. This grossly ignores the actual history to the point of >> weakening my tolerance for this entire discussion. It shows a pretty >> irrational bias against mmacy and Netgate and a gross ignorance of >> the history and provenience of the work. >>=20 >=20 > I'm sorry that I got heated here, your tone was immediately aggressive > after I just spent five days cleaning up the mess that was left > behind. At least one of the security issues I found was a small > configuration tweak and near-immediate destruction of the system when > applying any real load to the driver. >=20 >> - The Netgate copyright was unilaterally removed. Yes, it was re- >> instated a few minutes ago, and with good reason. Please don=E2=80=99t= >> do that again. >>=20 >=20 > I won't touch on this, other than "ack". >=20 >> - The removal of the ASM crypto bits really confuses me. An >> accusation was made, again tonight, that Netgate merely paid to >> benchmark the code, that we contributed nothing to it, and that = it=E2=80=99s >> bad code that can=E2=80=99t be audited. What I see is that the meat = of the >> implementation is copyrighted by Jason and others. There=E2=80=99s a >> modest but non-trivial amount of glue code that has (or had) the >> Netgate copyright. I=E2=80=99m not going to touch the audit nonsense. >> I need data here, and I=E2=80=99m not seeing it. >>=20 >=20 > I would have appreciated a pointer to the copyrighted 63 lines in > include/, because this was obviously ignorance on my part. You > suggested functional testing and bug fixing, the former of which I > inherently include in 'benchmarking' since you can't benchmark > something that isn't functional, and received no pointer of the > latter. >=20 >> There seems to be a lot of bad blood, poor understanding, and >> misinformation going on. I need all of this bullshit to stop. This >> is 5000-ish lines of a nice way to get a point-to-point VPN going >> without the hassle of IPSec or the performance problems of >> OpenVPN. It=E2=80=99s consuming way more time and energy than it=E2=80= =99s >> worth to me, and I=E2=80=99d much rather spend time and money to >> implement OpenVPN DCO and work on profiling and optimizing >> IPSec. Wireguard is important to Netgate because we recognize >> that it=E2=80=99s a feature that customers want, we=E2=80=99re = committed to making >> security accessible, and we strongly believe in open source and >> FreeBSD. It=E2=80=99s not perfect code, not by a long shot, but I = expect >> better collaboration and communication than what I=E2=80=99m seeing = here. >>=20 >=20 > It's important to me that we do what's right for the community, and > the if_wg module that was in-tree was not right for the community. I > just want something secure and usable in the tree, the latter point > being the packet loss complaints from the Netgate support forum that I > pointed you at as well as the kernel not handling allowed-ips > conflicts that I had mentioned, among various protocol violations and > other things the module did not handle w.r.t. configuration. The > former point being at least the buffer overflow I mentioned, but there > are more. >=20 > Thanks, >=20 > Kyle Evans >=20 >> Scott >>=20 >>=20 >>> On Mar 14, 2021, at 10:52 PM, Kyle Evans <kevans@FreeBSD.org> wrote: >>>=20 >>> The branch main has been updated by kevans: >>>=20 >>> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D74ae3f3e33b810248da19004c58b3581= cd367843 >>>=20 >>> commit 74ae3f3e33b810248da19004c58b3581cd367843 >>> Author: Kyle Evans <kevans@FreeBSD.org> >>> AuthorDate: 2021-03-15 02:25:40 +0000 >>> Commit: Kyle Evans <kevans@FreeBSD.org> >>> CommitDate: 2021-03-15 04:52:04 +0000 >>>=20 >>> if_wg: import latest fixup work from the wireguard-freebsd project >>>=20 >>> This is the culmination of about a week of work from three = developers to >>> fix a number of functional and security issues. This patch = consists of >>> work done by the following folks: >>>=20 >>> - Jason A. Donenfeld <Jason@zx2c4.com> >>> - Matt Dunwoodie <ncon@noconroy.net> >>> - Kyle Evans <kevans@FreeBSD.org> >>>=20 >>> Notable changes include: >>> - Packets are now correctly staged for processing once the = handshake has >>> completed, resulting in less packet loss in the interim. >>> - Various race conditions have been resolved, particularly w.r.t. = socket >>> and packet lifetime (panics) >>> - Various tests have been added to assure correct functionality = and >>> tooling conformance >>> - Many security issues have been addressed >>> - if_wg now maintains jail-friendly semantics: sockets are created = in >>> the interface's home vnet so that it can act as the sole network >>> connection for a jail >>> - if_wg no longer fails to remove peer allowed-ips of 0.0.0.0/0 >>> - if_wg now exports via ioctl a format that is future proof and >>> complete. It is additionally supported by the upstream >>> wireguard-tools (which we plan to merge in to base soon) >>> - if_wg now conforms to the WireGuard protocol and is more closely >>> aligned with security auditing guidelines >>>=20 >>> Note that the driver has been rebased away from using iflib. = iflib >>> poses a number of challenges for a cloned device trying to operate = in a >>> vnet that are non-trivial to solve and adds complexity to the >>> implementation for little gain. >>>=20 >>> The crypto implementation that was previously added to the tree = was a >>> super complex integration of what previously appeared in an old = out of >>> tree Linux module, which has been reduced to crypto.c containing = simple >>> boring reference implementations. This is part of a near-to-mid = term >>> goal to work with FreeBSD kernel crypto folks and take advantage = of or >>> improve accelerated crypto already offered elsewhere. >>>=20 >>> There's additional test suite effort underway out-of-tree taking >>> advantage of the aforementioned jail-friendly semantics to test a = number >>> of real-world topologies, based on netns.sh. >>>=20 >>> Also note that this is still a work in progress; work going = further will >>> be much smaller in nature
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1D0606F6-73C2-42B9-9CF5-E3EBE01CE4EE>