WireGuard implementation for the OpenBSD kernel https://git.zx2c4.com/wireguard-openbsd/atom/share?h=master 2021-04-13T05:47:30Z 2021-04-13T05:47:30Z Matt Dunwoodie ncon@noconroy.net 2021-04-03T13:15:47Z urn:sha1:e447e5e6911f4dc85b71b8683614807a4bf342e1 This function (or of similar nature) is required to safely use a refcnt and smr_entry together. Such functions exist on other platforms as kref_get_unless_zero (on Linux) and refcount_acquire_if_gt (on FreeBSD). The following diagram details the following situation with and without refcnt_take_if_gt in 3 cases, with the first showing the "invalid" use of refcnt_take. Situation: Thread #1 is removing the global referenc (o). Thread #2 wants to reference an object (r), using a thread pointer (t). Case: 1) refcnt_take after Thread #1 has released "o" 2) refcnt_take_if_gt before Thread #1 has released "o" 3) refcnt_take_if_gt after Thread #1 has released "o" Data: struct obj { struct smr_entry smr; struct refcnt refcnt; } *o, *r, *t1, *t2; Thread #1 | Thread #2 ---------------------------------+------------------------------------ | r = NULL; rw_enter_write(&lock); | smr_read_enter(); | t1 = SMR_PTR_GET_LOCKED(&o); | t2 = SMR_PTR_GET(&o); SMR_PTR_SET_LOCKED(&o, NULL); | | if (refcnt_rele(&t1->refcnt) | smr_call(&t1->smr, free, t1); | | if (t2 != NULL) { | refcnt_take(&t2->refcnt); | r = t2; | } rw_exit_write(&lock); | smr_read_exit(); ..... // called by smr_thread | free(t1); | ..... | // use after free | *r ---------------------------------+------------------------------------ | r = NULL; rw_enter_write(&lock); | smr_read_enter(); | t1 = SMR_PTR_GET_LOCKED(&o); | t2 = SMR_PTR_GET(&o); SMR_PTR_SET_LOCKED(&o, NULL); | | if (refcnt_rele(&t1->refcnt) | smr_call(&t1->smr, free, t1); | | if (t2 != NULL && | refcnt_take_if_gt(&t2->refcnt, 0)) | r = t2; rw_exit_write(&lock); | smr_read_exit(); ..... // called by smr_thread | // we don't have a valid reference free(t1); | assert(r == NULL); ---------------------------------+------------------------------------ | r = NULL; rw_enter_write(&lock); | smr_read_enter(); | t1 = SMR_PTR_GET_LOCKED(&o); | t2 = SMR_PTR_GET(&o); SMR_PTR_SET_LOCKED(&o, NULL); | | if (t2 != NULL && | refcnt_take_if_gt(&t2->refcnt, 0)) | r = t2; if (refcnt_rele(&t1->refcnt) | smr_call(&t1->smr, free, t1); | rw_exit_write(&lock); | smr_read_exit(); ..... | // we need to put our reference | if (refcnt_rele(&t2->refcnt)) | smr_call(&t2->smr, free, t2); ..... // called by smr_thread | free(t1); | ---------------------------------+------------------------------------ Currently it uses atomic_add_int_nv to atomically read the refcnt, but I'm open to suggestions for better ways. The atomic_cas_uint is used to ensure that refcnt hasn't been modified since reading `old`. 2021-04-02T06:09:44Z jmc jmc@openbsd.org 2021-04-02T06:09:44Z urn:sha1:98a1f272468e346492fe501e54186682dbcccf12 2021-04-02T01:56:20Z dv dv@openbsd.org 2021-04-02T01:56:20Z urn:sha1:01b1f408bce69bf247a47268f5b37515452b6a01 2021-03-31T08:00:57Z espie espie@openbsd.org 2021-03-31T08:00:57Z urn:sha1:3d3bcb1e65be1e527fb35eb4dcd56dcbfd0f78f1 2021-03-28T12:10:05Z sthen sthen@openbsd.org 2021-03-28T12:10:05Z urn:sha1:1b066e180f056ae6d5f3dcae2d9bb44f3a4b3133 2021-03-26T22:41:06Z mvs mvs@openbsd.org 2021-03-26T22:41:06Z urn:sha1:51772eb6ca0378a11bd1f4965abf57cb7fb874bc case when requested table is already exists. Except initialization time, route_output() and if_createrdomain() are the only paths where we call rtable_add(9). We check requested table existence by rtable_exists(9) and it's not the error condition if the table exists. Otherwise we are trying to create requested table by rtable_add(9). Those paths are kernel locked so concurrent thread can't create requested table just after rtable_exists(9) check. Also rtable_add(9) has internal rtable_exists(9) check and in this case the table existence assumed as EEXIST error. This error path is never reached. We are going to unlock PF_ROUTE sockets. This means route_output() will not be serialized with if_createrdomain() and concurrent thread could create requested table. Table existence check and creation should be serialized and it makes sense to do this within rtable_add(9). This time kernel lock is used for this so it pushed down to rtable_add(9). The internal rtable_exists(9) check was modified and table existence is not error now. Since the external rtable_exists(9) check is useless it was removed from if_createrdomain(). It still exists in route_output() path because the logic is more complicated here. ok mpi@ 2021-03-23T19:37:51Z sthen sthen@openbsd.org 2021-03-23T19:37:51Z urn:sha1:87bdd1c1f5079703296c27c75010d75744e0d3f3 textual-convention so is sometimes printed as hex dump) to DisplayString (ascii). likewise change pfLabelName and pfTblName to SnmpAdminString (UTF-8 textual-convention). feedback/tweaks (notably pointing out UTF-8)/ok martijn@ 2021-03-23T16:26:53Z claudio claudio@openbsd.org 2021-03-23T16:26:53Z urn:sha1:7949ec392ddd75cbd0e74690c271875a88f7edcb OK kn@ 2021-03-23T13:22:16Z abieber abieber@openbsd.org 2021-03-23T13:22:16Z urn:sha1:b27aa8c48c0ab366232d47a9f3be259e9d8e2fb6 Patch from Josh Rickmar. Ty jrick! 2021-03-22T07:34:34Z espie espie@openbsd.org 2021-03-22T07:34:34Z urn:sha1:56d14dabd20c5d3483dccb42ce0972af57872ce8