[PATCH RFC batadv] batman-adv: mcast: fix use-after-free in orig_node RCU release

[PATCH RFC batadv] batman-adv: mcast: fix use-after-free in orig_node RCU release

[Sven Eckelmann]

batadv_mcast_purge_orig() removes entries from RCU-protected hlists but
does not wait for an RCU grace period before returning. Concurrent RCU
readers may still accesses references to those entries at the point of
removal. RCU-protected readers trying to operate on entries like
orig->mcast_want_all_ipv6_node will then access already freed memory.

Fix this by moving batadv_mcast_purge_orig() to batadv_orig_node_release(),
just before the call_rcu() invocation. This ensures RCU readers that were
active at purge time have drained before the orig_node memory is reclaimed.

Fixes: 1c090349e2f6 ("batman-adv: Add IPv4 link-local/IPv6-ll-all-nodes multicast support")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
See
https://sashiko.dev/#/patchset/05bdee6e85c3514822f98afa8fb75826b3928dd0.1778671969.git.ruijieli51%40gmail.com
---
 net/batman-adv/originator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c
index b3468cca..ad4921b6 100644
--- a/net/batman-adv/originator.c
+++ b/net/batman-adv/originator.c
@@ -835,8 +835,6 @@ static void batadv_orig_node_free_rcu(struct rcu_head *rcu)
 
 	orig_node = container_of(rcu, struct batadv_orig_node, rcu);
 
-	batadv_mcast_purge_orig(orig_node);
-
 	batadv_frag_purge_orig(orig_node, NULL);
 
 	kfree(orig_node->tt_buff);
@@ -887,6 +885,8 @@ void batadv_orig_node_release(struct kref *ref)
 	}
 	spin_unlock_bh(&orig_node->vlan_list_lock);
 
+	batadv_mcast_purge_orig(orig_node);
+
 	call_rcu(&orig_node->rcu, batadv_orig_node_free_rcu);
 }
 

---
base-commit: 5e1068c577818529e2f7a7f5ccb9fe4a440198c7
change-id: 20260514-mcast-rcu-list-free-8401ba2c6698

Best regards,
--  
Sven Eckelmann <sven@narfation.org>

Re: [PATCH RFC batadv] batman-adv: mcast: fix use-after-free in orig_node RCU release

[Linus Lüssing]

On Thu, May 14, 2026 at 07:41:38PM +0200, Sven Eckelmann wrote:
> batadv_mcast_purge_orig() removes entries from RCU-protected hlists but
> does not wait for an RCU grace period before returning. Concurrent RCU
> readers may still accesses references to those entries at the point of
> removal. RCU-protected readers trying to operate on entries like
> orig->mcast_want_all_ipv6_node will then access already freed memory.

This one I don't really get yet. The mcat_want_all_* lists/entries should
be spinlock protected (&bat_priv->mcast.want_lists_lock), not RCU
protected?

We don't use RCU for these lists in the first place because within
the list changes / spinlocks &bat_priv->mcast.num_want_all_*
atomic counters are increased/decreased. And these atomic counters
are then used in fast path. Not those lists.

Re: [PATCH RFC batadv] batman-adv: mcast: fix use-after-free in orig_node RCU release

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 2681 bytes --]

On Sunday, 17 May 2026 18:38:53 CEST Linus Lüssing wrote:
> On Thu, May 14, 2026 at 07:41:38PM +0200, Sven Eckelmann wrote:
> > batadv_mcast_purge_orig() removes entries from RCU-protected hlists but
> > does not wait for an RCU grace period before returning. Concurrent RCU
> > readers may still accesses references to those entries at the point of
> > removal. RCU-protected readers trying to operate on entries like
> > orig->mcast_want_all_ipv6_node will then access already freed memory.
> 
> This one I don't really get yet. The mcat_want_all_* lists/entries should
> be spinlock protected (&bat_priv->mcast.want_lists_lock), not RCU
> protected?
> 
> We don't use RCU for these lists in the first place because within
> the list changes / spinlocks &bat_priv->mcast.num_want_all_*
> atomic counters are increased/decreased. And these atomic counters
> are then used in fast path. Not those lists.
> 

Um? I can see RCU modification function here (which are correctly protected
by spinlocks):

static void batadv_mcast_want_ipv4_update(struct batadv_priv *bat_priv,
					  struct batadv_orig_node *orig,
					  u8 mcast_flags)
{
	struct hlist_node *node = &orig->mcast_want_all_ipv4_node;
	struct hlist_head *head = &bat_priv->mcast.want_all_ipv4_list;

	lockdep_assert_held(&orig->mcast_handler_lock);

	/* switched from flag unset to set */
	if (mcast_flags & BATADV_MCAST_WANT_ALL_IPV4 &&
	    !(orig->mcast_flags & BATADV_MCAST_WANT_ALL_IPV4)) {
[...]
		hlist_add_head_rcu(node, head);
[...]
	/* switched from flag set to unset */
	} else if (!(mcast_flags & BATADV_MCAST_WANT_ALL_IPV4) &&
		   orig->mcast_flags & BATADV_MCAST_WANT_ALL_IPV4) {
[...]
		hlist_del_init_rcu(node);
[...]
	}
}



But this looks super RCU-like (without locks):

static int
batadv_mcast_forw_want_all_ipv4(struct batadv_priv *bat_priv,
				struct sk_buff *skb, unsigned short vid)
{
	struct batadv_orig_node *orig_node;
	int ret = NET_XMIT_SUCCESS;
	struct sk_buff *newskb;

	rcu_read_lock();
	hlist_for_each_entry_rcu(orig_node,
				 &bat_priv->mcast.want_all_ipv4_list,
				 mcast_want_all_ipv4_node) {
[..]
	}
	rcu_read_unlock();
	return ret;
}



And when you do something like this, you can't try do run these functions in a 
free_rcu function. Because you are then missing the RCU grace period. The list 
can still be accessed in a parallel running RCU reader and the 
batadv_orig_node_free_rcu function might then already have freed the originator.
The reader then goes *KABUMM*.

Or am I missing something and the functions themelf need to be freed from RCU
references (or something else)?

Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RFC batadv] batman-adv: mcast: fix use-after-free in orig_node RCU release

[Linus Lüssing]

On Sun, May 17, 2026 at 06:54:09PM +0200, Sven Eckelmann wrote:
> But this looks super RCU-like (without locks):
> 
> static int
> batadv_mcast_forw_want_all_ipv4(struct batadv_priv *bat_priv,
> 				struct sk_buff *skb, unsigned short vid)
> {
> 	struct batadv_orig_node *orig_node;
> 	int ret = NET_XMIT_SUCCESS;
> 	struct sk_buff *newskb;
> 
> 	rcu_read_lock();
> 	hlist_for_each_entry_rcu(orig_node,
> 				 &bat_priv->mcast.want_all_ipv4_list,
> 				 mcast_want_all_ipv4_node) {
> [..]
> 	}
> 	rcu_read_unlock();
> 	return ret;
> }

Ah, no, you're absolutely right, I mixed it up!

Maybe this might make more sense as a fixes line then, though?

Fixes: 500ea14940f8 ("batman-adv: Add multicast-to-unicast support for multiple targets")


I think before that commit we only used these atomic counters in
fast path?

Re: [PATCH RFC batadv] batman-adv: mcast: fix use-after-free in orig_node RCU release

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

On Sunday, 17 May 2026 19:55:56 CEST Linus Lüssing wrote:
[...]
> Maybe this might make more sense as a fixes line then, though?
> 
> Fixes: 500ea14940f8 ("batman-adv: Add multicast-to-unicast support for multiple targets")
> 
> 
> I think before that commit we only used these atomic counters in
> fast path?

I don't really care about the fast path. I care about things which could be 
called in parallel and is only RCU protected. For example in 1c090349e2f6,
we have the reader:

static struct batadv_orig_node *
batadv_mcast_forw_unsnoop_node_get(struct batadv_priv *bat_priv)
{
	struct batadv_orig_node *orig_node;

	rcu_read_lock();
	hlist_for_each_entry_rcu(orig_node,
				 &bat_priv->mcast.want_all_unsnoopables_list,
				 mcast_want_all_unsnoopables_node) {
		if (atomic_inc_not_zero(&orig_node->refcount))
			goto unlock;
	}

	orig_node = NULL;

unlock:
	rcu_read_unlock();
	return orig_node;
}


And here the cleanup code in 1c090349e2f6

static void batadv_mcast_want_unsnoop_update(struct batadv_priv *bat_priv,
					     struct batadv_orig_node *orig,
					     uint8_t mcast_flags)
{
[...]
		hlist_add_head_rcu(&orig->mcast_want_all_unsnoopables_node,
				   &bat_priv->mcast.want_all_unsnoopables_list);
[...]
		hlist_del_rcu(&orig->mcast_want_all_unsnoopables_node);
[...]
}

/**
 * batadv_mcast_purge_orig - reset originator global mcast state modifications
 * @orig: the originator which is going to get purged
 */
void batadv_mcast_purge_orig(struct batadv_orig_node *orig)
{
[...]
	batadv_mcast_want_unsnoop_update(bat_priv, orig, BATADV_NO_FLAGS);
}


Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RFC batadv] batman-adv: mcast: fix use-after-free in orig_node RCU release

[Linus Lüssing]

On Thu, May 14, 2026 at 07:41:38PM +0200, Sven Eckelmann wrote:
> batadv_mcast_purge_orig() removes entries from RCU-protected hlists but
> does not wait for an RCU grace period before returning. Concurrent RCU
> readers may still accesses references to those entries at the point of
> removal. RCU-protected readers trying to operate on entries like
> orig->mcast_want_all_ipv6_node will then access already freed memory.
> 
> Fix this by moving batadv_mcast_purge_orig() to batadv_orig_node_release(),
> just before the call_rcu() invocation. This ensures RCU readers that were
> active at purge time have drained before the orig_node memory is reclaimed.
> 
> Fixes: 1c090349e2f6 ("batman-adv: Add IPv4 link-local/IPv6-ll-all-nodes multicast support")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Makes sense to me now and does not seem to crash on my laptop with
Debian Sid in a simple batadv/veth setup.

Acked-by: Linus Lüssing <linus.luessing@c0d3.blue>