[PATCH] Merge two rcu types

[PATCH] Merge two rcu types

[Bruno Wolff III]

Paul McKenney made it harder to mess up ending rcu sections with an
incorrect function call by using the same functions to end multiple
types of rcu sections.

I replaced synchronize_rcu_bh with synchronize_rcu, rcu_barrier_bh
with rcu_barrier and call_rcu_bh with call_rcu.

I'm not sure how this should be done with compat. Using the old names
and fixing things with a macro isn't going to be liked for new code.

I don't know for sure this is really the correct way to fix things.
---
 src/allowedips.c | 6 +++---
 src/device.c     | 6 +++---
 src/noise.c      | 2 +-
 src/peer.c       | 8 ++++----
 src/socket.c     | 2 +-
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/allowedips.c b/src/allowedips.c
index bfb6020f350e..592501e39bc3 100644
--- a/src/allowedips.c
+++ b/src/allowedips.c
@@ -112,7 +112,7 @@ static void walk_remove_by_peer(struct allowedips_node __rcu **top,
 				if (!node->bit[0] || !node->bit[1]) {
 					rcu_assign_pointer(*nptr, DEREF(
 					       &node->bit[!REF(node->bit[0])]));
-					call_rcu_bh(&node->rcu, node_free_rcu);
+					call_rcu(&node->rcu, node_free_rcu);
 					node = DEREF(nptr);
 				}
 			}
@@ -300,12 +300,12 @@ void wg_allowedips_free(struct allowedips *table, struct mutex *lock)
 	RCU_INIT_POINTER(table->root6, NULL);
 	if (rcu_access_pointer(old4)) {
 		root_remove_peer_lists(old4);
-		call_rcu_bh(&rcu_dereference_protected(old4,
+		call_rcu(&rcu_dereference_protected(old4,
 				lockdep_is_held(lock))->rcu, root_free_rcu);
 	}
 	if (rcu_access_pointer(old6)) {
 		root_remove_peer_lists(old6);
-		call_rcu_bh(&rcu_dereference_protected(old6,
+		call_rcu(&rcu_dereference_protected(old6,
 				lockdep_is_held(lock))->rcu, root_free_rcu);
 	}
 }
diff --git a/src/device.c b/src/device.c
index 2866dd98ff4e..779c41521ea9 100644
--- a/src/device.c
+++ b/src/device.c
@@ -94,7 +94,7 @@ static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
 		mutex_unlock(&wg->device_update_lock);
 	}
 	rtnl_unlock();
-	rcu_barrier_bh();
+	rcu_barrier();
 	return 0;
 }
 
@@ -244,7 +244,7 @@ static void wg_destruct(struct net_device *dev)
 	destroy_workqueue(wg->packet_crypt_wq);
 	wg_packet_queue_free(&wg->decrypt_queue, true);
 	wg_packet_queue_free(&wg->encrypt_queue, true);
-	rcu_barrier_bh(); /* Wait for all the peers to be actually freed. */
+	rcu_barrier(); /* Wait for all the peers to be actually freed. */
 	wg_ratelimiter_uninit();
 	memzero_explicit(&wg->static_identity, sizeof(wg->static_identity));
 	skb_queue_purge(&wg->incoming_handshakes);
@@ -468,5 +468,5 @@ void wg_device_uninit(void)
 #ifdef CONFIG_PM_SLEEP
 	unregister_pm_notifier(&pm_notifier);
 #endif
-	rcu_barrier_bh();
+	rcu_barrier();
 }
diff --git a/src/noise.c b/src/noise.c
index 4405125e7a0a..2e05e2740685 100644
--- a/src/noise.c
+++ b/src/noise.c
@@ -132,7 +132,7 @@ static void keypair_free_kref(struct kref *kref)
 			    keypair->entry.peer->internal_id);
 	wg_index_hashtable_remove(keypair->entry.peer->device->index_hashtable,
 				  &keypair->entry);
-	call_rcu_bh(&keypair->rcu, keypair_free_rcu);
+	call_rcu(&keypair->rcu, keypair_free_rcu);
 }
 
 void wg_noise_keypair_put(struct noise_keypair *keypair, bool unreference_now)
diff --git a/src/peer.c b/src/peer.c
index 996f40b919ca..0c7e942f6893 100644
--- a/src/peer.c
+++ b/src/peer.c
@@ -99,7 +99,7 @@ static void peer_make_dead(struct wg_peer *peer)
 	/* Mark as dead, so that we don't allow jumping contexts after. */
 	WRITE_ONCE(peer->is_dead, true);
 
-	/* The caller must now synchronize_rcu_bh() for this to take effect. */
+	/* The caller must now synchronize_rcu() for this to take effect. */
 }
 
 static void peer_remove_after_dead(struct wg_peer *peer)
