[PATCH net] net: hsr: wait for pending node-free RCU callbacks on module exit

[PATCH net] net: hsr: wait for pending node-free RCU callbacks on module exit

[Ali Ahmet Memis]

HSR/PRP node teardown and the prune paths queue node frees with
call_rcu(&node->rcu_head, hsr_free_node_rcu). hsr_free_node_rcu() lives
in the hsr module text.

hsr_exit() tears down netlink, debugfs and the netdev notifier, but it
never calls rcu_barrier(). If a node-free callback is still pending when
the module is unloaded, RCU can invoke hsr_free_node_rcu() after the
module text has been freed, branching into freed memory.

Per Documentation/RCU/checklist.rst, a module that registers callbacks
with call_rcu() must call rcu_barrier() in its exit path before it is
unloaded; synchronize_rcu() is not sufficient because it only waits for
a grace period, not for the queued callbacks to actually run.

Add rcu_barrier() at the end of hsr_exit(), after netlink/link teardown
has stopped any new callbacks from being queued.

Fixes: 415e6367512b ("hsr: Implement more robust duplicate discard for PRP")
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 net/hsr/hsr_main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
index 33951d9bd..05d2e5750 100644
--- a/net/hsr/hsr_main.c
+++ b/net/hsr/hsr_main.c
@@ -179,6 +179,13 @@ static void __exit hsr_exit(void)
 	hsr_netlink_exit();
 	hsr_debugfs_remove_root();
 	unregister_netdevice_notifier(&hsr_nb);
+
+	/* Node teardown queues frees via call_rcu(hsr_free_node_rcu), whose
+	 * callback lives in this module. Wait for any still-pending callback
+	 * before the module text is freed. synchronize_rcu() is not enough:
+	 * it waits for a grace period, not for the callbacks to run.
+	 */
+	rcu_barrier();
 }
 
 module_init(hsr_init);
-- 
2.54.0

Re: [PATCH net] net: hsr: wait for pending node-free RCU callbacks on module exit

[Ali Ahmet Memis]

Please drop the Cc: stable from this one if you take it, and I would
rather the changelog was read as hardening than as a fix for something
observed. I overstated it.

Going back over the exit path: hsr_dellink() deletes the four timers with
timer_delete_sync() and then queues every call_rcu(hsr_free_node_rcu)
from hsr_del_self_node() and hsr_del_nodes(), all under rtnl. The
rtnl_unlock() at the end of rtnl_link_unregister() runs netdev_run_todo(),
which does

	/* Wait for rcu callbacks to finish before next phase */
	if (!list_empty(&list))
		rcu_barrier();

so the callbacks queued during teardown are already waited for before
hsr_netlink_exit() returns. netdev_wait_allrefs_any() calls rcu_barrier()
as well. I do not have a case where a callback survives that.

I still think the barrier belongs there. Documentation/RCU/checklist.rst
asks a module that registers call_rcu() callbacks to rcu_barrier() before
it is unloaded, and net/8021q/vlan.c, net/sctp/protocol.c and net/ipv6/sit.c
all do exactly this in their exit paths, vlan being the same shape as hsr,
an rtnl_link_ops module torn down through rtnl_link_unregister(). It costs
nothing at module unload and does not depend on netdev_run_todo() keeping
that barrier.

But that is defence in depth, not a bug I can point at, so it does not
meet the stable rules. Happy to resend with the changelog rewritten that
way, or to drop it entirely if you would rather not carry it.

--
Ali