More fixes for stacks

[kib]

Summary

Add vm_map_insert1(9)

The function returns the newly created entry.
Use vm_map_insert1() in stack grow code to avoid gap entry re-lookup.

Suggested by:   alc

vm_map_growstack(): handle max protection for stacks

Do not assume that protection is same as max_protection.  Store both in
offset, packed in the same way as the prot syscall parameter.

vm_map_protect(): handle stack protection stored in the stack guard

mprotect(2) on the stack region needs to adjust guard stored protection,
so that e.g. enable executing on stack worked properly on stack growth.

Reported by:    dchagin

vm_map: do not allow to merge stack gap entries

At least, offset handling is wrong for them.

vm_map_growstack(): restore stack gap data if gap entry was removed

and then restored.

vm_map_protect(): propagate lowest stack segment protection to the grow gap

This seems to be required for Linux emulation.

vm_map_protect(): add VM_MAP_PROTECT_GROWSDOWN flag

which requests to propagate lowest stack segment protection to the grow gap.
This seems to be required for Linux emulation.

Reported by:    dchagin

linuxolator: implement Linux' PROT_GROWSDOWN

From the Linux man page for mprotect(2):
   PROT_GROWSDOWN
       Apply  the  protection  mode  down to the beginning of a mapping
       that grows downward (which should be a stack segment or a
       segment mapped with the MAP_GROWSDOWN flag set).

Reported by:    dchagin

[kib]

In D41099#938145, @dchagin wrote:

Thank you for fixing this. Native test fine, however glibc rtld still fails under Linuxulator.
Glibc rtld calls mprotect of stack with len=pagesize, so after stack looks like:
00007ffffffdd000-00007fffffffc000 rw-p 00000000 00:00 0
00007fffffffc000-00007fffffffd000 rwxp 0003e000 00:00 0 [stack]

Can this be fixed as well? it's hard to fix at Linuxulator level.

Can you provide me with the self-contained native test for this case?

[dougm]

sys/vm/vm_map.c

1611

What if res is NULL? If I didn't read carefully, I'd think I could pass NULL as the last argument if I didn't care about the new entry result.

Consider dropping insert1 and just have insert take an extra argument that can be a NULL when the new entry is unwanted.

2806

