From: John Hubbard <[email protected]>
Previously, MAP_FIXED was "discouraged", due to portability
issues with the fixed address. In fact, there are other, more
serious issues. Also, alignment requirements were a bit vague.
So:
-- 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.
Some of the wording is lifted from Matthew Wilcox's review
(the "Portability issues" section). The alignment requirements
section uses Cyril Hrubis' wording, with light editing applied.
Suggested-by: Matthew Wilcox <[email protected]>
Suggested-by: Cyril Hrubis <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
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/<pid>/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 | 40 ++++++++++++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/man2/mmap.2 b/man2/mmap.2
index 385f3bfd5..56b05cff1 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
@@ -222,8 +223,39 @@ part of the existing mapping(s) will be discarded.
If the specified address cannot be used,
.BR mmap ()
will fail.
-Because requiring a fixed address for a mapping is less portable,
-the use of this option is discouraged.
+.IP
+This option is extremely hazardous (when used on its own) and moderately
+non-portable.
+.IP
+Portability issues: a process's memory map may change significantly from one
+run to the next, depending on library versions, kernel versions and random
+numbers.
+.IP
+Hazards: this option 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/<pid>/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.
--
2.15.1
On Tue 05-12-17 19:14:34, [email protected] wrote:
> From: John Hubbard <[email protected]>
>
> Previously, MAP_FIXED was "discouraged", due to portability
> issues with the fixed address. In fact, there are other, more
> serious issues. Also, alignment requirements were a bit vague.
> So:
>
> -- 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.
>
> Some of the wording is lifted from Matthew Wilcox's review
> (the "Portability issues" section). The alignment requirements
> section uses Cyril Hrubis' wording, with light editing applied.
>
> Suggested-by: Matthew Wilcox <[email protected]>
> Suggested-by: Cyril Hrubis <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
Would you mind if I take this patch and resubmit it along with my
MAP_FIXED_SAFE (or whatever name I will end up with) next week?
Acked-by: Michal Hocko <[email protected]>
> ---
>
> 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/<pid>/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 | 40 ++++++++++++++++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/man2/mmap.2 b/man2/mmap.2
> index 385f3bfd5..56b05cff1 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
> @@ -222,8 +223,39 @@ part of the existing mapping(s) will be discarded.
> If the specified address cannot be used,
> .BR mmap ()
> will fail.
> -Because requiring a fixed address for a mapping is less portable,
> -the use of this option is discouraged.
> +.IP
> +This option is extremely hazardous (when used on its own) and moderately
> +non-portable.
> +.IP
> +Portability issues: a process's memory map may change significantly from one
> +run to the next, depending on library versions, kernel versions and random
> +numbers.
> +.IP
> +Hazards: this option 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/<pid>/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.
> --
> 2.15.1
>
--
Michal Hocko
SUSE Labs
On 12/10/2017 02:31 AM, Michal Hocko wrote:
> On Tue 05-12-17 19:14:34, [email protected] wrote:
>> From: John Hubbard <[email protected]>
>>
>> Previously, MAP_FIXED was "discouraged", due to portability
>> issues with the fixed address. In fact, there are other, more
>> serious issues. Also, alignment requirements were a bit vague.
>> So:
>>
>> -- 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.
>>
>> Some of the wording is lifted from Matthew Wilcox's review
>> (the "Portability issues" section). The alignment requirements
>> section uses Cyril Hrubis' wording, with light editing applied.
>>
>> Suggested-by: Matthew Wilcox <[email protected]>
>> Suggested-by: Cyril Hrubis <[email protected]>
>> Signed-off-by: John Hubbard <[email protected]>
>
> Would you mind if I take this patch and resubmit it along with my
> MAP_FIXED_SAFE (or whatever name I will end up with) next week?
>
> Acked-by: Michal Hocko <[email protected]>
Sure, that works for me. A tiny complication: I see that Michael
Kerrisk has already applied the much smaller v2 of this patch (the
one that "no longer discourages" the option, but that's all), as:
ffa518803e14 mmap.2: MAP_FIXED is no longer discouraged
so this one here will need to be adjusted slightly to merge
gracefully. Let me know if you want me to respin, or if you
want to handle the merge.
thanks,
--
John Hubbard
NVIDIA
On Sun 10-12-17 18:22:05, John Hubbard wrote:
> On 12/10/2017 02:31 AM, Michal Hocko wrote:
> > On Tue 05-12-17 19:14:34, [email protected] wrote:
> >> From: John Hubbard <[email protected]>
> >>
> >> Previously, MAP_FIXED was "discouraged", due to portability
> >> issues with the fixed address. In fact, there are other, more
> >> serious issues. Also, alignment requirements were a bit vague.
> >> So:
> >>
> >> -- 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.
> >>
> >> Some of the wording is lifted from Matthew Wilcox's review
> >> (the "Portability issues" section). The alignment requirements
> >> section uses Cyril Hrubis' wording, with light editing applied.
> >>
> >> Suggested-by: Matthew Wilcox <[email protected]>
> >> Suggested-by: Cyril Hrubis <[email protected]>
> >> Signed-off-by: John Hubbard <[email protected]>
> >
> > Would you mind if I take this patch and resubmit it along with my
> > MAP_FIXED_SAFE (or whatever name I will end up with) next week?
> >
> > Acked-by: Michal Hocko <[email protected]>
>
> Sure, that works for me. A tiny complication: I see that Michael
> Kerrisk has already applied the much smaller v2 of this patch (the
> one that "no longer discourages" the option, but that's all), as:
>
> ffa518803e14 mmap.2: MAP_FIXED is no longer discouraged
>
> so this one here will need to be adjusted slightly to merge
> gracefully. Let me know if you want me to respin, or if you
> want to handle the merge.
Yes, please respin.
--
Michal Hocko
SUSE Labs