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

[Sven Eckelmann <sven@narfation.org>]

[-- 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 --]