On 27/05/20(Wed) 20:18, Matt Dunwoodie wrote: > On Wed, 27 May 2020 09:34:53 +0200 > Martin Pieuchot <m...@openbsd.org> wrote: > > 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.
I'm thinking of the `inp_upcall' abstraction and the in6_ifattach() chunk. Is it possible to do without adding a function pointer to "struct inpcb" and guard the logic with #if NWG > 0" instead? I'm not saying that such abstraction is not wanted, but I would prefer we think it through rather than add it for one particular driver/subsystem. Have you seen the "#if NVXLAN" chunk in udp_input()? Maybe this hook could also be considered for the abstraction you're suggesting. > > 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/? In my opinion putting them in sys/net/ is better. > > 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. As long as rwlock are used in wg(4)'s taskqs that makes sense. However in the processing paths of the network stack (wg_start, wg_output, wg_input and if you introduce it wg_enqueue) they should be avoided. The rational is that they introduce sleeping points which we still try to avoid. At least wg_input() calls wg_index_get() that tries to grab a read lock. It would be great if those small iterations could be protected by a mutex. I haven't done a complete audit so they might be others ;) I'd also suggest you look at vlan_enqueue() and see if it makes sense to implement an if_enqueue() routine. That should allow you to avoid useless queueing involving locking, if HFSQ is not used on top of wg(4), and maybe help for the double error accounting ;) > > 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. Sure, please make sure dlg@ can review this change then :)
