Re: [net,v1] net/ipv6: mcast: fix circular locking dependency in __ipv6_dev_mc_inc()

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net/ipv6: mcast: fix circular locking dependency in __ipv6_dev_mc_inc()

This commit fixes a circular locking dependency deadlock by moving the
GFP_KERNEL allocation outside the mc_lock critical section. The fix
splits mca_alloc() into allocation and initialization phases and also
moves inet6_ifmcaddr_notify() outside the lock.

Link: https://lore.kernel.org/all/69b7dc76.050a0220.248e02.0113.GAE@xxxxxxxxxx/T/

> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c

[ ... ]

> @@ -953,25 +957,23 @@ static int __ipv6_dev_mc_inc(struct net_device *dev,
>  			mc->mca_users++;
>  			ip6_mc_add_src(idev, &mc->mca_addr, mode, 0, NULL, 0);
>  			mutex_unlock(&idev->mc_lock);
> +			kfree(mc_alloced);
>  			in6_dev_put(idev);
>  			return 0;
>  		}
>  	}
>
> -	mc = mca_alloc(idev, addr, mode);
> -	if (!mc) {
> -		mutex_unlock(&idev->mc_lock);
> -		in6_dev_put(idev);
> -		return -ENOMEM;
> -	}
> +	mca_init(idev, addr, mode, mc_alloced);
> +	mc = mc_alloced;
>
>  	rcu_assign_pointer(mc->next, idev->mc_list);
>  	rcu_assign_pointer(idev->mc_list, mc);
>
>  	mld_del_delrec(idev, mc);
>  	igmp6_group_added(mc);
> +	mutex_unlock(&idev->mc_lock);
> +
>  	inet6_ifmcaddr_notify(dev, mc, RTM_NEWMULTICAST);
> -	mutex_unlock(&idev->mc_lock);
                ^^^^

Can this lead to a use-after-free on mc?

After mutex_unlock(&idev->mc_lock), the mc object has been published to
idev->mc_list with mca_refcnt=1. At this point, a concurrent
ipv6_mc_destroy_dev() could acquire mc_lock, iterate mc_list, and call
ma_put() on mc:

net/ipv6/mcast.c:ipv6_mc_destroy_dev() {
    mutex_lock(&idev->mc_lock);
    while ((mc = rcu_dereference_protected(idev->mc_list, ...)) != NULL) {
        ...
        ma_put(mc);
    }
    mutex_unlock(&idev->mc_lock);
}

Since mca_refcnt starts at 1 (set by mca_init), ma_put() would decrement
it to 0 and call kfree_rcu(mc, rcu):

net/ipv6/mcast.c:ma_put() {
    if (refcount_dec_and_test(&mc->mca_refcnt)) {
        in6_dev_put(mc->idev);
        kfree_rcu(mc, rcu);
    }
}

Then inet6_ifmcaddr_notify() calls nlmsg_new(..., GFP_KERNEL) which can
sleep and create an RCU quiescent state, allowing the kfree_rcu callback
to execute and free mc before inet6_fill_ifmcaddr() accesses mc->idev,
mc->idev->dev->ifindex, and mc->mca_addr.

Should the fix take an additional refcount on mc (refcount_inc(&mc->mca_refcnt))
before releasing mc_lock, then drop it with ma_put(mc) after
inet6_ifmcaddr_notify() returns?

>
>  	return 0;
>  }