@@ -171,7 +171,7 @@ void wg_peer_remove(struct wg_peer *peer)
 	lockdep_assert_held(&peer->device->device_update_lock);
 
 	peer_make_dead(peer);
-	synchronize_rcu_bh();
+	synchronize_rcu();
 	peer_remove_after_dead(peer);
 }
 
@@ -189,7 +189,7 @@ void wg_peer_remove_all(struct wg_device *wg)
 		peer_make_dead(peer);
 		list_add_tail(&peer->peer_list, &dead_peers);
 	}
-	synchronize_rcu_bh();
+	synchronize_rcu();
 	list_for_each_entry_safe(peer, temp, &dead_peers, peer_list)
 		peer_remove_after_dead(peer);
 }
@@ -228,7 +228,7 @@ static void kref_release(struct kref *refcount)
 	wg_packet_purge_staged_packets(peer);
 
 	/* Free the memory used. */
-	call_rcu_bh(&peer->rcu, rcu_release);
+	call_rcu(&peer->rcu, rcu_release);
 }
 
 void wg_peer_put(struct wg_peer *peer)
diff --git a/src/socket.c b/src/socket.c
index 652d79817e58..5a77b0c9ba2e 100644
--- a/src/socket.c
+++ b/src/socket.c
@@ -426,7 +426,7 @@ void wg_socket_reinit(struct wg_device *wg, struct sock *new4,
 	if (new4)
 		wg->incoming_port = ntohs(inet_sk(new4)->inet_sport);
 	mutex_unlock(&wg->socket_update_lock);
-	synchronize_rcu_bh();
+	synchronize_rcu();
 	synchronize_net();
 	sock_free(old4);
 	sock_free(old6);
-- 
2.21.0

_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [PATCH] Merge two rcu types

[Bruno Wolff III]

On Thu, Mar 14, 2019 at 00:16:08 -0500,
  Bruno Wolff III <bruno@wolff.to> wrote:
>Paul McKenney made it harder to mess up ending rcu sections with an
>incorrect function call by using the same functions to end multiple
>types of rcu sections.

There are a number of commits involved in this change, but this commit 
has a pretty explicit comment about doing the renames I did.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c8d1da4000b0b95bf95d3e13b7450eec5428da1e
netfilter: Replace call_rcu_bh(), rcu_barrier_bh(), and synchronize_rcu_bh()
Now that call_rcu()'s callback is not invoked until after bh-disable
regions of code have completed (in addition to explicitly marked
RCU read-side critical sections), call_rcu() can be used in place
of call_rcu_bh().  Similarly, rcu_barrier() can be used in place of
rcu_barrier_bh() and synchronize_rcu() in place of synchronize_rcu_bh().
This commit therefore makes these changes.

In case it isn't obvious, the patch I supplied is only good for 5.1+ 
kernels. I'm not sure how you wanted to handle doing compatibility for 
older kernels and didn't even have a good idea how to start.
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [PATCH] Merge two rcu types

[Bruno Wolff III]

On Thu, Mar 14, 2019 at 07:32:11 -0500,
  Bruno Wolff III <bruno@wolff.to> wrote:
>
>In case it isn't obvious, the patch I supplied is only good for 5.1+ 
>kernels. I'm not sure how you wanted to handle doing compatibility for 
>older kernels and didn't even have a good idea how to start.

If I want to try to rework this myself so that the main code is going to 
be usable upstream, but it still works with older kernels, is there a 
suggested approach. Using the old names in the main code using macros 
to make it work isn't going to fly upstream. I'm not sure that replicating 
large chunks of code in compat is a good idea either.
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [PATCH] Merge two rcu types

[Jason A. Donenfeld]

Ideally the way to fix this would be to replace our usages with the
proper non-_bh ones, and then in compat, based on a version
comparison, define the non-_bh to the _bh. The problem is that
ratelimiter.c uses non-_bh already, and it's unclear how bad it would
be if this suddenly became _bh on old kernels.

IIRC, the _bh variant of those functions has been aliased to non-_bh
since a few versions. Do you know the first time it was the same?
Perhaps if that's far enough back, then it'd be worth the effort to
try to qualify the breakage of improperly _bh-ing ratelimiter.c would
be on those kernels. On the other hand, if it's too recent or if the
breakage would actually be significant, we could always fallback to
the `#ifndef COMPAT_CANNOT_*` pattern used in a few other places
(which gets stripped out automatically for upstream submissions).
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [PATCH] Merge two rcu types

[Bruno Wolff III]

On Thu, Mar 14, 2019 at 12:58:41 -0600,
  "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
>IIRC, the _bh variant of those functions has been aliased to non-_bh
>since a few versions. Do you know the first time it was the same?

At this point I don't know. I knew of the change because I usually like to 
watch Paul's talks and watched his LCA talk not too long ago. I got the 
impression that he started working on the change in the last year, so it 
probably doesn't go back too many kernels.

I should be able to look at this some this weekend. It might be beyond my skill 
to do acceptibly, but I'm interested in giving it a shot. rc1 won't be out 
until the end of the weekend so there is a bit of time yet.
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

[PATCH] global: the _bh variety of rcu helpers have been unified

[Jason A. Donenfeld]

---
Hey Bruno,

Based on your research, how does the below strike you? It's certainly
not pretty, but I'm struggling to come up with a better solution.

Jason


 src/allowedips.c    |  6 +++---
 src/compat/compat.h | 11 +++++++++++
 src/device.c        |  6 +++---
 src/noise.c         |  2 +-
 src/peer.c          |  8 ++++----
 src/ratelimiter.c   |  9 +++++++++
 src/socket.c        |  2 +-
 7 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/src/allowedips.c b/src/allowedips.c
index bfb6020f..592501e3 100644
--- a/src/allowedips.c
+++ b/src/allowedips.c
@@ -112,7 +112,7 @@ static void walk_remove_by_peer(struct allowedips_node __rcu **top,
 				if (!node->bit[0] || !node->bit[1]) {
 					rcu_assign_pointer(*nptr, DEREF(
 					       &node->bit[!REF(node->bit[0])]));
-					call_rcu_bh(&node->rcu, node_free_rcu);
+					call_rcu(&node->rcu, node_free_rcu);
 					node = DEREF(nptr);
 				}
 			}
@@ -300,12 +300,12 @@ void wg_allowedips_free(struct allowedips *table, struct mutex *lock)
 	RCU_INIT_POINTER(table->root6, NULL);
 	if (rcu_access_pointer(old4)) {
 		root_remove_peer_lists(old4);
-		call_rcu_bh(&rcu_dereference_protected(old4,
+		call_rcu(&rcu_dereference_protected(old4,
 				lockdep_is_held(lock))->rcu, root_free_rcu);
 	}
 	if (rcu_access_pointer(old6)) {
 		root_remove_peer_lists(old6);
-		call_rcu_bh(&rcu_dereference_protected(old6,
+		call_rcu(&rcu_dereference_protected(old6,
 				lockdep_is_held(lock))->rcu, root_free_rcu);
 	}
 }
diff --git a/src/compat/compat.h b/src/compat/compat.h
index 2dcdbaeb..8662b835 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -778,6 +778,17 @@ struct __kernel_timespec {
 #define skb_probe_transport_header(a) skb_probe_transport_header(a, 0)
 #endif
 
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 1, 0)
+/* Note that all intentional uses of the non-_bh variety need to explicitly
+ * undef these, conditionalized on COMPAT_CANNOT_DEPRECIATE_BH_RCU.
+ */
+#define synchronize_rcu synchronize_rcu_bh
+#define call_rcu call_rcu_bh
+#define rcu_barrier rcu_barrier_bh
+#define COMPAT_CANNOT_DEPRECIATE_BH_RCU
+#endif
+
 /* https://github.com/ClangBuiltLinux/linux/issues/7 */
 #if defined( __clang__) && (!defined(CONFIG_CLANG_VERSION) || CONFIG_CLANG_VERSION < 80000)
 #include <linux/bug.h>
diff --git a/src/device.c b/src/device.c
index 2866dd98..779c4152 100644
--- a/src/device.c
+++ b/src/device.c
@@ -94,7 +94,7 @@ static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
 		mutex_unlock(&wg->device_update_lock);
 	}
 	rtnl_unlock();
-	rcu_barrier_bh();
+	rcu_barrier();
 	return 0;
 }
 
@@ -244,7 +244,7 @@ static void wg_destruct(struct net_device *dev)
 	destroy_workqueue(wg->packet_crypt_wq);
 	wg_packet_queue_free(&wg->decrypt_queue, true);
 	wg_packet_queue_free(&wg->encrypt_queue, true);
-	rcu_barrier_bh(); /* Wait for all the peers to be actually freed. */
+	rcu_barrier(); /* Wait for all the peers to be actually freed. */
 	wg_ratelimiter_uninit();
 	memzero_explicit(&wg->static_identity, sizeof(wg->static_identity));
 	skb_queue_purge(&wg->incoming_handshakes);
@@ -468,5 +468,5 @@ void wg_device_uninit(void)
 #ifdef CONFIG_PM_SLEEP
 	unregister_pm_notifier(&pm_notifier);
 #endif
-	rcu_barrier_bh();
+	rcu_barrier();
 }
diff --git a/src/noise.c b/src/noise.c
index 4405125e..2e05e274 100644
--- a/src/noise.c
+++ b/src/noise.c
@@ -132,7 +132,7 @@ static void keypair_free_kref(struct kref *kref)
 			    keypair->entry.peer->internal_id);
 	wg_index_hashtable_remove(keypair->entry.peer->device->index_hashtable,
 				  &keypair->entry);
