mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net v1] igmp: use copy-on-write RCU updates in ip_mc_source()
@ 2026-11-02  9:53 Binbin Deng
  2026-11-02  9:59 ` netdev-bot+sinfo
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Binbin Deng @ 2026-11-02  9:53 UTC (permalink / raw)
  To: dsahern, idosch, davem, edumazet, kuba, pabeni, horms
  Cc: netdev, linux-kernel, Binbin Deng

pmc->sflist is read locklessly under rcu_read_lock() by ip_mc_sf_allow()
during packet reception in the UDP and RAW multicast receive paths.

ip_mc_source() mutated psl->sl_addr and psl->sl_count in-place when adding
or removing a source filter.  Additionally, when expanding the filter
buffer, newpsl was published via rcu_assign_pointer() before the new source
was written into the array.

The setsockopt writer holds RTNL and the socket lock, but neither excludes
ip_mc_sf_allow(), which traverses the filter inside an RCU read-side
critical section.  During an expansion, the race looks like this:

CPU0                                     CPU1
ip_mc_source(add = true)                 ip_mc_sf_allow()
/* RTNL and socket lock held */
allocate newpsl
copy old sources into newpsl
newpsl->sl_count = old source count
rcu_assign_pointer(pmc->sflist, newpsl)
psl = newpsl
                                         rcu_read_lock()
                                         psl = rcu_dereference(pmc->sflist)
                                         read psl->sl_count
shift psl->sl_addr[] for insertion
psl->sl_addr[i] = source
                                         read psl->sl_addr[i]
psl->sl_count++
                                         apply source-filter decision
                                         rcu_read_unlock()

The in-place add and remove paths have the same problem after a reader has
obtained the old list: the writer shifts or appends source addresses and
updates sl_count while the reader iterates those fields.  This is a data
race and can expose a mixed old/new filter state, with duplicated or missed
source entries.  A multicast packet may consequently be accepted or dropped
contrary to the configured filter.

An ordinary UDP socket can trigger the race: one thread updates a source
membership while multicast traffic is received and filtered on another CPU.
No special hardware is required.

Fix this by allocating and fully populating the replacement filter before
publishing it with rcu_assign_pointer(), and by reclaiming the old filter
with kfree_rcu().  The socket source list is therefore never modified while
an RCU reader may still be traversing it.

Fixes: c85bb41e9318 ("igmp: fix ip_mc_sf_allow race [v5]")
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 net/ipv4/igmp.c | 107 +++++++++++++++++++++++++++++-------------------
 1 file changed, 65 insertions(+), 42 deletions(-)

diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index d56355aca797..f965091500da 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -2489,10 +2489,10 @@ int ip_mc_source(int add, int omode, struct sock *sk, struct
 	struct ip_mc_socklist *pmc;
 	struct in_device *in_dev = NULL;
 	struct inet_sock *inet = inet_sk(sk);
-	struct ip_sf_socklist *psl;
+	struct ip_sf_socklist *newpsl, *psl;
 	struct net *net = sock_net(sk);
 	int leavegroup = 0;
-	int i, j, rv;
+	int count, i, j, rv;
 
 	if (!ipv4_is_multicast(addr))
 		return -EINVAL;
@@ -2554,13 +2554,34 @@ int ip_mc_source(int add, int omode, struct sock *sk, struct
 			goto done;
 		}
 
+		atomic_sub(struct_size(psl, sl_addr, psl->sl_max),
+			&sk->sk_omem_alloc);
+
+		if (psl->sl_count == 1) {
+			newpsl = NULL;
+		} else {
+			newpsl = sock_kmalloc(sk, struct_size(newpsl, sl_addr,
+				psl->sl_count - 1), GFP_KERNEL);
+			if (!newpsl) {
+				atomic_add(struct_size(psl, sl_addr, psl->sl_max),
+					&sk->sk_omem_alloc);
+				err = -ENOBUFS;
+				goto done;
+			}
+			newpsl->sl_max = psl->sl_count - 1;
+			newpsl->sl_count = psl->sl_count - 1;
+			for (j = 0; j < i; j++)
+				newpsl->sl_addr[j] = psl->sl_addr[j];
+			for (j = i + 1; j < psl->sl_count; j++)
+				newpsl->sl_addr[j - 1] = psl->sl_addr[j];
+		}
+
 		/* update the interface filter */
 		ip_mc_del_src(in_dev, &mreqs->imr_multiaddr, omode, 1,
 			&mreqs->imr_sourceaddr, 1);
-
-		for (j = i+1; j < psl->sl_count; j++)
-			psl->sl_addr[j-1] = psl->sl_addr[j];
-		psl->sl_count--;
+
+		rcu_assign_pointer(pmc->sflist, newpsl);
+		kfree_rcu(psl, rcu);
 		err = 0;
 		goto done;
 	}
@@ -2570,49 +2591,51 @@ int ip_mc_source(int add, int omode, struct sock *sk, struct
 		err = -ENOBUFS;
 		goto done;
 	}
-	if (!psl || psl->sl_count == psl->sl_max) {
-		struct ip_sf_socklist *newpsl;
-		int count = IP_SFBLOCK;
 
-		if (psl)
-			count += psl->sl_max;
-		newpsl = sock_kmalloc(sk, struct_size(newpsl, sl_addr, count),
-				      GFP_KERNEL);
-		if (!newpsl) {
-			err = -ENOBUFS;
-			goto done;
-		}
-		newpsl->sl_max = count;
-		newpsl->sl_count = count - IP_SFBLOCK;
-		if (psl) {
-			for (i = 0; i < psl->sl_count; i++)
-				newpsl->sl_addr[i] = psl->sl_addr[i];
-			/* decrease mem now to avoid the memleak warning */
-			atomic_sub(struct_size(psl, sl_addr, psl->sl_max),
-				   &sk->sk_omem_alloc);
+	rv = 1;
+	if (psl) {
+		for (i = 0; i < psl->sl_count; i++) {
+			rv = memcmp(&psl->sl_addr[i], &mreqs->imr_sourceaddr,
+				sizeof(__be32));
+			if (rv == 0)
+				break;
 		}
-		rcu_assign_pointer(pmc->sflist, newpsl);
-		if (psl)
-			kfree_rcu(psl, rcu);
-		psl = newpsl;
-	}
-	rv = 1;	/* > 0 for insert logic below if sl_count is 0 */
-	for (i = 0; i < psl->sl_count; i++) {
-		rv = memcmp(&psl->sl_addr[i], &mreqs->imr_sourceaddr,
-			sizeof(__be32));
-		if (rv == 0)
-			break;
-	}
+
 	if (rv == 0)		/* address already there is an error */
 		goto done;
-	for (j = psl->sl_count-1; j >= i; j--)
-		psl->sl_addr[j+1] = psl->sl_addr[j];
-	psl->sl_addr[i] = mreqs->imr_sourceaddr;
-	psl->sl_count++;
-	err = 0;
+
+	i = psl ? psl->sl_count + 1 : 1;
+	count = IP_SFBLOCK;
+	if (psl)
+		count += psl->sl_max;
+	newpsl = sock_kmalloc(sk, struct_size(newpsl, sl_addr, count),
+		GFP_KERNEL);
+	if (!newpsl) {
+		err = -ENOBUFS;
+		goto done;
+	}
+	newpsl->sl_max = count;
+	newpsl->sl_count = i;
+	if (psl) {
+		for (j = 0; j < psl->sl_count; j++)
+			newpsl->sl_addr[j] = psl->sl_addr[j];
+	}
+	newpsl->sl_addr[i - 1] = mreqs->imr_sourceaddr;
+
 	/* update the interface list */
 	ip_mc_add_src(in_dev, &mreqs->imr_multiaddr, omode, 1,
 		&mreqs->imr_sourceaddr, 1);
+
+	if (psl) {
+		/* decrease mem now to avoid the memleak warning */
+		atomic_sub(struct_size(psl, sl_addr, psl->sl_max),
+			&sk->sk_omem_alloc);
+	}
+	rcu_assign_pointer(pmc->sflist, newpsl);
+	if (psl)
+		kfree_rcu(psl, rcu);
+	err = 0;
+
 done:
 	if (leavegroup)
 		err = ip_mc_leave_group(sk, &imr);
-- 
2.43.0


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-11-02 18:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-02  9:53 [PATCH net v1] igmp: use copy-on-write RCU updates in ip_mc_source() Binbin Deng
2026-11-02  9:59 ` netdev-bot+sinfo
2026-11-02 15:59 ` kernel test robot
2026-11-02 17:49 ` kernel test robot
2026-11-02 18:10 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®