On Wed, 27 May 2020 09:34:53 +0200
Martin Pieuchot <m...@openbsd.org> wrote:
> Hello Matt,
>
> Thank you for your submission.
Hi Martin,
No worries, thank you for your feedback. This is something I want to
help make happen and if I recall correctly, someone once said that if I
wanted a new feature on OpenBSD then submit a patch :)
> On 26/05/20(Tue) 19:39, Matt Dunwoodie wrote:
> > After some feedback and comments, we've addressed the concerns, and
> > fixed a few things from our side too. Overall the structure is
> > familiar with no major changes, so any prior readings mostly carry
> > over.
>
> It's hard to review a such big diff. If you're interested in getting
> feedbacks in the code itself I'd suggest you split it :)
I do realise it is a bit unwieldy, but it is a fairly
integrated/cohesive patch. I'll have a think about splitting it up
if/when we get to a rev. 3.
> Regarding the kernel, I'd suggest you use "#if NWG > 0" like it is
> done for other pseudo-drives with 'needs-flag'.
For the most part there is no significant changes to other parts of the
network stack, so I don't believe this should be necessary. If there is
anything in particular that you think should be flagged like that then
please do say.
> Can you re-use PACKET_TAG_GRE instead of introduce
> PACKET_TAG_WIREGUARD? The former is already used by multiple
> pseudo-drivers.
As far as I can tell PACKET_TAG_GRE is only used in gif(4) and gre(4),
however they just use the tag to store the if_index. I also understand
m_pkthdr.ph_tagsset is limited on space so adding a new TAG type is not
a great idea therefore, I've repurposed PACKET_TAG_GIF to
PACKET_TAG_WIREGUARD.
In this case we store a little bit more information (and not the
if_index) and I would not want any kind of conflict between wg(4)
PACKET_TAG_GRE and gif(4)/gre(4) PACKET_TAG_GRE.
> Aren't wg_noise.c and wg_cookie.c meant to be used by if_wg.c only?
> Or would other parts of the kernel benefit from them? If wg(4) is
> the only user I'm questionnings whether putting them in crypto/ is
> the best choice.
Yes, I'm open to moving them somewhere, the were originally in crypto/
as they handled specifically the crypto side of WireGuard. The end
result though, I don't think they should be merged into if_wg.c as they
are designed to be separate to help auditing/review. Would it make
sense to have them in sys/net/?
> Why are you using rwlock to protect cookie_* and noise_* states? Is
> any sleeping involved? Did you consider a mutex with IPL_NONE? rwlock
> introduce sleep point that might be surprising. Did you try the
> option WITNESS of the kernel to check your locking?
Both the cookie_* and noise_* are expected to run concurrently, and I
wouldn't want one thread holding up another with a mutex while
performing (relatively) expensive crypto operations. Where possible and
correct, we use a read lock.
Running with WITNESS does not raise any errors, I've checked.
> Keeping comments away from source code, like you did in wg_cookie.h
> can result in them not being updated. This isn't something common in
> the tree and after some times comments tend to lie :o)
Makes sense, and if it isn't that common I'm more than happy to move
them.
> Would you mind using variables of more than one-letter? That makes is
> harder to grep for patterns.
I'll see what I can do :)
> If you prefix fields of variable, I'd suggest picking the firs letter
> of the two words, for example 'wt_' for "struct wg_tag" instead of
> "t_" currently. Then grepping for 'wt_endpoint' is more likely to
> yield what we're looking for :)
Very reasonable - will do.
> Is mq_push() required? I'm always surprise to see mbuf APIs grow and
> grow over years. Anyway, that could be a separate change.
To be explicit, the behaviour of mq_push is required, that is: we want
to add a packet to the queue; if the queue is full we want to drop the
oldest packet in the queue.
I'm aware of APIs growing though, so understand the concern. While it
would be possible to emulate this behaviour with mq_full, and then some
combination of mq_dequeue and mq_enqueue the result would be racey and
may have unintended side effects. The end goal is that we want to
achieve the behaviour above atomically.
> ioctl(2) shouldn't require the NET_LOCK, so if your pseudo-driver uses
> its own locks to serialize access to its data, please say that in the
> comments for SIOCSWG and SIOCGWG.
Yes, I'll add some comments.
> WG_PEERS_FOREACH{,SAFE} macros are only introducing abstraction and
> make code harder to understand.
Absolutely, I agree here. It was a remnant from some refactoring.
> Please do not cast function pointer in task_set(), use the expected
> prototype.
Noted, will fix.
> Why did you introduce wg_queue_*(), isn't mq_init(9) API good enough?
>
The queueing semantics for WireGuard are a little different from mq_*
for the most part due to encrypting on a taskq with multiple threads.
You may imagine a situation where two threads pull off a packet with
mq_dequeue and spend varying amounts of time processing the packet, due
to packet size, locks or mutexs. The end result is these packets are
likely to be (drastically) reordered which is not something we want.
Instead, wg_queue_* ensures that packets are not reordered in any way
for egress or ingress, while still allowing for them to be processed on
multiple threads in arbitrary order.
> My overall impression is that this can be simplified and integrate
> better in the kernel :) If you could explain the basic architecture
> or point me where you explained it, it would help.
The queueing semantics are described around if_wg.c:1405, which might
help in understanding the reasoning above.
Thanks again for your comments!
Cheers,
Matt