-	call_rcu_bh(&keypair->rcu, keypair_free_rcu);
+	call_rcu(&keypair->rcu, keypair_free_rcu);
 }
 
 void wg_noise_keypair_put(struct noise_keypair *keypair, bool unreference_now)
diff --git a/src/peer.c b/src/peer.c
index 996f40b9..0c7e942f 100644
--- a/src/peer.c
+++ b/src/peer.c
@@ -99,7 +99,7 @@ static void peer_make_dead(struct wg_peer *peer)
 	/* Mark as dead, so that we don't allow jumping contexts after. */
 	WRITE_ONCE(peer->is_dead, true);
 
-	/* The caller must now synchronize_rcu_bh() for this to take effect. */
+	/* The caller must now synchronize_rcu() for this to take effect. */
 }
 
 static void peer_remove_after_dead(struct wg_peer *peer)
@@ -171,7 +171,7 @@ void wg_peer_remove(struct wg_peer *peer)
 	lockdep_assert_held(&peer->device->device_update_lock);
 
 	peer_make_dead(peer);
-	synchronize_rcu_bh();
+	synchronize_rcu();
 	peer_remove_after_dead(peer);
 }
 
@@ -189,7 +189,7 @@ void wg_peer_remove_all(struct wg_device *wg)
 		peer_make_dead(peer);
 		list_add_tail(&peer->peer_list, &dead_peers);
 	}
