Re: [PATCH net] ipv4: igmp: report interface group allocation failure on join

[Hangbin Liu <hangbin.liu@linux.dev>]

Hi Tianyi,
On Thu, Oct 22, 2026 at 02:23:59AM +0900, Tianyi Chen wrote:
> setsockopt(IP_ADD_MEMBERSHIP) can return 0 while the group was never
> joined on the interface.  __ip_mc_join_group() publishes the socket-side
> ip_mc_socklist in inet->mc_list before calling ____ip_mc_inc_group().
> That helper returns void, so when its kzalloc_obj() fails it returns
> without creating the interface-side ip_mc_list and the syscall still
> reports success.  The socket then passes IP_MSFILTER but cannot receive
> the group's traffic: no IGMP report is generated, the group is absent
> from /proc/net/igmp, and the receive path drops the datagrams.  Closing
> the socket also decrements the interface group for every published
> ip_mc_socklist, so it can remove a group that another socket joined.
> 
> Make the helper return -ENOBUFS on allocation failure, call it before
> publishing iml, and free iml on failure.  The object is charged with
> sock_kmalloc(), so sock_kfree_s() with the same size keeps
> sk_omem_alloc balanced; it was never published and has no source list
> yet, so no RCU free is needed.  Successful joins still publish the fully
> initialized socket-side record after the interface group exists.  RTNL
> serializes the writers, not the RCU readers; this ordering prevents a
> published socket membership without a device group.
> 
> The legacy void wrapper keeps its best-effort behaviour: interface-up,
> init, bridge and tunnel callers cannot report an allocation failure.  A
> query does not recreate a missing group; only a later successful
> inc_group call can.  The silent failure predates the 2019 helper split
> (9fb20801dab4); there is no recent regression to tag with Fixes.

I think the fix could be Linux-2.6.12-rc2.

> 
> Reproduced with fail-nth on v6.19 and v7.3-rc2.  N=2 fails the helper's

Is there any details about the reproducer?

> kzalloc_obj(): the original returns 0 with no interface group and 0/4
> multicast datagrams delivered; this change returns -ENOBUFS, leaves no
> socket or interface state, and a later join delivers 2/2.  On the
> original, closing the half-joined socket also removes a second socket's
> group; the patched kernel keeps it.  Normal joins, the existing-group
> users++ path and source-specific joins (allowed 2/2, filtered 0/2) are
> unchanged.
> 
> Assisted-by: LLM
> Signed-off-by: Tianyi Chen <hi@tychen.cc>
> ---
>  net/ipv4/igmp.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> index d56355aca797..81a100330630 100644
> --- a/net/ipv4/igmp.c
> +++ b/net/ipv4/igmp.c
> @@ -1566,9 +1566,15 @@ static void inet_ifmcaddr_notify(struct net_device *dev,
>  
>  /*
>   *	A socket has joined a multicast group on device dev.
> + *
> + *	Returns 0 if the interface now has a reference to the group, or
> + *	-ENOBUFS if the new group could not be allocated.  Callers that can
> + *	report an error to userspace must check it: a socket that reports a
> + *	successful join while the interface has no group cannot receive the
> + *	group's traffic.
>   */
> -static void ____ip_mc_inc_group(struct in_device *in_dev, __be32 addr,
> -				unsigned int mode, gfp_t gfp)
> +static int ____ip_mc_inc_group(struct in_device *in_dev, __be32 addr,
> +			       unsigned int mode, gfp_t gfp)
>  {
>  	struct ip_mc_list __rcu **mc_hash;
>  	struct ip_mc_list *im;
> @@ -1595,12 +1601,12 @@ static void ____ip_mc_inc_group(struct in_device *in_dev, __be32 addr,
>  	if  (im) {
>  		WRITE_ONCE(im->users, im->users + 1);
>  		ip_mc_add_src(in_dev, &addr, mode, 0, NULL, 0);
> -		goto out;
> +		return 0;
>  	}
>  
>  	im = kzalloc_obj(*im, gfp);
>  	if (!im)
> -		goto out;
> +		return -ENOBUFS;
>  
>  	WRITE_ONCE(im->users, 1);
>  	im->interface = in_dev;
> @@ -1630,13 +1636,18 @@ static void ____ip_mc_inc_group(struct in_device *in_dev, __be32 addr,
>  	inet_ifmcaddr_notify(in_dev->dev, im, RTM_NEWMULTICAST);
>  	if (!in_dev->dead)
>  		ip_rt_multicast_event(in_dev);
> -out:
> -	return;
> +	return 0;
>  }
>  
>  void __ip_mc_inc_group(struct in_device *in_dev, __be32 addr, gfp_t gfp)
>  {
> -	____ip_mc_inc_group(in_dev, addr, MCAST_EXCLUDE, gfp);
> +	/*
> +	 * Legacy void interface: interface-up, init, bridge and tunnel
> +	 * callers cannot report an allocation failure.  Keep the best-effort
> +	 * behaviour.  A query does not recreate a missing group; only a later
> +	 * successful inc_group call can.
> +	 */
> +	(void)____ip_mc_inc_group(in_dev, addr, MCAST_EXCLUDE, gfp);
>  }
>  EXPORT_SYMBOL(__ip_mc_inc_group);
>  
> @@ -2386,11 +2397,19 @@ static int __ip_mc_join_group(struct sock *sk, struct ip_mreqn *imr,
>  		goto done;
>  
>  	memcpy(&iml->multi, imr, sizeof(*imr));
> -	iml->next_rcu = inet->mc_list;
>  	iml->sflist = NULL;
>  	iml->sfmode = mode;
> +	/*
> +	 * Make the interface group exist before publishing the socket's
> +	 * membership: success must mean the group can actually receive.
> +	 */
> +	err = ____ip_mc_inc_group(in_dev, addr, mode, GFP_KERNEL);
> +	if (err) {
> +		sock_kfree_s(sk, iml, sizeof(*iml));
> +		goto done;
> +	}
> +	iml->next_rcu = inet->mc_list;
>  	rcu_assign_pointer(inet->mc_list, iml);
> -	____ip_mc_inc_group(in_dev, addr, mode, GFP_KERNEL);
>  	err = 0;
>  done:
>  	return err;
> 
> base-commit: df2908090cda368b01ff43709f51890076c56157
> -- 
> 2.56.0
> 

BTW, I feel there are too many comments generated by the LLM.
I wonder if others share the same feeling.

Thanks
Hangbin