linux-kernel - Re: [Patch] x86,mm: check freeze request in page fault handler

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140812120905.GB7049@dhcp22.suse.cz>
Date:	Tue, 12 Aug 2014 14:09:05 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	Cong Wang <xiyou.wangcong@...il.com>
Cc:	linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	David Rientjes <rientjes@...gle.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>, Tejun Heo <tj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [Patch] x86,mm: check freeze request in page fault handler

On Mon 11-08-14 17:53:55, Cong Wang wrote:
> When a process triggers a page fault and kernel keeps
> trying to retry the fault, there is no chance for this process
> to be frozen, so the freeze request will always be pending.

The retry cannot happen indefinitely, no?

Besides that the patch is broken in at least 2 ways. You are not
releasing mmap_sem and this will break memcg OOM killer handling.

If a memcg is under OOM (because of hard limit) then try_charge
calls mem_cgroup_oom which marks the current task with OOM
information. Notably takes a reference to memcg->css. The charge fail
will then gets up the pagefault stack until we get to mm_fault_error
where you put the task into freezer and then returns without
pagefault_out_of_memory which would handle memcg specific parts in
mem_cgroup_oom_synchronize. If the task wakes up and the page fault
retry succeeds (because some charges were released in the meantime) then
you leak a reference to memcg->css.

Besides that the whole change would need a better justification. Why
other archs do not need this?

> This patch lets the page fault handler check pending
> freeze request and freeze current process if so.
> 
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: David Rientjes <rientjes@...gle.com>
> Cc: Michal Hocko <mhocko@...e.cz>
> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
> Cc: Tejun Heo <tj@...nel.org>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> ---
>  arch/x86/mm/fault.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index a241946..ad9728a 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -14,6 +14,7 @@
>  #include <linux/hugetlb.h>		/* hstate_index_to_shift	*/
>  #include <linux/prefetch.h>		/* prefetchw			*/
>  #include <linux/context_tracking.h>	/* exception_enter(), ...	*/
> +#include <linux/freezer.h>		/* try_to_freeze()		*/
>  
>  #include <asm/traps.h>			/* dotraplinkage, ...		*/
>  #include <asm/pgalloc.h>		/* pgd_*(), ...			*/
> @@ -885,6 +886,9 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
>  		up_read(&current->mm->mmap_sem);
>  		no_context(regs, error_code, address, 0, 0);
>  		return;
> +	} else if (signal_pending(current) && (error_code & PF_USER)) {
> +		if (try_to_freeze())
> +			return;
>  	}
>  
>  	if (fault & VM_FAULT_OOM) {
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/