Re: [PATCH net 2/5] ipv6: mcast: use copy-on-write RCU updates in ip6_mc_source()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ipv6: mcast: use copy-on-write RCU updates in ip6_mc_source()

This converts ip6_mc_source() from in-place mutation of pmc->sflist to
copy-on-write: a new ip6_sf_socklist is fully populated before being
published with rcu_assign_pointer(), and the old one is freed with
kfree_rcu(), matching ip6_mc_msfilter(). The unused IP6_SFBLOCK macro is
also removed.

> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index ec7fac511c8d5..40e996f36c373 100644
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c

[ ... ]

> @@ -424,58 +422,75 @@ int ip6_mc_source(int add, int omode, struct sock *sk,
>  			goto done;
>  		}
>  
> +		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) {
> +				err = -ENOBUFS;
> +				goto done;
> +			}

[Severity: Medium]
Should removing a source from the filter be able to fail with -ENOBUFS
now?

Before this change the !add path only shifted psl->sl_addr[] down and
decremented psl->sl_count, so MCAST_LEAVE_SOURCE_GROUP and
MCAST_UNBLOCK_SOURCE could not fail.  Now a shrinking update needs a
fresh sock_kmalloc(), and the old buffer's charge is released only after
that allocation succeeded:

	newpsl = sock_kmalloc(sk, struct_size(newpsl, sl_addr,
					      psl->sl_count - 1), GFP_KERNEL);
	...
	atomic_sub(struct_size(psl, sl_addr, psl->sl_max),
		   &sk->sk_omem_alloc);
	rcu_assign_pointer(pmc->sflist, newpsl);

so the peak sk_omem_alloc charge for the operation is old + (old - 16).

sock_kmalloc() in net/core/sock.c rejects on the sum:

	if ((unsigned int)size <= optmem_max &&
	    atomic_read(&sk->sk_omem_alloc) + size < optmem_max) {

With the default sysctl_mld_max_msf of 64 a full filter is 24 + 16*64 =
1048 bytes, so roughly 19 fully populated groups reach the default
optmem_max of 20480 and any further per-source removal returns -ENOBUFS
instead of releasing memory.

mld_max_msf is also registered with plain proc_dointvec and no extra2
clamp in net/ipv6/sysctl_net_ipv6.c, so a single filter installed via
ip6_mc_msfilter() can exceed half of optmem_max, in which case
per-source removal stays impossible for that filter until userspace
clears the whole list (MCAST_MSFILTER with gf_numsrc == 0 takes the
newpsl == NULL path) or leaves the group.

The IPv4 counterpart ip_mc_source() in net/ipv4/igmp.c still does the
in-place shift:

	for (j = i+1; j < psl->sl_count; j++)
		psl->sl_addr[j-1] = psl->sl_addr[j];
	psl->sl_count--;
	err = 0;

Is the resulting difference in error contracts between the two families
for the same socket option intended?

> +			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 */
>  		ip6_mc_del_src(idev, group, omode, 1, source, 1);
>  
> -		for (j = i+1; j < psl->sl_count; j++)
> -			psl->sl_addr[j-1] = psl->sl_addr[j];
> -		psl->sl_count--;
> +		atomic_sub(struct_size(psl, sl_addr, psl->sl_max),
> +			   &sk->sk_omem_alloc);
> +		rcu_assign_pointer(pmc->sflist, newpsl);
> +		kfree_rcu(psl, rcu);
>  		err = 0;
>  		goto done;
>  	}
>  	/* else, add a new source to the filter */

[ ... ]

> +	newpsl->sl_max = i;
> +	newpsl->sl_count = i;
> +	if (psl) {
> +		for (j = 0; j < psl->sl_count; j++)
> +			newpsl->sl_addr[j] = psl->sl_addr[j];
>  	}
> -	for (j = psl->sl_count-1; j >= i; j--)
> -		psl->sl_addr[j+1] = psl->sl_addr[j];
> -	psl->sl_addr[i] = *source;
> -	psl->sl_count++;
> +	newpsl->sl_addr[i - 1] = *source;
> +
> +	err = ip6_mc_add_src(idev, group, omode, 1, source, 1);
> +	if (err) {
> +		sock_kfree_s(sk, newpsl, struct_size(newpsl, sl_addr,
> +						     newpsl->sl_max));
> +		goto done;
> +	}

[Severity: Medium]
Is the new error propagation from ip6_mc_add_src() a deliberate part of
this conversion?

The old code was:

	err = 0;
	/* update the interface list */
	ip6_mc_add_src(idev, group, omode, 1, source, 1);

so MCAST_BLOCK_SOURCE and MCAST_JOIN_SOURCE_GROUP always returned 0 and
always installed the socket filter.  Two failures are now returned to
userspace with pmc->sflist left unchanged:

- -ENOBUFS from the interface-level allocation in ip6_mc_add1_src():

	psf = kzalloc_obj(*psf);
	if (!psf)
		return -ENOBUFS;

- -ESRCH when ip6_mc_add_src() does not find the group on idev.  That
  is reachable with gsr_interface == 0, since the pmc lookup skips the
  ifindex comparison:

	if (pgsr->gsr_interface && pmc->ifindex != pgsr->gsr_interface)
		continue;

  while ip6_mc_find_idev()/ip6_mc_find_dev() resolves the device from
  rt6_lookup(), which need not be the device the socket joined on.

The delete branch above still ignores the identical -ESRCH from
ip6_mc_del_src(), so the two branches disagree.  The commit message
describes only the RCU publication fix and does not mention this change
in setsockopt() return values, which matters for the stable backport
implied by the Fixes: tag.  Would it be worth either splitting this out
or documenting it in the changelog?

> +
> +	if (psl)
> +		atomic_sub(struct_size(psl, sl_addr, psl->sl_max),
> +			   &sk->sk_omem_alloc);
> +	rcu_assign_pointer(pmc->sflist, newpsl);
> +	kfree_rcu(psl, rcu);
>  	err = 0;
> -	/* update the interface list */
> -	ip6_mc_add_src(idev, group, omode, 1, source, 1);
>  done:
>  	mutex_unlock(&idev->mc_lock);
>  	in6_dev_put(idev);

[Severity: Medium]
This isn't a bug introduced by this patch, but the companion field
pmc->sfmode is still a plain store while the list is now published
atomically.  inet6_mc_check() consumes both fields together under
rcu_read_lock() only:

	psl = rcu_dereference(mc->sflist);
	if (!psl) {
		rv = mc->sfmode == MCAST_EXCLUDE;
	} else {
		...
		if (mc->sfmode == MCAST_INCLUDE && i >= psl->sl_count)
			rv = false;
		if (mc->sfmode == MCAST_EXCLUDE && i < psl->sl_count)
			rv = false;
	}

ip6_mc_msfilter() publishes the list first and updates the mode after:

	rcu_assign_pointer(pmc->sflist, newpsl);
	kfree_rcu(psl, rcu);
	pmc->sfmode = gsf->gf_fmode;

and ip6_mc_source() does the same kind of plain store with
pmc->sfmode = omode.

Can a softirq receive path then pair the newly published list with the
stale mode?  For an EXCLUDE{A} to INCLUDE{A} transition a reader that
sees the new list but the old mode takes i < psl->sl_count and sets
rv = false, dropping a datagram the socket now includes; the opposite
interleaving accepts one that should be blocked.  Both sides are also
unannotated, so KCSAN can report the sfmode access as a data race.

Would publishing the mode and the source list as one RCU-protected
object be a reasonable follow-up?