linux-kernel - Re: [PATCH v2 2/3] mm, thp: fix possible circular locking dependency caused by sum_vm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20160527084039.GI2322@bbox>
Date:	Fri, 27 May 2016 17:40:39 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Ebru Akagunduz <ebru.akagunduz@...il.com>
CC:	<linux-mm@...ck.org>, <hughd@...gle.com>, <riel@...hat.com>,
	<akpm@...ux-foundation.org>, <kirill.shutemov@...ux.intel.com>,
	<n-horiguchi@...jp.nec.com>, <aarcange@...hat.com>,
	<iamjoonsoo.kim@....com>, <gorcunov@...nvz.org>,
	<linux-kernel@...r.kernel.org>, <mgorman@...e.de>,
	<rientjes@...gle.com>, <vbabka@...e.cz>,
	<aneesh.kumar@...ux.vnet.ibm.com>, <hannes@...xchg.org>,
	<mhocko@...e.cz>, <boaz@...xistor.com>
Subject: Re: [PATCH v2 2/3] mm, thp: fix possible circular locking dependency
 caused by sum_vm_event()

Hi Ebru,

On Fri, May 27, 2016 at 10:59:23AM +0300, Ebru Akagunduz wrote:
> Nested circular locking dependency detected by kernel robot (udevadm).
> 
>   udevadm/221 is trying to acquire lock:
>    (&mm->mmap_sem){++++++}, at: [<ffffffff81262543>] __might_fault+0x83/0x150
>   but task is already holding lock:
>    (s_active#12){++++.+}, at: [<ffffffff813315ee>] kernfs_fop_write+0x8e/0x250
>   which lock already depends on the new lock.
> 
>  Possible unsafe locking scenario:
> 
>  CPU0                    CPU1
>  ----                    ----
>  lock(s_active);
>                          lock(cpu_hotplug.lock);
>                          lock(s_active);
>  lock(&mm->mmap_sem);
> 
>  the existing dependency chain (in reverse order) is:
>  -> #2 (s_active#12){++++.+}:
>         [<ffffffff8117da2c>] lock_acquire+0xac/0x180
>         [<ffffffff8132f50a>] __kernfs_remove+0x2da/0x410
>         [<ffffffff81330630>] kernfs_remove_by_name_ns+0x40/0x90
>         [<ffffffff813339fb>] sysfs_remove_file_ns+0x2b/0x70
>         [<ffffffff81ba8a16>] device_del+0x166/0x320
>         [<ffffffff81ba943c>] device_destroy+0x3c/0x50
>         [<ffffffff8105aa61>] cpuid_class_cpu_callback+0x51/0x70
>         [<ffffffff81131ce9>] notifier_call_chain+0x59/0x190
>         [<ffffffff81132749>] __raw_notifier_call_chain+0x9/0x10
>         [<ffffffff810fe6b0>] __cpu_notify+0x40/0x90
>         [<ffffffff810fe890>] cpu_notify_nofail+0x10/0x30
>         [<ffffffff810fe8d7>] notify_dead+0x27/0x1e0
>         [<ffffffff810fe273>] cpuhp_down_callbacks+0x93/0x190
>         [<ffffffff82096062>] _cpu_down+0xc2/0x1e0
>         [<ffffffff810ff727>] do_cpu_down+0x37/0x50
>         [<ffffffff8110003b>] cpu_down+0xb/0x10
>         [<ffffffff81038e4d>] _debug_hotplug_cpu+0x7d/0xd0
>         [<ffffffff8435d6bb>] debug_hotplug_cpu+0xd/0x11
>         [<ffffffff84352426>] do_one_initcall+0x138/0x1cf
>         [<ffffffff8435270a>] kernel_init_freeable+0x24d/0x2de
>         [<ffffffff8209533a>] kernel_init+0xa/0x120
>         [<ffffffff820a7972>] ret_from_fork+0x22/0x50
> 
>  -> #1 (cpu_hotplug.lock#2){+.+.+.}:
>         [<ffffffff8117da2c>] lock_acquire+0xac/0x180
>         [<ffffffff820a20d1>] mutex_lock_nested+0x71/0x4c0
>         [<ffffffff810ff526>] get_online_cpus+0x66/0x80
>         [<ffffffff81246fb3>] sum_vm_event+0x23/0x1b0
>         [<ffffffff81293768>] collapse_huge_page+0x118/0x10b0
>         [<ffffffff81294c5d>] khugepaged+0x55d/0xe80
>         [<ffffffff81130304>] kthread+0x134/0x1a0
>         [<ffffffff820a7972>] ret_from_fork+0x22/0x50
> 
>  -> #0 (&mm->mmap_sem){++++++}:
>         [<ffffffff8117bf61>] __lock_acquire+0x2861/0x31f0
>         [<ffffffff8117da2c>] lock_acquire+0xac/0x180
>         [<ffffffff8126257e>] __might_fault+0xbe/0x150
>         [<ffffffff8133160f>] kernfs_fop_write+0xaf/0x250
>         [<ffffffff812a8933>] __vfs_write+0x43/0x1a0
>         [<ffffffff812a8d3a>] vfs_write+0xda/0x240
>         [<ffffffff812a8f84>] SyS_write+0x44/0xa0
>         [<ffffffff820a773c>] entry_SYSCALL_64_fastpath+0x1f/0xbd
> 
> This patch moves sum_vm_event() before taking down_write(&mm->mmap_sem)
> to solve dependency lock.

sum_vm_event works with CONFIG_VM_EVENT_COUNTERS so if a system doesn't
enable the config, THP swapin throttling logic is void in that case.
Right? If so, shouldn't we approach another way?

If it was already discussed, sorry for bothering you.

> 
> Signed-off-by: Ebru Akagunduz <ebru.akagunduz@...il.com>
> ---
> Changes in v2:
>  - Nothing changed
> 
>  mm/huge_memory.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 5e71d7a..9adf1c7 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2452,6 +2452,9 @@ static void collapse_huge_page(struct mm_struct *mm,
>  		goto out_nolock;
>  	}
>  
> +	swap = get_mm_counter(mm, MM_SWAPENTS);
> +	curr_allocstall = sum_vm_event(ALLOCSTALL);
> +
>  	/*
>  	 * Prevent all access to pagetables with the exception of
>  	 * gup_fast later hanlded by the ptep_clear_flush and the VM
> @@ -2484,8 +2487,6 @@ static void collapse_huge_page(struct mm_struct *mm,
>  		goto out;
>  	}
>  
> -	swap = get_mm_counter(mm, MM_SWAPENTS);
> -	curr_allocstall = sum_vm_event(ALLOCSTALL);
>  	/*
>  	 * Don't perform swapin readahead when the system is under pressure,
>  	 * to avoid unnecessary resource consumption.
> -- 
> 1.9.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@...ck.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>