Hello Matt,

Thank you for your submission.

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 :)

First question is, is it possible to use the wg(4) interface without a
port?

Regarding the kernel, I'd suggest you use "#if NWG > 0" like it is done
for other pseudo-drives with 'needs-flag'.

Can you re-use PACKET_TAG_GRE instead of introduce PACKET_TAG_WIREGUARD?
The former is already used by multiple pseudo-drivers.

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.

The blake implementation and ChaCha extensions could be reviewed
independently.

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?

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)

Would you mind using variables of more than one-letter?  That makes is
harder to grep for patterns.

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 :)

Is mq_push() required?  I'm always surprise to see mbuf APIs grow and
grow over years.  Anyway, that could be a separate change.

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.

Is IFT_WIREGUARD needed?  Isn't the interface a point-to-point interface?

WG_PEERS_FOREACH{,SAFE} macros are only introducing abstraction and
make code harder to understand.

Please do not cast function pointer in task_set(), use the expected
prototype.

Why did you introduce wg_queue_*(), isn't mq_init(9) API good enough?

Why did you re-implemented a routing table?  Did you consider storing
the information you need in existing data structures?    

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.

Thanks

Reply via email to