Removing WireGuard Support From FreeBSD Base

[Kyle Evans]

Hi,

You may have recently noticed some chatter around the internet about
FreeBSD's in-kernel WireGuard implementation, and the work we've done
on it in the last week. You may have also noticed additional chatter
afterwards with regards to the original implementation. I'd like to give
some context and information with regards to the current situation, as
well as provide some insight into the future as one of the developers
involved.

With regard to the original implementation, this will be my only
commentary on the matter. I'm a developer, and I'm passionate
about the work that I do- often to a fault. I've said some things that
I regret; the accusations that Scott Long alluded to in an e-mail on FreeBSD
mailing lists were indeed made by me, and his phrasing of what I
said was much kinder than it could have been. These were mistakes,
and I'm going to own that. However, my personal belief is that neither
Netgate, pfSense, nor the original developer deserved the level of
scorn and criticism that they've received in the past days from both the
press and the community at large.

In the next day or so, I will be committing a removal of all WireGuard
related bits from our 'main' branch, including the work that I recently
committed. It will be followed up by a removal of the implementation
from stable/13, and we will seek appropriate approval to remove it
from releng/13.0 as well. Please, do not be concerned by any of this;
this is being done with mutual support from all parties.

Did the original implementation have issues? Yes, it did. Are we
certain that our new version -doesn't- have issues? I believe it
doesn't, but it hasn't been through thorough enough review. We hacked
on this for a week, and we all reviewed each others' work in the
process. The problem is that this work, in particular, is a driver with fairly
severe security implications. Review by "three developers working
and beating on it" is not the higher bar that we should be
holding this to. While I believed I was doing what's right for the
community, it's become clear that what's right for the community is
to take a step back and do this the right way.

Note that we're not dropping this effort. We will continue iterating
on this out-of-tree, and we will go through the proper review
channels. Folks will be unhappy in the interim because we're removing
it right now, but in the end we will have a better FreeBSD because of
it. There will be a kernel module available in ports at some point,
but not before it's ready.

Moving forward, myself, members of Netgate, and members of the larger
community *are* working together on strictly technical details. I urge
anyone with an interest in reviewing the driver to also get in touch with me.
Please, let's move forward as a community on this.

Thank you,

Kyle Evans

[Jason A. Donenfeld]

Hi Gordon,

I am not sure, if the removal is a great idea, a removal from
releng/13 and stable/13 - possibly yes, but from main?
This is still -CURRENT and -CURRENT should be central place for development,
even if we have phabricator for review.

It looks like Kyle has gone ahead with the revert anyway, so
development is now happening at:

https://git.zx2c4.com/wireguard-freebsd/

And there are now regular snapshot releases:

https://lists.zx2c4.com/pipermail/wireguard/2021-March/006518.html

As for your objections, and the question of what -CURRENT should or
shouldn't be used for, I really have no idea as a community outsider.
But I do look forward to submitting it for proper inclusion in
-CURRENT after a few more cycles of development and refinement.
There's also the crypto question that I'd welcome some feedback on:

https://lists.freebsd.org/pipermail/freebsd-hackers/2021-March/057076.html

If the complete backout is happening, please don't forget the manual
page. I have spend a lot of time on it, while OpenBSD made a good
template.

Thanks for bringing this up; I had actually forgotten about that. Do
you want to re-add it and keep that current as we develop? If you
email me your SSH key, you can just commit it directly.

Jason

[Evilham]

On Wed, Mar 17, 2021 at 12:34:02PM -0600, Jason A. Donenfeld

Post by Jason A. Donenfeld
Hi Gordon,
On Wed, Mar 17, 2021 at 6:53 AM Gordon Bergling

I am not sure, if the removal is a great idea, a removal from
releng/13 and stable/13 - possibly yes, but from main?
This is still -CURRENT and -CURRENT should be central place
for development,
even if we have phabricator for review.

It looks like Kyle has gone ahead with the revert anyway, so
https://git.zx2c4.com/wireguard-freebsd/
https://lists.zx2c4.com/pipermail/wireguard/2021-March/006518.html
As for your objections, and the question of what -CURRENT
should or
shouldn't be used for, I really have no idea as a community
outsider.
But I do look forward to submitting it for proper inclusion in
-CURRENT after a few more cycles of development and refinement.
https://lists.freebsd.org/pipermail/freebsd-hackers/2021-March/057076.html

If the complete backout is happening, please don't forget the manual
page. I have spend a lot of time on it, while OpenBSD made a good
template.

Thanks for bringing this up; I had actually forgotten about
that. Do
you want to re-add it and keep that current as we develop? If
you
email me your SSH key, you can just commit it directly.
Jason

Thanks for the reply. I still think that the removal from main
was a mistake,
but it has happened.
I'll create a port for WireGuard tomorrow so that FreeBSD isn't
losing WireGuard
support at all, for whatever reason.
--Gordon

If you do that, please take following tiny patch into account
(missing from the git repo @zx2c4, posted to the WG ML awaiting
moderation):

This is due to the removal commit form stable/13 and, from what I
saw, didn't affect CURRENT or 12.

---
src/compat.h | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/compat.h b/src/compat.h
index 6126e26..bc29c01 100644
--- a/src/compat.h
+++ b/src/compat.h
@@ -7,6 +7,9 @@
*/

#include <sys/param.h>
+#if __FreeBSD_version < 1400000
+#include <sys/smp.h>
+#include <sys/gtaskqueue.h>
#if __FreeBSD_version < 1300000
#define VIMAGE

@@ -18,8 +21,6 @@
#include <sys/malloc.h>
#include <sys/proc.h>
#include <sys/lock.h>
-#include <sys/smp.h>
-#include <sys/gtaskqueue.h>
#include <sys/socketvar.h>
#include <sys/protosw.h>
#include <net/vnet.h>
@@ -39,6 +40,7 @@

#undef atomic_load_ptr
#define atomic_load_ptr(p) (*(volatile __typeof(*p) *)(p))
+#endif /* __FreeBSD_version < 1300000 */

struct taskqgroup_cpu {
LIST_HEAD(, grouptask) tgc_tasks;
@@ -67,7 +69,7 @@ static inline void taskqgroup_drain_all(struct
taskqgroup *tqg)
gtaskqueue_drain_all(q);
}
}
-#endif
+#endif /* __FreeBSD_version < 1400000 */

#if __FreeBSD_version < 1202000
static inline uint32_t arc4random_uniform(uint32_t bound)
--
2.30.1

[Jason A. Donenfeld]

Hi Jeffrey,

Post by Kyle Evans
In the next day or so, I will be committing a removal of all WireGuard
related bits from our 'main' branch, including the work that I recently
committed. It will be followed up by a removal of the implementation
from stable/13, and we will seek appropriate approval to remove it
from releng/13.0 as well. Please, do not be concerned by any of this;
this is being done with mutual support from all parties.

The thing I find unusual is, the move appears to lack technical
justification. The best I can tell, the reasons seem to be political.
But like I said, maybe my feeds are missing something...
As a naive outsider, if you are going to yank it, then the technical
reasons for the action should be clearly enumerated. Everything else
is just chatter or noise. The move just looks like a bunch of bruised
egos and sour grapes.

I'd just like to chime in and point out that although this is
happening in a political context as you've pointed out, this is in my
opinion the *best possible technical situation*, and the one I would
have preferred in the beginning anyway if it were presented as a
choice.

Here's the technical background you asked for:
- We found tons of issues with the original code base.
- We spent a week rewriting that codebase.
So here's the rationale:
- Merging a week-old codebase into an operating system kernel is a bad idea.

It's really not more complicated than that. I'm *sure* we'll find more
things to fix. That's just the nature of it.

And from a practical perspective, it's a lot easier for me, anyway, to
casually push fixes as I code to a normal repo on git.zx2c4.com. When
there's a lot of potential code churn, sometimes it's easiest to be
able to move fast at first. When we get it to a place where we feel
extra good about it, then we can do the full review process on what
we've got, which has the added benefit of even more eyeballs and ways
of looking at things. I think the code will benefit from this type of
process.

Maybe a good middle ground would be to take the existing code and put
it in a Wireguard branch. Those who wish to keep Wireguard out of
FreeBSD mainline have done so. FreeBSD users who wish to use Wireguard
can build the Wireguard branch. And those who wish to improve
Wireguard have a working branch for patches. Later, the branch can be
re-merged back to master.

We're actually going to do something like that already. We'll have it
as an out-of-tree module, since it's fairly standalone anyway. And
then when it's ready, we'll send that for merging back into the
FreeBSD main branch. Also, from a technical perspective, dealing with
out of tree modules on FreeBSD seems way, way easier than on Linux.
There's not nearly as much API churn, as far as I can see. We probably
can even offer prebuilts at some point for people who want to test out
snapshots. So I'm really not very worried (at least at the moment; I'm
still new to FreeBSD kernel development).

Jason

[Jason A. Donenfeld]

Hi Kyle,

involved with this announcement that I'm leaving it for now. There's
been too much press surrounding this, and it's distracting me from the
work that I like to do and what I'm typically known for.

Makes sense and is understandable. It's been pretty miserable for all of us.

It looks like we'll eventually find somebody on the FreeBSD side of
things to take over where you left off, but hopefully for now in the
coming weeks things can just level out to some tranquility, so we can
get back to distraction-free coding without all the drama.

Jason

[Jason A. Donenfeld]

Hi Kyle,

I think what you describe is a great plan. I think everybody realizes
at this point that the original code base from the original author
never should have been merged. We went head first in trying to fix it
in a week because we thought that was our only choice. But knowing now
that we can simply remove it, and get back to coding it carefully and
deliberately, is just a huge relief. So that's great. And while it's
under development, we can have an out-of-tree repo for folks to test
out intermediate snapshots and provide feedback, just like the
WireGuard project has always done. In other words, we'll follow the
tried and true formulation of: slow, careful coding + regular
snapshots to receive testing and feedback.

So, I'm quite happy there. And when it is ready, I'm confident it'll
get a thorough review from FreeBSD core developers, which is terrific.
More review ==> better code.

I also want to thank you for your words about Netgate and the various
parties involved. I think nobody wants animosity and tension, and I
imagine your email has helped to calm the tone quite a bit. That's
just the type of reset we need, so that we can get back to what we do
best: writing and refining code.

To others reading, with regards to actual project logistics, I think
what I wrote in the original announcement still stands: we'll have
instructions for module building and such online and we'll announce it
here. And for developers interested, Kyle, MattD, and I have been
coordinating code writing on IRC; if you'd like to join in, ping one
of us there and we'll get you up to speed on repos and ssh keys
whatnot.

Regards,
Jason