linux-kernel - Re: [PATCH] mm/fs: don't keep pages when receiving a pending SIGKILL in __get_user

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 16 Jan 2014 20:59:26 +0800
From:	Xishi Qiu <qiuxishi@...wei.com>
To:	David Rientjes <rientjes@...gle.com>
CC:	Li Zefan <lizefan@...wei.com>, <robin.yb@...wei.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mel Gorman <mgorman@...e.de>, Rik van Riel <riel@...hat.com>,
	<linux-fsdevel@...r.kernel.org>, <linux-mm@...ck.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm/fs: don't keep pages when receiving a pending SIGKILL
 in __get_user_pages()

On 2014/1/16 7:15, David Rientjes wrote:

> On Wed, 15 Jan 2014, Xishi Qiu wrote:
> 
>> In the process IO direction, dio_refill_pages will call get_user_pages_fast 
>> to map the page from user space. If ret is less than 0 and IO is write, the 
>> function will create a zero page to fill data. This may work for some file 
>> system, but in some device operate we prefer whole write or fail, not half 
>> data half zero, e.g. fs metadata, like inode, identy.
>> This happens often when kill a process which is doing direct IO. Consider 
>> the following cases, the process A is doing IO process, may enter __get_user_pages 
>> function, if other processes send process A SIG_KILL, A will enter the 
>> following branches 
>> 		/*
>> 		 * If we have a pending SIGKILL, don't keep faulting
>> 		 * pages and potentially allocating memory.
>> 		 */
>> 		if (unlikely(fatal_signal_pending(current)))
>> 			return i ? i : -ERESTARTSYS;
>> Return current pages. direct IO will write the pages, the subsequent pages 
>> which can’t get will use zero page instead. 
>> This patch will modify this judgment, if receive SIG_KILL, release pages and 
>> return an error. Direct IO will find no blocks_available and return error 
>> direct, rather than half IO data and half zero page.
>>
>> Signed-off-by: Xishi Qiu <qiuxishi@...wei.com>
>> Signed-off-by: Bin Yang <robin.yb@...wei.com>
> 
> It's scary to change the behavior of gup when some callers may want the 
> exact opposite of what you're intending here, which is sane fallback by 
> mapping the zero page.  In fact, gup never does put_page() itself and 
> __get_user_pages() always returns the number of pages pinned and may not 
> equal what is passed.
> 
> So, this definitely isn't the right solution for a special-case direct IO.  
> Instead, it would be better to code this directly in the caller and 
> compare the return value with nr_pages in dio_refill_pages() and then do 
> the put_page() itself before falling back to ZERO_PAGE().

Hi Rientjes,
You are right, we should not change the behavior of gup.
I have a question, if we only get a part of the pages from get_user_pages_fast(),
shall we write them to the disk? or add a check before write?
I'm not familiar with fs.

dio_refill_pages()
	get_user_pages_fast()

Thanks,
Xishi Qiu

--
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/