vm: Fix racy checks for swap objects

[markj]

Summary

Commit 4b8365d752ef introduced the ability to dynamically register
VM object types, for use by tmpfs, which creates swap-backed objects.
As a part of this, checks for such objects changed from

object->type == OBJT_DEFAULT || object->type == OBJT_SWAP

to

object->type == OBJT_DEFAULT || (object->flags & OBJ_SWAP) != 0

In particular, objects of type OBJT_DEFAULT do not have OBJ_SWAP set;
the swap pager sets this flag when converting from OBJT_DEFAULT to
OBJT_SWAP.

A few of these checks are done without the object lock held. It turns
out that this can result in false negatives since the swap pager
converts objects like so:

object->type = OBJT_SWAP;
object->flags |= OBJ_SWAP;

See comment 16 of PR 258932 for an example of how this can cause
problems.

Fix the problem rather simplistically, by adding tests for
object->type == OBJT_SWAP in places where we locklessly test for a
swap-backed object. Suggestions for a more robust approach would be
appreciated. We could perhaps add a store barrier in
swp_pager_meta_build(), for instance.

PR: 258932
Fixes: 4b8365d752ef ("Add OBJT_SWAP_TMPFS pager")

[kib]

If you want to add a release fence to the code that sets obj->type to OBJT_SWAP, then reads need to get acquire fence. I do not think it is worth it.

sys/vm/vm_map.c

2826–2827

This is a micro-optimization for relatively rare operation. Why not remove the unlocked check at all?

[markj]

sys/vm/vm_map.c

2826–2827

Hmm, could it be that rtld is modifying protections on shared objects? So in a situation where many processing are exec()ing, we could see contention on the vnode object lock. Or maybe some of the earlier checks in this loop will catch that situation, I'm not sure.

I can't tell if the commit which added this check did so to address some specific workload, since it was part of a large push to reduce object lock contention. I'll try adding a counter to see how often it helps.

[kib]

sys/vm/vm_map.c

2826–2827

rtld modifying protection occurs for relro and textrel. For both cases, this must occur on shadow default/swap object over the vnode object. So I believe the lock used is of the swap object, not the backing vnode obj.

[markj]

sys/vm/vm_map.c

2826–2827

Based on some quick testing, you are right that this is a rare case. I'll try a few workloads and remove the racy check if things look ok.

[markj]

Remove the racy check in vm_map_protect().

[markj]

sys/vm/vm_map.c

2826–2827

Indeed, I don't think the check is very useful. After running poudriere for 24 hours I have:

vm.stats.map.protect_obj_lock_elided: 0
vm.stats.map.protect_obj_lock_not_elided: 13881

i.e., we reached this check 13881 times in total (when vm_map_protect() gets called ~1000 times per second), and avoided the object lock zero times. It seems to help mostly in the case where write access is enabled on a non-writeable non-anonymous object, which ought to be a rare situation.