| Message ID | 20220722170635.14847-1-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3e7d18b9dca388940a19cae30bfc1f76dccd8c28 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: mld: fix reference count leak in mld_{query | report}_work() | expand |
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>
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!
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,
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(-)