diff mbox series

[Openvpn-devel,net,v6,5/6] ovpn: run deferred work on a module-owned workqueue

Message ID 51a8febf0da1fe17e5fc0b857e8d0a8797002ea2.1785318038.git.ralf@mandelbit.com
State Accepted
Headers
Series [Openvpn-devel,net,v6,1/6] ovpn: fix NULL dereference when killing missing key |

Commit Message

Ralf Lici July 29, 2026, 10:21 a.m. UTC 
  ovpn queues several work items whose callbacks execute module text.
These works currently run on the global system workqueues, so module
exit has no driver-owned drain point that guarantees the callbacks have
fully returned before the module text can be freed.

Object references protect the objects used by the callbacks, but they do
not prove that a workqueue function has returned. In particular, a
worker can drop the final reference that unblocks device teardown while
it is still executing ovpn code.

Add a module-owned workqueue and queue all ovpn work items on it. During
module exit, unregister rtnl and netlink first, flush the workqueue so
ordinary ovpn workers finish, run the final RCU barrier, and destroy the
workqueue last. This keeps the workqueue available for cleanup work
queued from RCU callbacks, while ensuring no ovpn work item can outlive
the module text.

The per-device delayed keepalive work remains explicitly disabled during
netdev teardown (disable_delayed_work_sync in ndo_uninit), since
flush_workqueue does not flush delayed work that is still only pending
on its timer.

Fixes: 3ecfd9349f40 ("ovpn: implement keepalive mechanism")
Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
Changes since v5 https://lore.kernel.org/openvpn-devel/d530ecfc3719845075fbddd0cd7c34752bc2ce16.1783336121.git.ralf@mandelbit.com/
- Update the module-exit ordering for the queue_rcu_work-based key-slot
  release: flush ordinary ovpn work before rcu_barrier, then destroy the
  workqueue after RCU callbacks have queued their cleanup work.

Changes since v4 https://lore.kernel.org/openvpn-devel/6edfcc51e0855bfd34286b86d4e7f26bb3bcd3f7.1783099626.git.ralf@mandelbit.com/
- Rebase on the pending keepalive and TCP deferred-work refcount fixes,
  preserving their hold-before-queue and queue-failure put handling when
  converting schedule_work to queue_work.

Changes since v3 https://lore.kernel.org/openvpn-devel/49f38f89340e18ed30543d3990a7a7e20595b6af.1783080055.git.ralf@mandelbit.com/
- Replace the RCU-deferred peer netdev reference release with a
  module-owned workqueue that drains all ovpn work callbacks before
  module text can be freed.

 drivers/net/ovpn/main.c     | 19 ++++++++++++++++++-
 drivers/net/ovpn/ovpnpriv.h |  4 ++++
 drivers/net/ovpn/peer.c     |  8 ++++----
 drivers/net/ovpn/tcp.c      |  9 ++++-----
 4 files changed, 30 insertions(+), 10 deletions(-)

Comments

Antonio Quartulli Aug. 7, 2026, 6:36 a.m. UTC | #1 
Hi Ralf,

