lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 25 Oct 2021 12:06:38 +0800 From: Hangbin Liu <liuhangbin@...il.com> To: "Jason A. Donenfeld" <Jason@...c4.com> Cc: netdev@...r.kernel.org, David Miller <davem@...emloft.net>, Xiumei Mu <xmu@...hat.com>, Toke Høiland-Jørgensen <toke@...hat.com>, Paolo Abeni <pabeni@...hat.com>, wireguard@...ts.zx2c4.com Subject: Re: [PATCH net] wireguard: remove peer cache in netns_pre_exit Hi Jason, Do you have a schedule to post this patch to upstream? Thanks Hangbin On Wed, Sep 01, 2021 at 03:55:43PM +0200, Jason A. Donenfeld wrote: > > From f9984a41eeaebfdcef5aba8a71966b77ba0de8c0 Mon Sep 17 00:00:00 2001 > From: "Jason A. Donenfeld" <Jason@...c4.com> > Date: Wed, 1 Sep 2021 14:53:39 +0200 > Subject: [PATCH] wireguard: device: reset peer src endpoint when netns exits > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Each peer's endpoint contains a dst_cache entry that takes a reference > to another netdev. When the containing namespace exits, we take down the > socket and prevent future sockets from being created (by setting > creating_net to NULL), which removes that potential reference on the > netns. However, it doesn't release references to the netns that a netdev > cached in dst_cache might be taking, so the netns still might fail to > exit. Since the socket is gimped anyway, we can simply clear all the > dst_caches (by way of clearing the endpoint src), which will release all > references. > > However, the current dst_cache_reset function only releases those > references lazily. But it turns out that all of our usages of > wg_socket_clear_peer_endpoint_src are called from contexts that are not > exactly high-speed or bottle-necked. For example, when there's > connection difficulty, or when userspace is reconfiguring the interface. > And in particular for this patch, when the netns is exiting. So for > those cases, it makes more sense to call dst_release immediately. For > that, we add a small helper function to dst_cache. > > This patch also adds a test to netns.sh from Hangbin Liu to ensure this > doesn't regress. > > Test-by: Hangbin Liu <liuhangbin@...il.com> > Reported-by: Xiumei Mu <xmu@...hat.com> > Cc: Hangbin Liu <liuhangbin@...il.com> > Cc: Toke Høiland-Jørgensen <toke@...hat.com> > Cc: Paolo Abeni <pabeni@...hat.com> > Fixes: 900575aa33a3 ("wireguard: device: avoid circular netns references") > Signed-off-by: Jason A. Donenfeld <Jason@...c4.com> > --- > drivers/net/wireguard/device.c | 3 +++ > drivers/net/wireguard/socket.c | 2 +- > include/net/dst_cache.h | 11 ++++++++++ > net/core/dst_cache.c | 19 +++++++++++++++++ > tools/testing/selftests/wireguard/netns.sh | 24 +++++++++++++++++++++- > 5 files changed, 57 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c > index 551ddaaaf540..77e64ea6be67 100644 > --- a/drivers/net/wireguard/device.c > +++ b/drivers/net/wireguard/device.c > @@ -398,6 +398,7 @@ static struct rtnl_link_ops link_ops __read_mostly = { > static void wg_netns_pre_exit(struct net *net) > { > struct wg_device *wg; > + struct wg_peer *peer; > > rtnl_lock(); > list_for_each_entry(wg, &device_list, device_list) { > @@ -407,6 +408,8 @@ static void wg_netns_pre_exit(struct net *net) > mutex_lock(&wg->device_update_lock); > rcu_assign_pointer(wg->creating_net, NULL); > wg_socket_reinit(wg, NULL, NULL); > + list_for_each_entry(peer, &wg->peer_list, peer_list) > + wg_socket_clear_peer_endpoint_src(peer); > mutex_unlock(&wg->device_update_lock); > } > } > diff --git a/drivers/net/wireguard/socket.c b/drivers/net/wireguard/socket.c > index 8c496b747108..6f07b949cb81 100644 > --- a/drivers/net/wireguard/socket.c > +++ b/drivers/net/wireguard/socket.c > @@ -308,7 +308,7 @@ void wg_socket_clear_peer_endpoint_src(struct wg_peer *peer) > { > write_lock_bh(&peer->endpoint_lock); > memset(&peer->endpoint.src6, 0, sizeof(peer->endpoint.src6)); > - dst_cache_reset(&peer->endpoint_cache); > + dst_cache_reset_now(&peer->endpoint_cache); > write_unlock_bh(&peer->endpoint_lock); > } > > diff --git a/include/net/dst_cache.h b/include/net/dst_cache.h > index 67634675e919..df6622a5fe98 100644 > --- a/include/net/dst_cache.h > +++ b/include/net/dst_cache.h > @@ -79,6 +79,17 @@ static inline void dst_cache_reset(struct dst_cache *dst_cache) > dst_cache->reset_ts = jiffies; > } > > +/** > + * dst_cache_reset_now - invalidate the cache contents immediately > + * @dst_cache: the cache > + * > + * The caller must be sure there are no concurrent users, as this frees > + * all dst_cache users immediately, rather than waiting for the next > + * per-cpu usage like dst_cache_reset does. Most callers should use the > + * higher speed lazily-freed dst_cache_reset function instead. > + */ > +void dst_cache_reset_now(struct dst_cache *dst_cache); > + > /** > * dst_cache_init - initialize the cache, allocating the required storage > * @dst_cache: the cache > diff --git a/net/core/dst_cache.c b/net/core/dst_cache.c > index be74ab4551c2..0ccfd5fa5cb9 100644 > --- a/net/core/dst_cache.c > +++ b/net/core/dst_cache.c > @@ -162,3 +162,22 @@ void dst_cache_destroy(struct dst_cache *dst_cache) > free_percpu(dst_cache->cache); > } > EXPORT_SYMBOL_GPL(dst_cache_destroy); > + > +void dst_cache_reset_now(struct dst_cache *dst_cache) > +{ > + int i; > + > + if (!dst_cache->cache) > + return; > + > + dst_cache->reset_ts = jiffies; > + for_each_possible_cpu(i) { > + struct dst_cache_pcpu *idst = per_cpu_ptr(dst_cache->cache, i); > + struct dst_entry *dst = idst->dst; > + > + idst->cookie = 0; > + idst->dst = NULL; > + dst_release(dst); > + } > +} > +EXPORT_SYMBOL_GPL(dst_cache_reset_now); > diff --git a/tools/testing/selftests/wireguard/netns.sh b/tools/testing/selftests/wireguard/netns.sh > index 2e5c1630885e..8a9461aa0878 100755 > --- a/tools/testing/selftests/wireguard/netns.sh > +++ b/tools/testing/selftests/wireguard/netns.sh > @@ -613,6 +613,28 @@ ip0 link set wg0 up > kill $ncat_pid > ip0 link del wg0 > > +# Ensure that dst_cache references don't outlive netns lifetime > +ip1 link add dev wg0 type wireguard > +ip2 link add dev wg0 type wireguard > +configure_peers > +ip1 link add veth1 type veth peer name veth2 > +ip1 link set veth2 netns $netns2 > +ip1 addr add fd00:aa::1/64 dev veth1 > +ip2 addr add fd00:aa::2/64 dev veth2 > +ip1 link set veth1 up > +ip2 link set veth2 up > +waitiface $netns1 veth1 > +waitiface $netns2 veth2 > +ip1 -6 route add default dev veth1 via fd00:aa::2 > +ip2 -6 route add default dev veth2 via fd00:aa::1 > +n1 wg set wg0 peer "$pub2" endpoint [fd00:aa::2]:2 > +n2 wg set wg0 peer "$pub1" endpoint [fd00:aa::1]:1 > +n1 ping6 -c 1 fd00::2 > +pp ip netns delete $netns1 > +pp ip netns delete $netns2 > +pp ip netns add $netns1 > +pp ip netns add $netns2 > + > # Ensure there aren't circular reference loops > ip1 link add wg1 type wireguard > ip2 link add wg2 type wireguard > @@ -631,7 +653,7 @@ while read -t 0.1 -r line 2>/dev/null || [[ $? -ne 142 ]]; do > done < /dev/kmsg > alldeleted=1 > for object in "${!objects[@]}"; do > - if [[ ${objects["$object"]} != *createddestroyed ]]; then > + if [[ ${objects["$object"]} != *createddestroyed && ${objects["$object"]} != *createdcreateddestroyeddestroyed ]]; then > echo "Error: $object: merely ${objects["$object"]}" >&3 > alldeleted=0 > fi > -- > 2.32.0
Powered by blists - more mailing lists