Re: "wireguard" implementation improperly merged and needs revert

["Jason A. Donenfeld" <Jason%zx2c4.com@localhost>]

Hi Maxime,

Thanks for your email. Some thoughts and replies below:

On Mon, Aug 24, 2020 at 9:09 AM Maxime Villard <max%m00nbsd.net@localhost> wrote:
> Your email more sounds like you haven't had time to look at the actual code
> and try to have it reverted because you are frustrated that it was developed
> without your benediction.
>
> I understand your sentiment, but your email has obvious mischaracterizations.

No, actually, that isn't the case at all. I've spent several hours
with the code and have a few pages of notes, should Taylor decide that
going with Ozaki-san's code is preferable to the OpenBSD route. This
has nothing to do with spilled grapes or benediction anything like
that. Rather, what you've put in the tree isn't WireGuard; it's not a
correct or a complete implementation, and that's not something that
will be trivially fixed with a few fixes here and a few features
there. We need to take a big fundamental look at how the mechanics of
this are operating.

> To me the status is simple: Ozaki has a solid background of developing
> network components, he wrote a wg implementation 2 years ago by carefully
> following the spec, Taylor imported it a few days ago as a first shot, and
> kept it disabled for the time being. Taylor then started making improvements.
> That's how development happens.

I addressed this notion of "solid background" in my previous email to
Taylor. It seems very clear from reading his code that Ozaki-san is
indeed a terrific network programmer. That's a lot different, though,
from getting a WireGuard implementation secure and complete. For that
reason, I'm here offering to jump into this in basically boundless
capacity so that NetBSD can have an implementation. To be clear, I
come to this mailing list volunteering my own time and energy.

> Maybe it would have been wise to commit it to a separate branch for the time
> being; but WireGuard is no very complex piece of engineering and I can
> understand why Taylor decided to commit the code to the main tree right away.

A proper implementation of WireGuard *is* quite complex, actually. And
that misunderstanding is what's gotten us here. Again, see my previous
reply to Taylor. This isn't a matter of dusting off some old code that
sends packets or something; that simply won't cut it with WireGuard,
and pretending otherwise is incredibly irresponsible. I'll address
your other point about a separate branch below when it comes up again.

> I will note four things on the technical side:
>
> (1) I am not sure we can actually import OpenBSD's code. The OpenBSD kernel
>     isn't remotely as MP-safe as NetBSD's, and a copy-pasta will likely add
>     more bugs than it fixes.

We tried to make the locking pretty granular and correct, and also
most of the crypto we've fully cordoned off into separate stand-alone
components. You might wind up being right in the end, though. This is
one of the things I wanted to look at with Taylor (and you, if you're
interested) in September. It may very well be the case that porting
the OpenBSD code is more arduous, though I suspect it will be less so.
We'll see, and I'm fully open to pursuing either possibility.

> (3) NetBSD has advanced bug detection features (KUBSAN, KASAN, KMSAN, KCSAN),
>     that simply do not exist in other OSes. These automatically detect bugs
>     and vulnerabilities at run time. That's a very strong capability we have
>     developed. You and Matt having reviewed code physically together sounds
>     nice and all; but humans can miss memory corruptions, the above features
>     do not.

I'm not sure this is a relevant technical point. The entire intent of
me writing to Taylor has nothing to do with fixing the inevitable slow
trickle of kernel programming bugs or network programming bugs, but
everything to do with the realities of WireGuard. I have no doubt that
the various instrumentation features in NetBSD will go a long way
toward resulting in a better codebase, and I look forward to
developing with those for whatever path we take.

> (4) Considering the overall poor quality of code coming out of OpenBSD, I
>     wouldn't feel confident with us importing their code, whether it be wg,
>     or anything else at large.

Again, I'm not sure this is a relevant technical point, and I *really*
don't want to wade into BSD political rivalries on a mailing list like
that. But I will say that neither Matt nor myself are actual OpenBSD
developers, so maybe you don't need to really have concern about the
pedigree.

> All of this being said, I believe we are all interested in getting the best
> possible WireGuard implementation, so let's not argue over unimportant
> matters.
>
> You and Taylor should definitely talk in September (in one week). Meanwhile
> reverting the code altogether sounds like a very big step that so far has
> received little technical justification.

If you haven't noticed, I'm trying very hard not to get bogged down in
classic mailing list errors of secumbing to, "oh, there's a problem?
make a list of bugs for us and we'll prioritize them and then fix them
one by one," because that _entirely_ misses the point here, and the
current issues won't be coherently addressed in that manner.
Similarly, "but the code works for me for years, it sends packets"
also misses the point entirely. The right thing to do is back up the
truck a bit and reapproach the problem space more carefully.

> However, moving it to a development
> branch would probably be a good move; it would eliminate the confusion as to
> whether it is production-ready (which it isn't yet), while still allowing
> people to make changes and development to happen. Jason/Taylor, does that
> sound good to you?

Actually, yes, sure. If you want to put this in a "wgdev" branch, or
some such name, and move it out of the master development branch,
that'd be productive and also give us some more room to unpack things
-- specifically, a wg-specific feature branch, rather than a "next
version" branch or something. When I asked that you revert the code, I
didn't mean, "revert it and place it in the dumpster", but was more
thinking of moving it to a development tree somewhere, because with
Linux development, things usually work with individual developer
trees. If NetBSD has feature-development branches within its main
repository, that works well.I wouldn't want to totally discard
Taylor's improvements on Ozaki-san's code, especially as that might
wind up being the basis of our work on this moving forward. In other
words, wherever it makes sense to develop it, let's do that, so long
as we merge it to the master tree *not before* it's ready. (With that
said, is a cvs branch what you had in mind? Or does NetBSD have a
pretty established flow for git/hg feature development branches
instead?)

Jason