On 29/07/2026 12:21, Ralf Lici wrote:
> ovpn queues several work items whose callbacks execute module text.
> These works currently run on the global system workqueues, so module
> exit has no driver-owned drain point that guarantees the callbacks have
> fully returned before the module text can be freed.
> 
> Object references protect the objects used by the callbacks, but they do
> not prove that a workqueue function has returned. In particular, a
> worker can drop the final reference that unblocks device teardown while
> it is still executing ovpn code.
> 
> Add a module-owned workqueue and queue all ovpn work items on it. During
> module exit, unregister rtnl and netlink first, flush the workqueue so
> ordinary ovpn workers finish, run the final RCU barrier, and destroy the
> workqueue last. This keeps the workqueue available for cleanup work
> queued from RCU callbacks, while ensuring no ovpn work item can outlive
> the module text.
> 
> The per-device delayed keepalive work remains explicitly disabled during
> netdev teardown (disable_delayed_work_sync in ndo_uninit), since
> flush_workqueue does not flush delayed work that is still only pending
> on its timer.
> 
> Fixes: 3ecfd9349f40 ("ovpn: implement keepalive mechanism")
> Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
> Signed-off-by: Ralf Lici <ralf@mandelbit.com>
> ---
> Changes since v5 https://lore.kernel.org/openvpn-devel/d530ecfc3719845075fbddd0cd7c34752bc2ce16.1783336121.git.ralf@mandelbit.com/
> - Update the module-exit ordering for the queue_rcu_work-based key-slot
>    release: flush ordinary ovpn work before rcu_barrier, then destroy the
>    workqueue after RCU callbacks have queued their cleanup work.
> 
> Changes since v4 https://lore.kernel.org/openvpn-devel/6edfcc51e0855bfd34286b86d4e7f26bb3bcd3f7.1783099626.git.ralf@mandelbit.com/
> - Rebase on the pending keepalive and TCP deferred-work refcount fixes,
>    preserving their hold-before-queue and queue-failure put handling when
>    converting schedule_work to queue_work.
> 
> Changes since v3 https://lore.kernel.org/openvpn-devel/49f38f89340e18ed30543d3990a7a7e20595b6af.1783080055.git.ralf@mandelbit.com/
> - Replace the RCU-deferred peer netdev reference release with a
>    module-owned workqueue that drains all ovpn work callbacks before
>    module text can be freed.
> 
>   drivers/net/ovpn/main.c     | 19 ++++++++++++++++++-
>   drivers/net/ovpn/ovpnpriv.h |  4 ++++
>   drivers/net/ovpn/peer.c     |  8 ++++----
>   drivers/net/ovpn/tcp.c      |  9 ++++-----
>   4 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
> index 5093a3b5aba6..80d10f9ef7d8 100644
> --- a/drivers/net/ovpn/main.c
> +++ b/drivers/net/ovpn/main.c
> @@ -12,6 +12,7 @@
>   #include <linux/module.h>
>   #include <linux/netdevice.h>
>   #include <linux/inetdevice.h>
> +#include <linux/workqueue.h>
>   #include <net/gro_cells.h>
>   #include <net/ip.h>
>   #include <net/rtnetlink.h>
> @@ -26,6 +27,9 @@
>   #include "tcp.h"
>   #include "udp.h"
>   
> +/* module-owned workqueue on which all ovpn-specific work is queued */
> +struct workqueue_struct *ovpn_wq;
> +
>   static void ovpn_priv_free(struct net_device *net)
>   {
>   	struct ovpn_priv *ovpn = netdev_priv(net);
> @@ -237,10 +241,16 @@ static int __init ovpn_init(void)
>   
>   	ovpn_tcp_init();
>   
> +	ovpn_wq = alloc_workqueue("ovpn", 0, 0);

This is triggering:


workqueue: ovpn is using neither WQ_PERCPU or WQ_UNBOUND. Setting WQ_PERCPU.
WARNING: kernel/workqueue.c:5852 at __alloc_workqueue+0x1cf5/0x2060, 
CPU#0: swapper/0/1


Can you fix it and resend this patch only please?


Regards,
Ralf Lici Aug. 7, 2026, 6:52 a.m. UTC | #2 
From: ralf@mandelbit.com

On Fri, 07 Aug 2026 08:36:20 +0200, Antonio Quartulli <a@unstable.cc> wrote:
> Hi Ralf,
>
> On 29/07/2026 12:21, Ralf Lici wrote:
> > ovpn queues several work items whose callbacks execute module text.
> > These works currently run on the global system workqueues, so module
> > exit has no driver-owned drain point that guarantees the callbacks have
> > fully returned before the module text can be freed.
> > 
> > Object references protect the objects used by the callbacks, but they do
> > not prove that a workqueue function has returned. In particular, a
> > worker can drop the final reference that unblocks device teardown while
> > it is still executing ovpn code.
> > 
> > Add a module-owned workqueue and queue all ovpn work items on it. During
> > module exit, unregister rtnl and netlink first, flush the workqueue so
> > ordinary ovpn workers finish, run the final RCU barrier, and destroy the
> > workqueue last. This keeps the workqueue available for cleanup work
> > queued from RCU callbacks, while ensuring no ovpn work item can outlive
> > the module text.
> > 
> > The per-device delayed keepalive work remains explicitly disabled during
> > netdev teardown (disable_delayed_work_sync in ndo_uninit), since
> > flush_workqueue does not flush delayed work that is still only pending
> > on its timer.
> > 
> > Fixes: 3ecfd9349f40 ("ovpn: implement keepalive mechanism")
> > Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
> > Signed-off-by: Ralf Lici <ralf@mandelbit.com>
> > ---
> > Changes since v5 https://lore.kernel.org/openvpn-devel/d530ecfc3719845075fbddd0cd7c34752bc2ce16.1783336121.git.ralf@mandelbit.com/
> > - Update the module-exit ordering for the queue_rcu_work-based key-slot
> >    release: flush ordinary ovpn work before rcu_barrier, then destroy the
> >    workqueue after RCU callbacks have queued their cleanup work.
> > 
> > Changes since v4 https://lore.kernel.org/openvpn-devel/6edfcc51e0855bfd34286b86d4e7f26bb3bcd3f7.1783099626.git.ralf@mandelbit.com/
> > - Rebase on the pending keepalive and TCP deferred-work refcount fixes,
> >    preserving their hold-before-queue and queue-failure put handling when
> >    converting schedule_work to queue_work.
> > 
> > Changes since v3 https://lore.kernel.org/openvpn-devel/49f38f89340e18ed30543d3990a7a7e20595b6af.1783080055.git.ralf@mandelbit.com/
> > - Replace the RCU-deferred peer netdev reference release with a
> >    module-owned workqueue that drains all ovpn work callbacks before
> >    module text can be freed.
> > 
> >   drivers/net/ovpn/main.c     | 19 ++++++++++++++++++-
> >   drivers/net/ovpn/ovpnpriv.h |  4 ++++
> >   drivers/net/ovpn/peer.c     |  8 ++++----
> >   drivers/net/ovpn/tcp.c      |  9 ++++-----
> >   4 files changed, 30 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
> > index 5093a3b5aba6..80d10f9ef7d8 100644
> > --- a/drivers/net/ovpn/main.c
> > +++ b/drivers/net/ovpn/main.c
> > @@ -12,6 +12,7 @@
> >   #include <linux/module.h>
> >   #include <linux/netdevice.h>
> >   #include <linux/inetdevice.h>
> > +#include <linux/workqueue.h>
> >   #include <net/gro_cells.h>
> >   #include <net/ip.h>
> >   #include <net/rtnetlink.h>
> > @@ -26,6 +27,9 @@
> >   #include "tcp.h"
> >   #include "udp.h"
> >   
> > +/* module-owned workqueue on which all ovpn-specific work is queued */
> > +struct workqueue_struct *ovpn_wq;
> > +
> >   static void ovpn_priv_free(struct net_device *net)
> >   {
> >   	struct ovpn_priv *ovpn = netdev_priv(net);
> > @@ -237,10 +241,16 @@ static int __init ovpn_init(void)
> >   
> >   	ovpn_tcp_init();
> >   
> > +	ovpn_wq = alloc_workqueue("ovpn", 0, 0);
>
> This is triggering:
>
>
> workqueue: ovpn is using neither WQ_PERCPU or WQ_UNBOUND. Setting WQ_PERCPU.
> WARNING: kernel/workqueue.c:5852 at __alloc_workqueue+0x1cf5/0x2060, 
> CPU#0: swapper/0/1
>
>
> Can you fix it and resend this patch only please?
>

Sure. Apparently the development tree used for this series predated
commit 21c05ca88a54 ("workqueue: Add warnings and ensure one among
WQ_PERCPU or WQ_UNBOUND is present"), which now warns when the selection
is implicit.

I'll resend this patch, explicitly selecting WQ_PERCPU to preserve the
existing workqueue behavior.

>
> Regards,
>

Regards,
diff mbox series

Patch

diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
index 5093a3b5aba6..80d10f9ef7d8 100644
--- a/drivers/net/ovpn/main.c
+++ b/drivers/net/ovpn/main.c
@@ -12,6 +12,7 @@ 
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/inetdevice.h>
+#include <linux/workqueue.h>
 #include <net/gro_cells.h>
 #include <net/ip.h>
 #include <net/rtnetlink.h>
@@ -26,6 +27,9 @@ 
 #include "tcp.h"
 #include "udp.h"
 
+/* module-owned workqueue on which all ovpn-specific work is queued */
+struct workqueue_struct *ovpn_wq;
+
 static void ovpn_priv_free(struct net_device *net)
 {
 	struct ovpn_priv *ovpn = netdev_priv(net);
@@ -237,10 +241,16 @@  static int __init ovpn_init(void)
 
 	ovpn_tcp_init();
 
+	ovpn_wq = alloc_workqueue("ovpn", 0, 0);
+	if (!ovpn_wq) {
+		pr_err("ovpn: cannot allocate workqueue\n");
+		return -ENOMEM;
+	}
+
 	err = rtnl_link_register(&ovpn_link_ops);
 	if (err) {
 		pr_err("ovpn: can't register rtnl link ops: %d\n", err);
-		return err;
+		goto destroy_wq;
 	}
 
 	err = ovpn_nl_register();
@@ -253,6 +263,9 @@  static int __init ovpn_init(void)
 
 unreg_rtnl:
 	rtnl_link_unregister(&ovpn_link_ops);
+destroy_wq:
+	destroy_workqueue(ovpn_wq);
+	ovpn_wq = NULL;
 	return err;
 }
 
@@ -261,7 +274,11 @@  static __exit void ovpn_cleanup(void)
 	ovpn_nl_unregister();
 	rtnl_link_unregister(&ovpn_link_ops);
 
+	flush_workqueue(ovpn_wq);
 	rcu_barrier();
+
+	destroy_workqueue(ovpn_wq);
+	ovpn_wq = NULL;
 }
 
 module_init(ovpn_init);
diff --git a/drivers/net/ovpn/ovpnpriv.h b/drivers/net/ovpn/ovpnpriv.h
index 5898f6adada7..84499140e4bd 100644
--- a/drivers/net/ovpn/ovpnpriv.h
+++ b/drivers/net/ovpn/ovpnpriv.h
@@ -15,6 +15,10 @@ 
 #include <uapi/linux/if_link.h>
 #include <uapi/linux/ovpn.h>
 
+struct workqueue_struct;
+
+extern struct workqueue_struct *ovpn_wq;
+
 /**
  * struct ovpn_peer_collection - container of peers for MultiPeer mode
  * @by_id: table of peers index by ID
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index a21d02ac715e..a80e85a62af5 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -62,7 +62,7 @@  void ovpn_peer_keepalive_set(struct ovpn_peer *peer, u32 interval, u32 timeout)
 	/* now that interval and timeout have been changed, kick
 	 * off the worker so that the next delay can be recomputed
 	 */
-	mod_delayed_work(system_percpu_wq, &peer->ovpn->keepalive_work, 0);
+	mod_delayed_work(ovpn_wq, &peer->ovpn->keepalive_work, 0);
 }
 
 /**
@@ -1287,7 +1287,7 @@  static time64_t ovpn_peer_keepalive_work_single(struct ovpn_peer *peer,
 			   peer->id);
 		if (WARN_ON(!ovpn_peer_hold(peer)))
 			return 0;
-		if (!schedule_work(&peer->keepalive_work))
+		if (!queue_work(ovpn_wq, &peer->keepalive_work))
 			ovpn_peer_put(peer);
 	}
 
@@ -1379,8 +1379,8 @@  void ovpn_peer_keepalive_work(struct work_struct *work)
 		netdev_dbg(ovpn->dev,
 			   "scheduling keepalive work: now=%llu next_run=%llu delta=%llu\n",
 			   next_run, now, next_run - now);
-		schedule_delayed_work(&ovpn->keepalive_work,
-				      (next_run - now) * HZ);
+		queue_delayed_work(ovpn_wq, &ovpn->keepalive_work,
+				   (next_run - now) * HZ);
 	}
 	unlock_ovpn(ovpn, &release_list);
 }
diff --git a/drivers/net/ovpn/tcp.c b/drivers/net/ovpn/tcp.c
index 0af14055c39a..8fe8a8e750a4 100644
--- a/drivers/net/ovpn/tcp.c
+++ b/drivers/net/ovpn/tcp.c
@@ -151,7 +151,7 @@  static void ovpn_tcp_rcv(struct strparser *strp, struct sk_buff *skb)
 	/* take reference for deferred peer deletion. should never fail */
 	if (WARN_ON(!ovpn_peer_hold(peer)))
 		goto err_nopeer;
-	if (!schedule_work(&peer->tcp.defer_del_work))
+	if (!queue_work(ovpn_wq, &peer->tcp.defer_del_work))
 		ovpn_peer_put(peer);
 	ovpn_dev_dstats_rx_dropped(peer->ovpn->dev);
 err_nopeer:
@@ -284,13 +284,12 @@  static void ovpn_tcp_send_sock(struct ovpn_peer *peer, struct sock *sk)
 			 * stream therefore we abort the connection
 			 */
 			ovpn_peer_hold(peer);
-			if (!schedule_work(&peer->tcp.defer_del_work))
+			if (!queue_work(ovpn_wq, &peer->tcp.defer_del_work))
 				ovpn_peer_put(peer);
 
 			/* we bail out immediately and keep tx_in_progress set
 			 * to true. This way we prevent more TX attempts
-			 * which would lead to more invocations of
-			 * schedule_work()
+			 * which would lead to more invocations of queue_work()
 			 */
 			return;
 		}
@@ -487,7 +486,7 @@  static void ovpn_tcp_write_space(struct sock *sk)
 	rcu_read_lock();
 	sock = rcu_dereference_sk_user_data(sk);
 	if (likely(sock && sock->peer)) {
-		schedule_work(&sock->tcp_tx_work);
+		queue_work(ovpn_wq, &sock->tcp_tx_work);
 		sock->peer->tcp.sk_cb.sk_write_space(sk);
 	}
 	rcu_read_unlock();