Re: [PATCH v2 net] igmp: convert struct ip_sf_list to RCU

[Ido Schimmel <idosch@nvidia.com>]

On Thu, Aug 27, 2026 at 04:06:56PM +0000, Eric Dumazet wrote:
> Commit 23d2b94043ca ("igmp: Add ip_mc_list lock in ip_check_mc_rcu")
> added spin_lock_bh(&im->lock) to ip_check_mc_rcu() to prevent a
> use-after-free while iterating im->sources during concurrent deletions.
> 
> However, ip_check_mc_rcu() is called from RCU read-side critical
> sections in packet receive and route lookup fast paths (e.g.
> __mkroute_output(), ip_route_input_rcu(), and __udp4_lib_rcv()).
> 
> When igmpv3_send_cr() or igmpv3_send_report() holds &pmc->lock and
> calls add_grec() -> igmpv3_newpack() -> ip_route_output_ports(),
> an XFRM policy matching a multicast destination triggers
> xfrm_tmpl_resolve_one() -> xfrm4_get_saddr() -> __mkroute_output() ->
> ip_check_mc_rcu(). This attempts to acquire &im->lock while &pmc->lock
> is already held on the same CPU, triggering a lockdep recursive locking
> warning / deadlock.
> 
> Fix this by converting IPv4 struct ip_sf_list to RCU, mirroring the
> IPv6 implementation in net/ipv6/mcast.c:
> 
> 1. Add struct rcu_head to struct ip_sf_list and annotate sf_next,
>    sources, and tomb as __rcu pointers.
> 2. Use rcu_assign_pointer() and kfree_rcu() for list updates and
>    deletions.
> 3. Remove spin_lock_bh(&im->lock) from ip_check_mc_rcu() and traverse
>    im->sources locklessly with for_each_psf_rcu(), reading and writing
>    counter fields with READ_ONCE() and WRITE_ONCE().
> 
> Note: RCU conversion of /proc/net/mcfilter will be done in a
> separate patch.
> 
> Fixes: 23d2b94043ca ("igmp: Add ip_mc_list lock in ip_check_mc_rcu")
> Reported-by: syzbot+3d99fb01bcd740f2fc1e@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=3d99fb01bcd740f2fc1e
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

[...]

> @@ -1228,11 +1254,12 @@ static void igmpv3_add_delrec(struct in_device *in_dev, struct ip_mc_list *im,
>  	if (pmc->sfmode == MCAST_INCLUDE) {
>  		struct ip_sf_list *psf;
>  
> +		for_each_psf_mclock(im, psf)
> +			psf->sf_crcount = pmc->crcount;
>  		pmc->tomb = im->tomb;
>  		pmc->sources = im->sources;

In case you need another version:

RCU_INIT_POINTER(pmc->tomb, pmc_dereference(im->tomb, im));
RCU_INIT_POINTER(pmc->sources,
		 pmc_dereference(im->sources, im));

> -		im->tomb = im->sources = NULL;
> -		for (psf = pmc->sources; psf; psf = psf->sf_next)
> -			psf->sf_crcount = pmc->crcount;
> +		RCU_INIT_POINTER(im->tomb, NULL);
> +		RCU_INIT_POINTER(im->sources, NULL);
>  	}
>  	spin_unlock_bh(&im->lock);

[...]

> @@ -2011,19 +2047,28 @@ static int ip_mc_del1_src(struct ip_mc_list *pmc, int sfmode,
>  
>  		/* no more filters for this source */
>  		if (psf_prev)
> -			psf_prev->sf_next = psf->sf_next;
> +			rcu_assign_pointer(psf_prev->sf_next,
> +					   pmc_dereference(psf->sf_next, pmc));
>  		else
> -			pmc->sources = psf->sf_next;
> +			rcu_assign_pointer(pmc->sources,
> +					   pmc_dereference(psf->sf_next, pmc));
>  #ifdef CONFIG_IP_MULTICAST
>  		if (psf->sf_oldin &&
>  		    !IGMP_V1_SEEN(in_dev) && !IGMP_V2_SEEN(in_dev)) {
> -			psf->sf_crcount = in_dev->mr_qrv ?: READ_ONCE(net->ipv4.sysctl_igmp_qrv);
> -			psf->sf_next = pmc->tomb;
> -			pmc->tomb = psf;
> -			rv = 1;
> -		} else
> +			struct ip_sf_list *dpsf = kmalloc_obj(*dpsf, GFP_ATOMIC);
> +
> +			if (dpsf) {
> +				*dpsf = *psf;
> +				dpsf->sf_crcount = in_dev->mr_qrv ?:
> +					READ_ONCE(net->ipv4.sysctl_igmp_qrv);
> +				rcu_assign_pointer(dpsf->sf_next,
> +						   pmc_dereference(pmc->tomb, pmc));
> +				rcu_assign_pointer(pmc->tomb, dpsf);
> +				rv = 1;
> +			}
> +		}
>  #endif
> -			kfree(psf);
> +		kfree_rcu(psf, rcu);
>  	}
>  	return rv;
>  }

The allocation can fail, but I don't see a better fix. I assume it's
done in that way for the reason mentioned in [1].

[1] https://lore.kernel.org/netdev/20260828084531.1826790-2-edumazet@google.com/