if ((new_prot & ~gmax_prot) != 0 ||

is an alternative.

2894

It's a small thing, but everywhere else in the code, I see GAP_DN before GAP_UP.

4555

Why not use insert1 here to get new_entry?

sys/vm/vm_map.h

528

I don't see any callers outside vm_map.c, so why is this externally visible?

[kib]

sys/vm/vm_map.c

1611

vm_map_insert() is quite popular KPI, used a lot outside of vm/. I decided that it is too much churn to change it.

I caller does not need the entry, it can use vm_map_insert(). vm_map_insert1() does not accept NULL res, which avoids NULL checks.

2806

This change is dropped in the latest version of the patch. For new gap entry code, I prefer to use literally the same text as for regular map entries.

sys/vm/vm_map.h

528

There is a strange code in sys/security/mac/mac_process.c mac_proc_vm_revoke_recurse().

[kib]

Handle dougm notes. Add a hack to propagate lowest stack segment permissions to the gap, which is what Linux requires.

[dougm]

sys/vm/vm_map.c

2742

if (!CONTAINS_SET(max_prot, new_prot) || !CONTAINS_SET(max_prot, new_maxprot))

2747

if (CONTAINS_BITS(flags, VM_MAP_PROTECT_SET_ROOT | VM_MAP_PROTECT_SET_MAXROOT) && !CONTAINS_BITS(new_maxprot, new_prot))

2757

CONTAINS_BITS(new_prot, VM_PROT_WRITE | VM_PROT_EXECUTE)

2767

#define CONTAINS_BITS(set, bits) ((~(set) & (bits)) == 0)

2786

if ((flags & VM_MAP_PROTECT_SET_PROT) == 0)
new_prot = entry->protection;
else if (!CONTAINS_BITS(entry->max_protection, new_prot) {

vm_map_unlock(map);
return (KERN_PROTECTION_FAILURE);

}

2788

else if (!CONTAINS_BITS(entry->max_protection, new_maxprot)) {
unlock; return;
}

2796

I think you don't have to do this if VM_MAP_PROTECT_SET_PROT and VM_MAP_PROTECT_SET_MAXPROT are set in flags; if they are set, then new_prot and new_maxprot haven't changed since the last iteration, when you checked them already.

2800

if (CONTAINS_BITS(prev_entry->eflags, MAP_ENTRY_GUARD | MAP_ENTRY_STACK_GAP_DN)

2929

CONTAINS_SET(prev_entry->eflags, MAP_ENTRY_GUARD | MAP_ENTRY_STACK_GAP_DN)

[kib]

sys/vm/vm_map.c

2788

I did both of chunk in the spirit of your suggestion, but differently. There is no need to recalculate new_maxprot and new_prot if the checks are local.

2796

prev_entry might be out of the range specified for this call to vm_map_protect(), this is why I have to check prev_entry for perms.

[kib]

CONTAINS_BITS

[kib]

Add VM_MAP_PROTECT_GROWSDOWN and implement LINUX_PROT_GROWSDOWN

[kib]

This version should fix all known regressions with the stack/gap protection handling so far.

[dougm]

sys/vm/vm_map.c

2740

if (!CONTAINS_BITS(max_prot, new_prot | new_maxprot))

2776

old_prot = 0;
if ((flags & VM_MAP_PROTECT_SET_PROT) != 0)

old_prot |= new_prot;

if ((flags & VM_MAP_PROTECT_SET_MAXPROT) != 0)

old_prot |= new_maxprot;

2799

if (!CONTAINS_BITS(entry->max_protection, old_prot)) {

 vm_map_unlock(map);
return (KERN_PROTECTION_FAILURE);

}

[kib]

More CONTAINS_BITS() code compaction.
Stop VM_MAP_PROTECT_GROWSDOWN loop to find stack gap, when we found the first map entry.

[dougm]

sys/vm/vm_map.c

2799

I meant that you could compute old_prot before the for loop, since it will have the same value in every iteration.

[kib]

Move old_prot calculation for phase1 out of loop.
Kill phase1 helper since it is shorter to inline.

[kib]

Unlock map on error.
Simplify and unify phase1 guard and non-guard entries handling even more.

[dougm]

sys/vm/vm_map.c

2726

I think you can drop "_phase3" from the name.

[dchagin]

Just to say, all glibc stack related tests are passed now, thank you.

[alc]

sys/vm/vm_map.c

1605

1606

2746

There is an extra space.

[kib]

Rename vm_map_protect_gap_phase3().
Editorial changes and comments and style.

[dougm]

sys/vm/vm_map.c

2774

Add a comment explaining this next bit.

[kib]

Added comment for vm_map_protect() implementation of PROT_GROWSDOWN.
Reset start to the original value on 'again' iteration, since we must re-lookup gap from the user-specified point.

[markj]

sys/vm/vm_map.c

2777

2781

old_prot isn't the right name for this value. Maybe check_prot?

2792

max_prot = (entry->eflags & MAP_ENTRY_GUARD) != 0 ?
    PROT_MAX_EXTRACT(entry->offset) : entry->max_protection;
if (!CONTAINS_BITS(max_prot, check_prot)) {
    ...
}

would be clearer to me.

4601

Does vm_map_mergeable_neighbors() need an update to handle this overloading?

[kib]

sys/vm/vm_map.c

4601

Yes. The MAP_ENTRY_NOMERGE_MASK was updated to include gaps. See the corresponding commit in the list.

[kib]

Add check_prot and max_prot locals.

[alc]

sys/vm/vm_map.c

1604–1608

1835

In fact, only a part of the object may be mapped. This is an old error.

[dougm]

sys/vm/vm_map.c

2340

A suggested comment rewrite, to include something about the return value:

Compare two consecutive map entries. If they can be merged, expand the range of the second to cover the range of the first and delete the first. Return the surviving map entry that covers the start of the combined range.

The map must be locked.

[dougm]

sys/vm/vm_map.c

2340

After verbal feedback from another reviewer, another try:

Consider two map entries that represent consecutive ranges. If the entries can be merged, expand the range of the second to cover the range of the first and delete the first. Then return the map entry that includes the first range.

[kib]

sys/vm/vm_map.c

1835

The last sentence in this paragraph is also not relevant, I removed it.

2340

I changed the first word from 'consider' to 'compare'. Is it fine?

[kib]

Comment editing.

In the first phase of vm_map_protect(), do skip MAP_ENTRY_GUARDs that are not stack grow areas (reported by pho).

[dougm]

sys/vm/vm_map.c

2340

Yes, it's fine.