[PATCH net v2] net: mld: fix reference count leak in mld_{query | report}_work()

[PATCH net v2] net: mld: fix reference count leak in mld_{query | report}_work()

[Taehee Yoo]

mld_{query | report}_work() processes queued events.
If there are too many events in the queue, it re-queue a work.
And then, it returns without in6_dev_put().
But if queuing is failed, it should call in6_dev_put(), but it doesn't.
So, a reference count leak would occur.

THREAD0				THREAD1
mld_report_work()
				spin_lock_bh()
				if (!mod_delayed_work())
					in6_dev_hold();
				spin_unlock_bh()
	spin_lock_bh()
	schedule_delayed_work()
	spin_unlock_bh()

Script to reproduce(by Hangbin Liu):
   ip netns add ns1
   ip netns add ns2
   ip netns exec ns1 sysctl -w net.ipv6.conf.all.force_mld_version=1
   ip netns exec ns2 sysctl -w net.ipv6.conf.all.force_mld_version=1

   ip -n ns1 link add veth0 type veth peer name veth0 netns ns2
   ip -n ns1 link set veth0 up
   ip -n ns2 link set veth0 up

   for i in `seq 50`; do
           for j in `seq 100`; do
                   ip -n ns1 addr add 2021:${i}::${j}/64 dev veth0
                   ip -n ns2 addr add 2022:${i}::${j}/64 dev veth0
           done
   done
   modprobe -r veth
   ip -a netns del

splat looks like:
 unregister_netdevice: waiting for veth0 to become free. Usage count = 2
 leaked reference.
  ipv6_add_dev+0x324/0xec0
  addrconf_notify+0x481/0xd10
  raw_notifier_call_chain+0xe3/0x120
  call_netdevice_notifiers+0x106/0x160
  register_netdevice+0x114c/0x16b0
  veth_newlink+0x48b/0xa50 [veth]
  rtnl_newlink+0x11a2/0x1a40
  rtnetlink_rcv_msg+0x63f/0xc00
  netlink_rcv_skb+0x1df/0x3e0
  netlink_unicast+0x5de/0x850
  netlink_sendmsg+0x6c9/0xa90
  ____sys_sendmsg+0x76a/0x780
  __sys_sendmsg+0x27c/0x340
  do_syscall_64+0x43/0x90
  entry_SYSCALL_64_after_hwframe+0x63/0xcd

Tested-by: Hangbin Liu <liuhangbin@gmail.com>
Fixes: f185de28d9ae ("mld: add new workqueues for process mld events")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

v2:
 - Fix commit message
 - Add reproducer script by Hangbin Liu

 net/ipv6/mcast.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 7f695c39d9a8..87c699d57b36 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1522,7 +1522,6 @@ static void mld_query_work(struct work_struct *work)
 
 		if (++cnt >= MLD_MAX_QUEUE) {
 			rework = true;
-			schedule_delayed_work(&idev->mc_query_work, 0);
 			break;
 		}
 	}
@@ -1533,8 +1532,10 @@ static void mld_query_work(struct work_struct *work)
 		__mld_query_work(skb);
 	mutex_unlock(&idev->mc_lock);
 
-	if (!rework)
-		in6_dev_put(idev);
+	if (rework && queue_delayed_work(mld_wq, &idev->mc_query_work, 0))
+		return;
+
+	in6_dev_put(idev);
 }
 
 /* called with rcu_read_lock() */
@@ -1624,7 +1625,6 @@ static void mld_report_work(struct work_struct *work)
 
 		if (++cnt >= MLD_MAX_QUEUE) {
 			rework = true;
-			schedule_delayed_work(&idev->mc_report_work, 0);
 			break;
 		}
 	}
@@ -1635,8 +1635,10 @@ static void mld_report_work(struct work_struct *work)
 		__mld_report_work(skb);
 	mutex_unlock(&idev->mc_lock);
 
-	if (!rework)
-		in6_dev_put(idev);
+	if (rework && queue_delayed_work(mld_wq, &idev->mc_report_work, 0))
+		return;
+
+	in6_dev_put(idev);
 }
 
 static bool is_in(struct ifmcaddr6 *pmc, struct ip6_sf_list *psf, int type,
-- 
2.17.1

Re: [PATCH net v2] net: mld: fix reference count leak in mld_{query | report}_work()

[Eric Dumazet]

On Fri, Jul 22, 2022 at 7:06 PM Taehee Yoo <ap420073@gmail.com> wrote:
>
> mld_{query | report}_work() processes queued events.
> If there are too many events in the queue, it re-queue a work.
> And then, it returns without in6_dev_put().
> But if queuing is failed, it should call in6_dev_put(), but it doesn't.
> So, a reference count leak would occur.
>
> THREAD0                         THREAD1
> mld_report_work()
>                                 spin_lock_bh()
>                                 if (!mod_delayed_work())
>                                         in6_dev_hold();
>                                 spin_unlock_bh()
>         spin_lock_bh()
>         schedule_delayed_work()
>         spin_unlock_bh()
>
> Script to reproduce(by Hangbin Liu):
>    ip netns add ns1
>    ip netns add ns2
>    ip netns exec ns1 sysctl -w net.ipv6.conf.all.force_mld_version=1
>    ip netns exec ns2 sysctl -w net.ipv6.conf.all.force_mld_version=1
>
>    ip -n ns1 link add veth0 type veth peer name veth0 netns ns2
>    ip -n ns1 link set veth0 up
>    ip -n ns2 link set veth0 up
>
>    for i in `seq 50`; do
>            for j in `seq 100`; do
>                    ip -n ns1 addr add 2021:${i}::${j}/64 dev veth0
>                    ip -n ns2 addr add 2022:${i}::${j}/64 dev veth0
>            done
>    done
>    modprobe -r veth
>    ip -a netns del
>

> Tested-by: Hangbin Liu <liuhangbin@gmail.com>
> Fixes: f185de28d9ae ("mld: add new workqueues for process mld events")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net v2] net: mld: fix reference count leak in mld_{query | report}_work()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 22 Jul 2022 17:06:35 +0000 you wrote:
> mld_{query | report}_work() processes queued events.
> If there are too many events in the queue, it re-queue a work.
> And then, it returns without in6_dev_put().
> But if queuing is failed, it should call in6_dev_put(), but it doesn't.
> So, a reference count leak would occur.
> 
> THREAD0				THREAD1
> mld_report_work()
> 				spin_lock_bh()
> 				if (!mod_delayed_work())
> 					in6_dev_hold();
> 				spin_unlock_bh()
> 	spin_lock_bh()
> 	schedule_delayed_work()
> 	spin_unlock_bh()
> 
> [...]

Here is the summary with links:
  - [net,v2] net: mld: fix reference count leak in mld_{query | report}_work()
    https://git.kernel.org/netdev/net/c/3e7d18b9dca3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html