mmap(MAP_STACK): on stack grow, use original protection

[kib]

Summary

If mprotect(2) changed protection in the bottom of the currently grown
stack region, currently the changed protection would be used for the
stack grow on next fault.  This is arguably unexpected.

Store the original protection for the entry at mmap(2) time in the
offset member of the gap vm_map_entry, and use it for protection of the
grown stack region.

PR:     272585
Reported by:    John F. Carr <[e-mail]>

[markj]

sys/vm/vm_map.c

4587

This assignment can be deduplicated now.

4763–4764

I'd add a comment, /* The "offset" field is overloaded, see vm_map_stack_locked(). */

[kib]

Update comments, deduplicate code.

[markj]

sys/vm/vm_map.c

4768

Instead of applying this subtle reasoning, can we simply load gap_entry->offset into a local variable before potentially deleting the entry?

[kib]

Use local for prot in stack grow.

[alc]

I think that the overloading of the vm map entry fields should be mentioned where the structure is defined.

P.S. While looking at this code, I would if it would be beneficial to have vm_map_insert() optionally return the new (or extended) map entry since finding the prev or succ map entry is no longer as trivial as it once was.

sys/vm/vm_map.c

4496

Alphabetize.

4578–4580

4764–4767

Move this comment up to where "prot" is initialized.

[olce]

Wouldn't it be better to use an anonymous union with offset as a member, and prot as another?

sys/vm/vm_map.c

4764–4767

Move this comment up to where "prot" is initialized.

Yes, that's the "surprising" statement.

[kib]

In D41089#935608, @alc wrote:

P.S. While looking at this code, I would if it would be beneficial to have vm_map_insert() optionally return the new (or extended) map entry since finding the prev or succ map entry is no longer as trivial as it once was.

I will do this as a followup.

[kib]

Manage comments and style.

[alc]

I'm okay with this. I just reread the mmap(2) description of MAP_STACK, and I don't think that it needs to change to explicitly mention the behavior implemented here. This change implements the behavior that I believe that a reader would most likely expect.

sys/vm/vm_map.c

4579

[markj]

In D41089#935642, @olce.freebsd_certner.fr wrote:

Wouldn't it be better to use an anonymous union with offset as a member, and prot as another?

I'm on the fence. Anonymous unions wouldn't really protect against bugs that can arise from this overloading. I'd wait to see more examples of map entries that need auxiliary fields before trying to change the vm_map_entry type definition.

[kib]

In D41089#936060, @markj wrote:

In D41089#935642, @olce.freebsd_certner.fr wrote:

Wouldn't it be better to use an anonymous union with offset as a member, and prot as another?

I'm on the fence. Anonymous unions wouldn't really protect against bugs that can arise from this overloading. I'd wait to see more examples of map entries that need auxiliary fields before trying to change the vm_map_entry type definition.

Anonymous unions are c11 feature, they are not in c99 AFAIR. Although we mostly acccept that kernel requires c11, vm/vm_map.h is user-visible header and should definitely be c99-compliant, if not c89.