-	synchronize_rcu_bh();
+	synchronize_rcu();
 	list_for_each_entry_safe(peer, temp, &dead_peers, peer_list)
 		peer_remove_after_dead(peer);
 }
@@ -228,7 +228,7 @@ static void kref_release(struct kref *refcount)
 	wg_packet_purge_staged_packets(peer);
 
 	/* Free the memory used. */
-	call_rcu_bh(&peer->rcu, rcu_release);
+	call_rcu(&peer->rcu, rcu_release);
 }
 
 void wg_peer_put(struct wg_peer *peer)
diff --git a/src/ratelimiter.c b/src/ratelimiter.c
index b3b1d258..0780d113 100644
--- a/src/ratelimiter.c
+++ b/src/ratelimiter.c
@@ -3,6 +3,15 @@
  * Copyright (C) 2015-2019 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
  */
 
+#ifdef COMPAT_CANNOT_DEPRECIATE_BH_RCU
+/* We normally alias all non-_bh functions to the _bh ones in the compat layer,
+ * but that's not appropriate here, where we actually do want _bh ones.
+ */
+#undef synchronize_rcu
+#undef call_rcu
+#undef rcu_barrier
+#endif
+
 #include "ratelimiter.h"
 #include <linux/siphash.h>
 #include <linux/mm.h>
diff --git a/src/socket.c b/src/socket.c
index 652d7981..5a77b0c9 100644
--- a/src/socket.c
+++ b/src/socket.c
@@ -426,7 +426,7 @@ void wg_socket_reinit(struct wg_device *wg, struct sock *new4,
 	if (new4)
 		wg->incoming_port = ntohs(inet_sk(new4)->inet_sport);
 	mutex_unlock(&wg->socket_update_lock);
-	synchronize_rcu_bh();
+	synchronize_rcu();
 	synchronize_net();
 	sock_free(old4);
 	sock_free(old6);
-- 
2.21.0

_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [PATCH] global: the _bh variety of rcu helpers have been unified

[Bruno Wolff III]

On Thu, Mar 14, 2019 at 23:14:39 -0600,
  "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
>---
>Hey Bruno,
>
>Based on your research, how does the below strike you? It's certainly
>not pretty, but I'm struggling to come up with a better solution.

I think anything that doesn't try to keep the _bh versions around is going to 
be ugly. But keeping the _bh versions is not going to be acceptible upstream, 
so that doesn't seem to be an option. What you did looks pretty good 
given the constraints.

In the talk I saw, the motivation for the change was that mismatching 
rcu functions caused security issues, though I don't know how practical 
it is to exploit those mistakes. So I worry a bit about breaking things 
in old kernels accidentally, in the future. I don't have any good ideas for 
automatically catching misuse in new code.

I did try the version you committed, yesterday, and it seemed to work 
fine on a 5.1 kernel.
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard