On 2020/12/01 21:27, Matt Dunwoodie wrote: > On Tue, 1 Dec 2020 10:32:29 +0100 > Sebastien Marie <sema...@online.fr> wrote: > > > Jason, Matt, > > > > sthen@ told me that the same lock is reported several times (exactly, > > two locks are reported several times: lock1, lock2, lock1, lock2...) > > > > witness(4) reports when a lock doesn't have LO_INITIALIZED flag set in > > internal part of `struct rwlock'. Next it sets it, and skip futher > > analysis for this particular lock. > > > > if the same lock is reported several times, it means the memory is > > zeroed (and so the flag removed). it could be intented or not. but in > > all cases, a rwlock should be properly initialized with rw_init() or > > RWLOCK_INITIALIZER() before use. > > > > I don't have enough experiency with wg(4) stuff to well understand the > > issue. in wg_decap(), the peer is get from a `struct wg_tag` (from > > wg_tag_get()). If i didn't mess myself, the p_remote could come from > > wg_send_initiation() or wg_send_response(). but for both, it comes > > from wg_peer_create(), and p_remote is initialized with > > noise_remote_init() (and so lock have proper rw_init()). > > > > do you have idea on the cause of the lost of the rwlock flag ? > > > > if you want to test it with witness(4) enabled, you will need to build > > a kernel with "option WITNESS" in config file. you could uncomment it > > from sys/arch/amd64/conf/GENERIC.MP, and run make config, make clean, > > make > > Hi, > > This certainly isn't intentional. > > I had a quick look and believe the following patch should fix it. The > rwlock in the antireplay counter is rw_init'ed and then bzero'ed, > matching the symptoms. It also explains why lock1,lock2,lock1,... was > seen as there are generally two counters in use (one for the current > session and one for the previous session). It wouldn't make sense for > r_keypair_lock to be in two different locations for the same peer. > > I haven't tested it my side but I'm quite confident it should fix the > unitialised lock issue. > > Cheers, > Matt
Thanks. Confirmed this works ok and stops witness from triggering, I'll go ahead and I'll commit it. > --- net/wg_noise.c > +++ net/wg_noise.c > @@ -467,8 +467,8 @@ noise_remote_begin_session(struct noise_remote *r) > kp.kp_local_index = hs->hs_local_index; > kp.kp_remote_index = hs->hs_remote_index; > getnanouptime(&kp.kp_birthdate); > - rw_init(&kp.kp_ctr.c_lock, "noise_counter"); > bzero(&kp.kp_ctr, sizeof(kp.kp_ctr)); > + rw_init(&kp.kp_ctr.c_lock, "noise_counter"); > > /* Now we need to add_new_keypair */ > rw_enter_write(&r->r_keypair_lock); >
