Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935010AbdLRV1R (ORCPT ); Mon, 18 Dec 2017 16:27:17 -0500 Received: from hqemgate16.nvidia.com ([216.228.121.65]:9090 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934889AbdLRV1L (ORCPT ); Mon, 18 Dec 2017 16:27:11 -0500 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Mon, 18 Dec 2017 13:27:11 -0800 Subject: Re: [PATCH v5] mmap.2: MAP_FIXED updated documentation To: "Michael Kerrisk (man-pages)" CC: linux-man , , Michael Ellerman , , LKML , , Jann Horn , Matthew Wilcox , Michal Hocko , Mike Rapoport , Cyril Hrubis , Michal Hocko , Pavel Machek References: <20171212002331.6838-1-jhubbard@nvidia.com> <3a07ef4d-7435-7b8d-d5c7-3bce80042577@gmail.com> X-Nvconfidentiality: public From: John Hubbard Message-ID: Date: Mon, 18 Dec 2017 13:27:10 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <3a07ef4d-7435-7b8d-d5c7-3bce80042577@gmail.com> X-Originating-IP: [10.110.48.28] X-ClientProxiedBy: HQMAIL102.nvidia.com (172.18.146.10) To HQMAIL107.nvidia.com (172.20.187.13) Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5508 Lines: 165 On 12/18/2017 11:15 AM, Michael Kerrisk (man-pages) wrote: > On 12/12/2017 01:23 AM, john.hubbard@gmail.com wrote: >> From: John Hubbard >> >> -- Expand the documentation to discuss the hazards in >> enough detail to allow avoiding them. >> >> -- Mention the upcoming MAP_FIXED_SAFE flag. >> >> -- Enhance the alignment requirement slightly. >> >> CC: Michael Ellerman >> CC: Jann Horn >> CC: Matthew Wilcox >> CC: Michal Hocko >> CC: Mike Rapoport >> CC: Cyril Hrubis >> CC: Michal Hocko >> CC: Pavel Machek >> Signed-off-by: John Hubbard > > John, > > Thanks for the patch. I think you win the prize for the > most iterations ever on a man-pages patch! (And Michal, > thanks for helping out.) I've applied your patch, made > some minor tweaks, and removed the mention of > MAP_FIXED_SAFE, since I don't like to document stuff > that hasn't yet been merged. (I only later noticed the > fuss about the naming...) > Hi Michael, The final result looks nice, thanks for all the editing fixes. One last thing: reading through this, I think it might need a wording fix (this is my fault), in order to avoid implying that brk() or malloc() use dlopen(). Something approximately like this: diff --git a/man2/mmap.2 b/man2/mmap.2 index 79681b31e..1c0bd80de 100644 --- a/man2/mmap.2 +++ b/man2/mmap.2 @@ -250,8 +250,9 @@ suffice. The .BR dlopen (3) call will map the library into the process's address space. -Furthermore, almost any library call may be implemented using this technique. -Examples include +Furthermore, almost any library call may be implemented in a way that +adds memory mappings to the address space, either with this technique, +or by simply allocating memory. Examples include .BR brk (2), .BR malloc (3), .BR pthread_create (3), ...or does the current version seem OK to other people? thanks, -- John Hubbard NVIDIA > Cheers, > > Michael > >> --- >> >> Changes since v4: >> >> -- v2 ("mmap.2: MAP_FIXED is no longer discouraged") was applied already, >> so v5 is a merge, including rewording of the paragraph transitions. >> >> -- We seem to have consensus about what to say about alignment >> now, and this includes that new wording. >> >> Changes since v3: >> >> -- Removed the "how to use this safely" part, and >> the SHMLBA part, both as a result of Michal Hocko's >> review. >> >> -- A few tiny wording fixes, at the not-quite-typo level. >> >> Changes since v2: >> >> -- Fixed up the "how to use safely" example, in response >> to Mike Rapoport's review. >> >> -- Changed the alignment requirement from system page >> size, to SHMLBA. This was inspired by (but not yet >> recommended by) Cyril Hrubis' review. >> >> -- Formatting: underlined /proc//maps >> >> Changes since v1: >> >> -- Covered topics recommended by Matthew Wilcox >> and Jann Horn, in their recent review: the hazards >> of overwriting pre-exising mappings, and some notes >> about how to use MAP_FIXED safely. >> >> -- Rewrote the commit description accordingly. >> >> man2/mmap.2 | 32 ++++++++++++++++++++++++++++++-- >> 1 file changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/man2/mmap.2 b/man2/mmap.2 >> index a5a8eb47a..400cfda2d 100644 >> --- a/man2/mmap.2 >> +++ b/man2/mmap.2 >> @@ -212,8 +212,9 @@ Don't interpret >> .I addr >> as a hint: place the mapping at exactly that address. >> .I addr >> -must be a multiple of the page size. >> -If the memory region specified by >> +must be suitably aligned: for most architectures a multiple of page >> +size is sufficient; however, some architectures may impose additional >> +restrictions. If the memory region specified by >> .I addr >> and >> .I len >> @@ -226,6 +227,33 @@ Software that aspires to be portable should use this option with care, keeping >> in mind that the exact layout of a process' memory map is allowed to change >> significantly between kernel versions, C library versions, and operating system >> releases. >> +.IP >> +Furthermore, this option is extremely hazardous (when used on its own), because >> +it forcibly removes pre-existing mappings, making it easy for a multi-threaded >> +process to corrupt its own address space. >> +.IP >> +For example, thread A looks through >> +.I /proc//maps >> +and locates an available >> +address range, while thread B simultaneously acquires part or all of that same >> +address range. Thread A then calls mmap(MAP_FIXED), effectively overwriting >> +the mapping that thread B created. >> +.IP >> +Thread B need not create a mapping directly; simply making a library call >> +that, internally, uses >> +.I dlopen(3) >> +to load some other shared library, will >> +suffice. The dlopen(3) call will map the library into the process's address >> +space. Furthermore, almost any library call may be implemented using this >> +technique. >> +Examples include brk(2), malloc(3), pthread_create(3), and the PAM libraries >> +(http://www.linux-pam.org). >> +.IP >> +Newer kernels >> +(Linux 4.16 and later) have a >> +.B MAP_FIXED_SAFE >> +option that avoids the corruption problem; if available, MAP_FIXED_SAFE >> +should be preferred over MAP_FIXED. >> .TP >> .B MAP_GROWSDOWN >> This flag is used for stacks. >> > >