KVM: mm: fd-based approach for supporting KVM guest private memory

Quoted 24 lines · Ackerley Tng

One issue I’ve found so far is that the pointer to kvm (gmem->kvm) is not cleaned up, and hence it is possible to crash the host kernel in the following way

1. Create a KVM VM
2. Create a guest mem fd on that VM
3. Create a memslot with the guest mem fd (hence binding the fd to the VM)
4. Close/destroy the KVM VM
5. Call fallocate(PUNCH_HOLE) on the guest mem fd, which uses gmem->kvm when it tries to do invalidation.

I then tried to clean up the gmem->kvm pointer during unbinding when the KVM VM is destroyed.

That works, but then I realized there’s a simpler way to use the pointer after freeing:

1. Create a KVM VM
2. Create a guest mem fd on that VM
3. Close/destroy the KVM VM
4. Call fallocate(PUNCH_HOLE) on the guest mem fd, which uses gmem->kvm when it tries to do invalidation.

Quoted 3 lines

Perhaps binding should mean setting the gmem->kvm pointer in addition to gmem->bindings. This makes binding and unbinding symmetric and avoids the use-after-frees described above.

Hrm, that would work, though it's a bit convoluted, e.g. would require detecting when the last binding is being removed. A similar (also ugly) solution would be to nullify gmem->kvm when KVM dies.

I don't love either approach idea because it means a file created in the context of a VM can outlive the VM itself, and then userspace ends up with a file descriptor that it can't do anything with except close(). I doubt that matters in practice though, e.g. when the VM dies, all memory can be freed so that the file ends up being little more than a shell. And if we go that route, there's no need to grab a reference to the file during bind, KVM can just grab a longterm reference when the file is initially created and then drop it when KVM dies (and nullifies gmem->kvm).

Blech, another wart is that I believe gmem would need to do __module_get() during file creation to prevent kvm.ko from being unloaded after the last VM dies. Ah, but that'd also be true if we went with a system-scoped KVM ioctl(), so I suppose it's not _that_ ugly.

Exchanging references (at binding or at creation) doesn't work, because that creates a circular dependency, i.e. gmem and KVM would pin each other.

A "proper" refcounting approach, where the file pins KVM and not vice versa, gets nasty because of how KVM's memslots work. The least awful approach I can think of would be to delete the associated memslot(s) when the file is released, possibly via deferred work to avoid deadlock issues. Not the prettiest thing ever and in some ways that'd yield an even worse ABI.

Side topic, there's a second bug (and probably more lurking): kvm_swap_active_memslots()'s call to synchronize_srcu_expedited() is done _before_ the call to kvm_gmem_unbind(), i.e. doesn't wait for readers in kvm_gmem_invalidate_begin() to go away. The easy solution for that one is to add another synchronize_srcu_expedited() after unbinding.

Quoted 7 lines · Ackerley Tng

Hi Sean,

Thanks for implementing this POC!

... snip ...

I don't love either approach idea because it means a file created in the context of a VM can outlive the VM itself, and then userspace ends up with a file descriptor that it can't do anything with except close(). I doubt that matters in practice though, e.g. when the VM dies, all memory can be freed so that the file ends up being little more than a shell. And if we go that route, there's no need to grab a reference to the file during bind, KVM can just grab a longterm reference when the file is initially created and then drop it when KVM dies (and nullifies gmem->kvm).

... snip ...

My preference is to make it a VM-scoped ioctl(), if it ends up being a KVM ioctl() and not a common syscall. If the file isn't tightly coupled to a single VM, then punching a hole is further complicated by needing to deal with invalidating multiple regions that are bound to different @kvm instances. It's not super complex, but AFAICT having the ioctl() be system-scoped doesn't add value, e.g. I don't think having one VM own the memory will complicate even if/when we get to the point where VMs can share "private" memory, and the gmem code would still need to deal with grabbing a module reference.

Quoted 19 lines · Sean Christopherson

Quoted 7 lines · Ackerley Tng

Hi Sean,

Thanks for implementing this POC!

... snip ...

I don't love either approach idea because it means a file created in the context of a VM can outlive the VM itself, and then userspace ends up with a file descriptor that it can't do anything with except close(). I doubt that matters in practice though, e.g. when the VM dies, all memory can be freed so that the file ends up being little more than a shell. And if we go that route, there's no need to grab a reference to the file during bind, KVM can just grab a longterm reference when the file is initially created and then drop it when KVM dies (and nullifies gmem->kvm).

... snip ...

Quoted 8 lines

My preference is to make it a VM-scoped ioctl(), if it ends up being a KVM ioctl() and not a common syscall. If the file isn't tightly coupled to a single VM, then punching a hole is further complicated by needing to deal with invalidating multiple regions that are bound to different @kvm instances. It's not super complex, but AFAICT having the ioctl() be system-scoped doesn't add value, e.g. I don't think having one VM own the memory will complicate even if/when we get to the point where VMs can share "private" memory, and the gmem code would still need to deal with grabbing a module reference.

I’d like to follow up on this discussion about a guest_mem file outliving the VM and whether to have a VM-scoped ioctl or a KVM ioctl.

Here's a POC of delayed binding of a guest_mem file to a memslot, where the guest_mem file outlives the VM [1].

I also hope to raise some points before we do the first integration of guest_mem patches!

A use case for guest_mem inodes outliving the VM is when the host VMM needs to be upgraded.

The guest_mem inode is passed between two VMs on the same host machine and all memory associated with the inode needs to be retained.

To support the above use case, binding of memslots is delayed until first use, so that the following inode passing flow can be used:

1. Source (old version of host VMM) process passes guest_mem inodes to destination (new version of host VMM) process via unix sockets.
2. Destination process initializes memslots identical to source process.
3. Destination process invokes ioctl to migrate guest_mem inode over to destination process by unbinding all memslots from the source VM and binding them to the destination VM. (The kvm pointer is updated in this process)

Without delayed binding, step 2 will fail since initialization of memslots would check and find that the kvm pointer in the guest_mem inode points to the kvm in the source process.

These two patches contain the meat of the changes required to support delayed binding:

https://github.com/googleprodkernel/linux-cc/commit/93b31a006ef2e4dbe1ef0ec5d2534ca30f3bf60c
https://github.com/googleprodkernel/linux-cc/commit/dd5ac5e53f14a1ef9915c9c1e4cc1006a40b49df

Some things to highlight for the approach set out in these two patches:

1. Previously, closing the guest_mem file in userspace is taken to mean that all associated memory is to be removed and cleared. With these two patches, each memslot also holds a reference to the file (and therefore inode) and so even if the host VMM closes the fd, the VM will be able to continue to function.

This is desirable to userspace since closing the file should not be interpreted as a command to clear memory.

This is aligned with the
way tmpfs files are used with KVM before guest_mem: when the file is closed in userspace, the memory contents are still mapped and can still be used by the VM. fallocate(PUNCH_HOLE) is how userspace should command memory to be removed, just like munmap() would be used to remove memory from use by KVM.

2. Creating a guest mem file no longer depends on a specific VM and hence the guest_mem creation ioctl can be a system ioctl instead of a VM specific ioctl. This will also address Chao's concern at [3].

I also separated cleaning up files vs inodes in
https://github.com/googleprodkernel/linux-cc/commit/0f5aa18910c515141e57e05c4cc791022047a242,
which I believe is more aligned with how files and inodes are cleaned up in FSes.

This alignment makes it easier to extend gmem to hugetlb, for one.

While working on this, I was also wondering if we should perhaps be storing the inode pointer in slot->gmem instead of the file pointer? The memory is associated with an inode->mapping rather than the file. Are we binding to a userspace handle on the inode (store file pointer), or are we really referencing the inode (store inode pointer)?

...
And _if_ there is a VMM that instantiates memory before KVM_CREATE_VM, IMO making the ioctl() /dev/kvm scoped would have no meaningful impact on adapting userspace to play nice with the required ordering. If userspace can get at /dev/kvm, then it can do KVM_CREATE_VM, because the only input to KVM_CREATE_VM is the type, i.e. the only dependencies for KVM_CREATE_VM should be known/resolved long before the VMM knows it wants to use gmem.

...
Practically, I think that gives us a clean, intuitive way to handle intra-host migration. Rather than transfer ownership of the file, instantiate a new file for the target VM, using the gmem inode from the source VM, i.e. create a hard link. That'd probably require new uAPI, but I don't think that will be hugely problematic. KVM would need to ensure the new VM's guest_memfd can't be mapped until KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM (which would also need to verify the memslots/bindings are identical), but that should be easy enough to enforce.

That way, a VM, its memslots, and its SPTEs are tied to the file, while allowing the memory and the *contents* of memory to outlive the VM, i.e. be effectively transfered to the new target VM. And we'll maintain the invariant that each guest_memfd is bound 1:1 with a single VM.

As above, that should also help us draw the line between mapping memory into a VM (file), and freeing/reclaiming the memory (inode).

There will be extra complexity/overhead as we'll have to play nice with the possibility of multiple files per inode, e.g. to zap mappings across all files when punching a hole, but the extra complexity is quite small, e.g. we can use address_space.private_list to keep track of the guest_memfd instances associated with the inode.

Quoted 7 lines · Sean Christopherson

...
And _if_ there is a VMM that instantiates memory before KVM_CREATE_VM, IMO making the ioctl() /dev/kvm scoped would have no meaningful impact on adapting userspace to play nice with the required ordering. If userspace can get at /dev/kvm, then it can do KVM_CREATE_VM, because the only input to KVM_CREATE_VM is the type, i.e. the only dependencies for KVM_CREATE_VM should be known/resolved long before the VMM knows it wants to use gmem.

I am not sure about the benefits of tying gmem creation to any given kvm instance.

I think the most important requirement here is that a given gmem range is always tied to a single VM

This can be enforced when memslots are bound to the gmem files.

I believe "Required ordering" is that gmem files are created first and then supplied while creating the memslots whose gpa ranges can generate private memory accesses.
Is there any other ordering we want to enforce here?

Quoted 16 lines

Practically, I think that gives us a clean, intuitive way to handle intra-host migration. Rather than transfer ownership of the file, instantiate a new file for the target VM, using the gmem inode from the source VM, i.e. create a hard link. That'd probably require new uAPI, but I don't think that will be hugely problematic. KVM would need to ensure the new VM's guest_memfd can't be mapped until KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM (which would also need to verify the memslots/bindings are identical), but that should be easy enough to enforce.

That way, a VM, its memslots, and its SPTEs are tied to the file, while allowing the memory and the *contents* of memory to outlive the VM, i.e. be effectively transfered to the new target VM. And we'll maintain the invariant that each guest_memfd is bound 1:1 with a single VM.

As above, that should also help us draw the line between mapping memory into a VM (file), and freeing/reclaiming the memory (inode).

Quoted 5 lines

There will be extra complexity/overhead as we'll have to play nice with the possibility of multiple files per inode, e.g. to zap mappings across all files when punching a hole, but the extra complexity is quite small, e.g. we can use address_space.private_list to keep track of the guest_memfd instances associated with the inode.

Are we talking about a different usecase of sharing gmem fd across VMs other than intra-host migration?

If not, ideally only one of the files should be catering to the guest memory mappings at any given time. i.e. any inode should be ideally bound to (through the file) a single kvm instance,

as we we are planning to ensure that guest_memfd can't be mapped until KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM is invoked on the target side.

Hi Sean,

Thanks for implementing this POC!

I’ve started porting the selftests (both Chao’s and those I added [1]).

guest mem seems to cover the use cases that have been discussed and proposed so far, but I still need to figure out how gmem can work with

+ hugetlbfs
+ specification of/storing memory policy (for NUMA node bindings)
+ memory accounting - we may need to account for memory used separately, so that guest mem shows up separately on /proc/meminfo and similar places.

One issue I’ve found so far is that the pointer to kvm (gmem->kvm) is not cleaned up, and hence it is possible to crash the host kernel in the following way

1. Create a KVM VM
2. Create a guest mem fd on that VM
3. Create a memslot with the guest mem fd (hence binding the fd to the VM)
4. Close/destroy the KVM VM
5. Call fallocate(PUNCH_HOLE) on the guest mem fd, which uses gmem->kvm when it tries to do invalidation.

I then tried to clean up the gmem->kvm pointer during unbinding when the KVM VM is destroyed.

That works, but then I realized there’s a simpler way to use the pointer after freeing:

1. Create a KVM VM
2. Create a guest mem fd on that VM
3. Close/destroy the KVM VM
4. Call fallocate(PUNCH_HOLE) on the guest mem fd, which uses gmem->kvm when it tries to do invalidation.

Perhaps binding should mean setting the gmem->kvm pointer in addition to gmem->bindings. This makes binding and unbinding symmetric and avoids the use-after-frees described above.

This also means that creating a guest mem fd is no longer dependent on the VM. Perhaps we can make creating a gmem fd a system ioctl (like KVM_GET_API_VERSION and KVM_CREATE_VM) instead of a vm ioctl?

[1]
https://lore.kernel.org/all/[email protected]/T/

Ackerley

...
cold. I poked around a bit to see how we could avoid reinventing all of that infrastructure for fd-only memory, and the best idea I could come up with is basically a rehash of Kirill's very original "KVM protected memory" RFC[3], i.e. allow "mapping" fd-only memory, but ensure that memory is never actually present from hardware's perspective.

Code is available here if folks want to take a look before any kind of formal posting:

https://github.com/sean-jc/linux.git x86/kvm_gmem_solo

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/20230418-anfallen-irdisch-6993a61be10b@brauner
[3] https://lore.kernel.org/linux-mm/[email protected]

Quoted 8 lines · Sean Christopherson

...
cold. I poked around a bit to see how we could avoid reinventing all of that infrastructure for fd-only memory, and the best idea I could come up with is basically a rehash of Kirill's very original "KVM protected memory" RFC[3], i.e. allow "mapping" fd-only memory, but ensure that memory is never actually present from hardware's perspective.

I am most likely missing a lot of context here and possibly venturing into an infeasible/already shot down direction here. But I would still like to get this discussed here before we move on.

I am wondering if it would make sense to implement restricted_mem/guest_mem file to expose both private and shared memory regions, inline with Kirill's original proposal now that the file implementation is controlled by KVM.

Thinking from userspace perspective:
1) Userspace creates guest mem files and is able to mmap them but all accesses to these files result into faults as no memory is allowed to be mapped into userspace VMM pagetables.
2) Userspace registers mmaped HVA ranges with KVM with additional KVM_MEM_PRIVATE flag
3) Userspace converts memory attributes and this memory conversion allows userspace to access shared ranges of the file because those are allowed to be faulted in from guest_mem. Shared to private conversion unmaps the file ranges from userspace VMM pagetables.
4) Granularity of userspace pagetable mappings for shared ranges will have to be dictated by KVM guest_mem file implementation.

Caveat here is that once private pages are mapped into userspace view.

Benefits here:
1) Userspace view remains consistent while still being able to use HVA ranges
2) It would be possible to use HVA based APIs from userspace to do things like binding.
3) Double allocation wouldn't be a concern since hva ranges and gpa ranges possibly map to the same HPA ranges.

Quoted 9 lines

Code is available here if folks want to take a look before any kind of formal posting:

https://github.com/sean-jc/linux.git x86/kvm_gmem_solo

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/20230418-anfallen-irdisch-6993a61be10b@brauner
[3] https://lore.kernel.org/linux-mm/[email protected]

Quoted 8 lines · Sean Christopherson

...
cold. I poked around a bit to see how we could avoid reinventing all of that infrastructure for fd-only memory, and the best idea I could come up with is basically a rehash of Kirill's very original "KVM protected memory" RFC[3], i.e. allow "mapping" fd-only memory, but ensure that memory is never actually present from hardware's perspective.

I am most likely missing a lot of context here and possibly venturing into an infeasible/already shot down direction here.

But I would still like to get this discussed here before we move on.

I am wondering if it would make sense to implement restricted_mem/guest_mem file to expose both private and shared memory regions, inline with Kirill's original proposal now that the file implementation is controlled by KVM.

Thinking from userspace perspective:
1) Userspace creates guest mem files and is able to mmap them but all accesses to these files result into faults as no memory is allowed to be mapped into userspace VMM pagetables.

2) Userspace registers mmaped HVA ranges with KVM with additional KVM_MEM_PRIVATE flag
3) Userspace converts memory attributes and this memory conversion allows userspace to access shared ranges of the file because those are allowed to be faulted in from guest_mem. Shared to private conversion unmaps the file ranges from userspace VMM pagetables.
4) Granularity of userspace pagetable mappings for shared ranges will have to be dictated by KVM guest_mem file implementation.

Caveat here is that once private pages are mapped into userspace view.

Benefits here:
1) Userspace view remains consistent while still being able to use HVA ranges
2) It would be possible to use HVA based APIs from userspace to do things like binding.
3) Double allocation wouldn't be a concern since hva ranges and gpa ranges possibly map to the same HPA ranges.

Quoted 8 lines

Code is available here if folks want to take a look before any kind of formal posting:

https://github.com/sean-jc/linux.git x86/kvm_gmem_solo

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/20230418-anfallen-irdisch-6993a61be10b@brauner
[3] https://lore.kernel.org/linux-mm/[email protected]

Quoted 10 lines · Vishal Annapurve

Quoted 8 lines · Sean Christopherson

...
cold. I poked around a bit to see how we could avoid reinventing all of that infrastructure for fd-only memory, and the best idea I could come up with is basically a rehash of Kirill's very original "KVM protected memory" RFC[3], i.e. allow "mapping" fd-only memory, but ensure that memory is never actually present from hardware's perspective.

Quoted 2 lines

I am most likely missing a lot of context here and possibly venturing into an infeasible/already shot down direction here.

Both :-)

Quoted 11 lines

But I would still like to get this discussed here before we move on.

I am wondering if it would make sense to implement restricted_mem/guest_mem file to expose both private and shared memory regions, inline with Kirill's original proposal now that the file implementation is controlled by KVM.

Thinking from userspace perspective:
1) Userspace creates guest mem files and is able to mmap them but all accesses to these files result into faults as no memory is allowed to be mapped into userspace VMM pagetables.

Never mapping anything into the userspace page table is infeasible. Technically it's doable, but it'd effectively require all of the work of an fd-based approach (and probably significantly more), _and_ it'd require touching core mm code.

VMAs don't provide hva=>pfn information, they're the kernel's way of implementing the abstraction provided to userspace by mmap(), mprotect() etc. Among many other things, a VMA describes properties of what is mapped, e.g. hugetblfs versus anonymous, where memory is mapped (virtual address), how memory is mapped, e.g. RWX protections, etc. But a VMA doesn't track the physical address, that info is all managed through the userspace page tables.

To make it possible to allow userspace to mmap() but not access memory (without redoing how the kernel fundamentally manages virtual=>physical mappings), the simplest approach is to install PTEs into userspace page tables, but never mark them Present in hardware, i.e. prevent actually accessing the backing memory. This is is exactly what Kirill's series in link [3] below implemented.

Issues that led to us abandoning the "map with special !Present PTEs" approach:

- Using page tables, i.e. hardware defined structures, to track gfn=>pfn mappings is inefficient and inflexible compared to software defined structures, especially for the expected use cases for CoCo guests.

- The kernel wouldn't _easily_ be able to enforce a 1:1 page:guest association, let alone a 1:1 pfn:gfn mapping.

- Does not work for memory that isn't backed by 'struct page', e.g. if devices gain support for exposing encrypted memory regions to guests.

- Poking into the VMAs to convert memory would be likely be less performant due to using infrastructure that is much "heavier", e.g. would require taking mmap_lock for write.

In short, shoehorning this into mmap() requires fighting how the kernel works at pretty much every step, and in the end, adding e.g. fbind() is a lot easier.

Quoted 9 lines

2) Userspace registers mmaped HVA ranges with KVM with additional KVM_MEM_PRIVATE flag
3) Userspace converts memory attributes and this memory conversion allows userspace to access shared ranges of the file because those are allowed to be faulted in from guest_mem. Shared to private conversion unmaps the file ranges from userspace VMM pagetables.
4) Granularity of userspace pagetable mappings for shared ranges will have to be dictated by KVM guest_mem file implementation.

Quoted 8 lines

Caveat here is that once private pages are mapped into userspace view.

Benefits here:
1) Userspace view remains consistent while still being able to use HVA ranges
2) It would be possible to use HVA based APIs from userspace to do things like binding.
3) Double allocation wouldn't be a concern since hva ranges and gpa ranges possibly map to the same HPA ranges.

#3 isn't entirely correct. If a different process (call it "B") maps shared memory, and then the guest converts that memory from shared to private, the backing pages for the previously shared mapping will still be mapped by process B unless userspace also ensures process B also unmaps on conversion.

#3 is also a limiter. E.g. if a guest is primarly backed by 1GiB pages, keeping the 1GiB mapping is desirable if the guest converts a few KiB of memory to shared, and possibly even if the guest converts a few MiB of memory.

Quoted 9 lines

Quoted 9 lines

Code is available here if folks want to take a look before any kind of formal posting:

https://github.com/sean-jc/linux.git x86/kvm_gmem_solo

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/20230418-anfallen-irdisch-6993a61be10b@brauner
[3] https://lore.kernel.org/linux-mm/[email protected]

Quoted 23 lines · Sean Christopherson

Quoted 11 lines

But I would still like to get this discussed here before we move on.

I am wondering if it would make sense to implement restricted_mem/guest_mem file to expose both private and shared memory regions, inline with Kirill's original proposal now that the file implementation is controlled by KVM.

Thinking from userspace perspective:
1) Userspace creates guest mem files and is able to mmap them but all accesses to these files result into faults as no memory is allowed to be mapped into userspace VMM pagetables.

Never mapping anything into the userspace page table is infeasible. Technically it's doable, but it'd effectively require all of the work of an fd-based approach (and probably significantly more), _and_ it'd require touching core mm code.

VMAs don't provide hva=>pfn information, they're the kernel's way of implementing the abstraction provided to userspace by mmap(), mprotect() etc. Among many other things, a VMA describes properties of what is mapped, e.g. hugetblfs versus anonymous, where memory is mapped (virtual address), how memory is mapped, e.g. RWX protections, etc. But a VMA doesn't track the physical address, that info is all managed through the userspace page tables.

Quoted 6 lines

To make it possible to allow userspace to mmap() but not access memory (without redoing how the kernel fundamentally manages virtual=>physical mappings), the simplest approach is to install PTEs into userspace page tables, but never mark them Present in hardware, i.e. prevent actually accessing the backing memory. This is is exactly what Kirill's series in link [3] below implemented.

Maybe it's simpler to do when mmaped regions are backed with files.

I see that shmem has fault handlers for accesses to VMA regions associated with the files, In theory a file implementation can always choose to not allocate physical pages for such faults (similar to F_SEAL_FAULT_AUTOALLOCATE that was discussed earlier).

Quoted 8 lines

Issues that led to us abandoning the "map with special !Present PTEs" approach:

- Using page tables, i.e. hardware defined structures, to track gfn=>pfn mappings is inefficient and inflexible compared to software defined structures, especially for the expected use cases for CoCo guests.

- The kernel wouldn't _easily_ be able to enforce a 1:1 page:guest association, let alone a 1:1 pfn:gfn mapping.

Maybe KVM can ensure that each page of the guest_mem file is associated with a single memslot.

HVAs when they are registered can be associated with offsets into guest_mem files.

Quoted 6 lines

- Does not work for memory that isn't backed by 'struct page', e.g. if devices gain support for exposing encrypted memory regions to guests.

- Poking into the VMAs to convert memory would be likely be less performant due to using infrastructure that is much "heavier", e.g. would require taking mmap_lock for write.

Converting memory doesn't necessarily need to poke holes into VMA, but rather just unmap pagetables just like what would happen when mmapped files are punched to free the backing file offsets.

Quoted 21 lines

In short, shoehorning this into mmap() requires fighting how the kernel works at pretty much every step, and in the end, adding e.g. fbind() is a lot easier.

Quoted 17 lines

2) Userspace registers mmaped HVA ranges with KVM with additional KVM_MEM_PRIVATE flag
3) Userspace converts memory attributes and this memory conversion allows userspace to access shared ranges of the file because those are allowed to be faulted in from guest_mem. Shared to private conversion unmaps the file ranges from userspace VMM pagetables.
4) Granularity of userspace pagetable mappings for shared ranges will have to be dictated by KVM guest_mem file implementation.

Caveat here is that once private pages are mapped into userspace view.

Benefits here:
1) Userspace view remains consistent while still being able to use HVA ranges
2) It would be possible to use HVA based APIs from userspace to do things like binding.
3) Double allocation wouldn't be a concern since hva ranges and gpa ranges possibly map to the same HPA ranges.

Quoted 5 lines

#3 isn't entirely correct. If a different process (call it "B") maps shared memory, and then the guest converts that memory from shared to private, the backing pages for the previously shared mapping will still be mapped by process B unless userspace also ensures process B also unmaps on conversion.

This should be ideally handled by something like: unmap_mapping_range()

Quoted 3 lines

#3 is also a limiter. E.g. if a guest is primarly backed by 1GiB pages, keeping the 1GiB mapping is desirable if the guest converts a few KiB of memory to shared, and possibly even if the guest converts a few MiB of memory.

This caveat maybe can be lived with as shared ranges most likely will not be backed by 1G pages anyways, possibly causing IO performance to get hit. This possibly needs more discussion about conversion granularity used by guests.

...
Ah, you're effectively suggesting a hybrid model where the file is the single source of truth for what's private versus shared, ad KVM gets pfns through direct communication with the backing store via the file descriptor, but userspace can still control things via mmap() and friends.

If you're not suggesting a backdoor, i.e. KVM still gets private pfns via hvas, then we're back at Kirill's series, because otherwise there's no easy way for KVM to retrieve the pfn.

A form of this was also discussed, though I don't know how much of the discussion happened on-list.

KVM actually does something like this for s390's Ultravisor (UV), which is quite a bit like TDX (UV is a trusted intermediary) except that it handles faults much, much more gracefully. Specifically, when the untrusted host attempts to access a secure page, a fault occurs and the kernel responds by telling UV to export the page. The fault is gracefully handled even even for kernel accesses (see do_secure_storage_access()). The kernel does BUG() if the export fails when handling fault from kernel context, but my understanding is that export can fail if and only if there's a fatal error elsewhere, i.e. the UV essentialy _ensures_ success, and goes straight to BUG()/panic() if something goes wrong.

On the guest side, accesses to exported (swapped) secure pages generate intercepts and KVM faults in the page. To do so, KVM freezes the page/folio refcount, tells the UV to import the page, and then unfreezes the page/folio. But very crucially, when _anything_ in the untrusted host attempts to access the secure page, the above fault handling for untrusted host accesses kicks in. In other words, the guest can cause thrash, but can't bring down the host.

TDX on the other hand silently poisons memory, i.e. doesn't even generate a synchronous fault. Thus the kernel needs to be 100% perfect on preventing _any_ accesses to private memory from the host, and doing that is non-trivial and invasive.

SNP does synchronously fault, but the automatically converting in the #PF handler got NAK'd[*] for good reasons, e.g. SNP doesn't guarantee conversion success as the guest can trigger concurrent RMP modifications. So the end result ends up being the same as TDX, host accesses need to be completely prevented.

Again, this is all doable, but costly. And IMO, provides very little value.

Allowing things like mbind() is nice-to-have at best, as implementing fbind() isn't straightforward and arguably valuable to have irrespective of this discussion, e.g. to allow userspace to say "use this policy regardless of what process maps the file".

Using a common memory pool (same physical page is used for both shared and private) is a similar story. There are plenty of existing controls to limit userspace/guest memory usage and to deal with OOM scenarios, so barring egregious host accounting and/or limiting bugs, which would affect _all_ VM types, the worst case scenario is that a VM is terminated because host userspace is buggy. On the slip side, using a common pool brings complexity into the kernel, as backing stores would need to be taught to deny access to a subset of pages in their mappings, and in multiple paths, e.g. faults, read()/write() and similar, page migration, swap, etc.

[*] https://lore.kernel.org/linux-mm/[email protected]

Quoted 9 lines

Quoted 8 lines

Issues that led to us abandoning the "map with special !Present PTEs" approach:

- Using page tables, i.e. hardware defined structures, to track gfn=>pfn mappings is inefficient and inflexible compared to software defined structures, especially for the expected use cases for CoCo guests.

- The kernel wouldn't _easily_ be able to enforce a 1:1 page:guest association, let alone a 1:1 pfn:gfn mapping.

Quoted 2 lines

Maybe KVM can ensure that each page of the guest_mem file is associated with a single memslot.

This is a hard NAK. Guest physical address space is guaranteed to have holes and/or be discontiguous, for the PCI hole at the top of lower memory. Allowing only a single binding would prevent userspace from backing all (or large chunks) of guest memory with a single file.

...
That'd work for the hybrid model (fd backdoor with pseudo mmap() support), but not for a generic VMA-based implementation. If the file isn't the single source of truth, then forcing all mappings to go away simply can't work.

Quoted 8 lines

Quoted 3 lines

#3 is also a limiter. E.g. if a guest is primarly backed by 1GiB pages, keeping the 1GiB mapping is desirable if the guest converts a few KiB of memory to shared, and possibly even if the guest converts a few MiB of memory.

This caveat maybe can be lived with as shared ranges most likely will not be backed by 1G pages anyways, possibly causing IO performance to get hit. This possibly needs more discussion about conversion granularity used by guests.

Yes, it's not the end of the world. My point is that separating shared and private memory provides more flexibility. Maybe that flexibility never ends up being super important, but at the same time we shouldn't willingly paint ourselves into a corner.

Code is available here if folks want to take a look before any kind of formal posting:

https://github.com/sean-jc/linux.git x86/kvm_gmem_solo

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/20230418-anfallen-irdisch-6993a61be10b@brauner
[3] https://lore.kernel.org/linux-mm/[email protected]

Quoted 5 lines · Sean Christopherson

Code is available here if folks want to take a look before any kind of formal posting:

https://github.com/sean-jc/linux.git x86/kvm_gmem_solo

Hi Sean,

I've been working on getting the SNP patches ported to this but I'm having some trouble working out a reasonable scheme for how to work the RMPUPDATE hooks into the proposed design.

One of the main things is kvm_gmem_punch_hole(): this is can free pages back to the host whenever userspace feels like it. Pages that are still marked private in the RMP table will blow up the host if they aren't returned to the normal state before handing them back to the kernel. So I'm trying to add a hook, orchestrated by kvm_arch_gmem_invalidate(), to handle that, e.g.:

  static long kvm_gmem_punch_hole(struct file *file, int mode, loff_t offset,
                                  loff_t len)
  {
          struct kvm_gmem *gmem = file->private_data;
          pgoff_t start = offset >> PAGE_SHIFT;
          pgoff_t end = (offset + len) >> PAGE_SHIFT;
          struct kvm *kvm = gmem->kvm;

          /*
           * Bindings must stable across invalidation to ensure the start+end
           * are balanced.
           */
          filemap_invalidate_lock(file->f_mapping);
          kvm_gmem_invalidate_begin(kvm, gmem, start, end);
  
          /* Handle arch-specific cleanups before releasing pages */
          kvm_arch_gmem_invalidate(kvm, gmem, start, end);
          truncate_inode_pages_range(file->f_mapping, offset, offset + len);
  
          kvm_gmem_invalidate_end(kvm, gmem, start, end);
          filemap_invalidate_unlock(file->f_mapping);
  
          return 0;

}

But there's another hook, kvm_arch_gmem_set_mem_attributes(), needed to put the page in its intended state in the RMP table prior to mapping it into the guest's NPT.

Currently I'm calling that hook via kvm_vm_ioctl_set_mem_attributes(), just after kvm->mem_attr_array is updated based on the ioctl. The reasoning there is that KVM MMU can then rely on the existing mmu_invalidate_seq logic to ensure both the state in the mem_attr_array and the RMP table are in sync and up-to-date once MMU lock is acquired and MMU is ready to map it, or retry #NPF otherwise.

But for kvm_gmem_punch_hole(), kvm_vm_ioctl_set_mem_attributes() can potentially result in something like the following sequence if I implement things as above:

CPU0: kvm_gmem_punch_hole():
kvm_gmem_invalidate_begin()
kvm_arch_gmem_invalidate() // set pages to default/shared state in RMP table before free'ing
CPU1: kvm_vm_ioctl_set_mem_attributes():
kvm_arch_gmem_set_mem_attributes() // maliciously set pages to private in RMP table
CPU0: truncate_inode_pages_range() // HOST BLOWS UP TOUCHING PRIVATE PAGES
kvm_arch_gmem_invalidate_end()

One quick and lazy solution is to rely on the fact that kvm_vm_ioctl_set_mem_attributes() holds the kvm->slots_lock throughout the entire begin()/end() portion of the invalidation sequence, and to similarly hold the kvm->slots_lock throughout the begin()/end() sequence in kvm_gmem_punch_hole() to prevent any interleaving.

But I'd imagine overloading kvm->slots_lock is not the proper approach. But would introducing a similar mutex to keep these operations grouped/atomic be a reasonable approach to you, or should we be doing something else entirely here?

Keep in mind that RMP updates can't be done while holding KVM->mmu_lock spinlock, because we also need to unmap pages from the directmap, which can lead to scheduling-while-atomic BUG()s[1], so that's another constraint we need to work around.

Thanks!

-Mike

[1] https://lore.kernel.org/linux-coco/[email protected]/T/#m45a1af063aa5ac0b9314d6a7d46eecb1253bba7a

Quoted 4 lines

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/20230418-anfallen-irdisch-6993a61be10b@brauner
[3] https://lore.kernel.org/linux-mm/[email protected]

Quoted 43 lines · Michael Roth

Quoted 5 lines · Sean Christopherson

Code is available here if folks want to take a look before any kind of formal posting:

https://github.com/sean-jc/linux.git x86/kvm_gmem_solo

Hi Sean,

I've been working on getting the SNP patches ported to this but I'm having some trouble working out a reasonable scheme for how to work the RMPUPDATE hooks into the proposed design.

One of the main things is kvm_gmem_punch_hole(): this is can free pages back to the host whenever userspace feels like it. Pages that are still marked private in the RMP table will blow up the host if they aren't returned to the normal state before handing them back to the kernel. So I'm trying to add a hook, orchestrated by kvm_arch_gmem_invalidate(), to handle that, e.g.:

  static long kvm_gmem_punch_hole(struct file *file, int mode, loff_t offset,
                                  loff_t len)
  {
          struct kvm_gmem *gmem = file->private_data;
          pgoff_t start = offset >> PAGE_SHIFT;
          pgoff_t end = (offset + len) >> PAGE_SHIFT;
          struct kvm *kvm = gmem->kvm;

          /*
           * Bindings must stable across invalidation to ensure the start+end
           * are balanced.
           */
          filemap_invalidate_lock(file->f_mapping);
          kvm_gmem_invalidate_begin(kvm, gmem, start, end);
  
          /* Handle arch-specific cleanups before releasing pages */
          kvm_arch_gmem_invalidate(kvm, gmem, start, end);
          truncate_inode_pages_range(file->f_mapping, offset, offset + len);
  
          kvm_gmem_invalidate_end(kvm, gmem, start, end);
          filemap_invalidate_unlock(file->f_mapping);
  
          return 0;

Quoted 5 lines

}

But there's another hook, kvm_arch_gmem_set_mem_attributes(), needed to put the page in its intended state in the RMP table prior to mapping it into the guest's NPT.

IMO, this approach is wrong. kvm->mem_attr_array is the source of truth for whether userspace wants _guest_ physical pages mapped private vs. shared, but the attributes array has zero insight into the _host_ physical pages. I.e. SNP shouldn't hook kvm_mem_attrs_changed(), because operating on the RMP from that code is fundamentally wrong.

A good analogy is moving a memslot (ignoring that AFAIK no VMM actually moves memslots, but it's a good analogy for KVM internals). KVM needs to zap all mappings for the old memslot gfn, but KVM does not create mappings for the new memslot gfn. Same for changing attributes; unmap, but never map.

As for the unmapping side of things, kvm_unmap_gfn_range() will unmap all relevant NPT entries, and the elevated mmu_invalidate_in_progress will prevent KVM from establishing a new NPT mapping. And mmu_invalidate_in_progress will reach '0' only after both truncation _and_ kvm_vm_ioctl_set_mem_attributes() complete, i.e. KVM can create new mappings only when both kvm->mem_attr_array and any relevant guest_mem bindings have reached steady state.

That leaves the question of when/where to do RMP updates. Off the cuff, I think RMP updates (and I _think_ also TDX page conversions) should _always_ be done in the context of either (a) file truncation (make host owned due, a.k.a. TDX reclaim) or (b) allocating a new page/folio in guest_mem, a.k.a. kvm_gmem_get_folio(). Under the hood, even though the gfn is the same, the backing pfn is different, i.e. installing a shared mapping should _never_ need to touch the RMP because pages common from the normal (non-guest_mem) pool must already be host owned.

Quoted 5 lines

Keep in mind that RMP updates can't be done while holding KVM->mmu_lock spinlock, because we also need to unmap pages from the directmap, which can lead to scheduling-while-atomic BUG()s[1], so that's another constraint we need to work around.

Quoted 11 lines

Thanks!

-Mike

[1] https://lore.kernel.org/linux-coco/[email protected]/T/#m45a1af063aa5ac0b9314d6a7d46eecb1253bba7a

Quoted 4 lines

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/20230418-anfallen-irdisch-6993a61be10b@brauner
[3] https://lore.kernel.org/linux-mm/[email protected]

Quoted 1 line · Sean Christopherson

On Thu, May 11, 2023, Michael Roth wrote:

I put together a tree with some fixups that are needed for against the kvm_gmem_solo base tree, and a set of hooks to handle invalidations, preparing the initial private state as suggested above, and a platform-configurable mask that the x86 MMU code can use for determining whether a fault is for private vs. shared pages.

KVM: x86: Determine shared/private faults using a configurable mask ^ for TDX we could trivially add an inverted analogue of the mask/logic
KVM: x86: Use full 64-bit error code for kvm_mmu_do_page_fault
KVM: x86: Add platform hooks for private memory invalidations

KVM: x86: Add platform hook for initializing private memory

*fixup (kvm_gmem_solo): KVM: Fix end range calculation for MMU invalidations

Quoted 8 lines · Michael Roth

Quoted 1 line · Sean Christopherson

On Thu, May 11, 2023, Michael Roth wrote:

I put together a tree with some fixups that are needed for against the kvm_gmem_solo base tree, and a set of hooks to handle invalidations, preparing the initial private state as suggested above, and a platform-configurable mask that the x86 MMU code can use for determining whether a fault is for private vs. shared pages.

Quoted 4 lines

KVM: x86: Determine shared/private faults using a configurable mask ^ for TDX we could trivially add an inverted analogue of the mask/logic
KVM: x86: Use full 64-bit error code for kvm_mmu_do_page_fault
KVM: x86: Add platform hooks for private memory invalidations

Hrm, I'd prefer to avoid adding another hook for this case, arch code already has a "hook" in the form of kvm_unmap_gfn_range(). We'd probably just need a kvm_gfn_range.is_private flag to communicate to arch/vendor code that the memory being zapped is private.

That'd leave a gap for the unbind() case because kvm_unmap_gfn_range() is invoked if and only if there's an overlapping memslot. I'll chew on that a bit to see if there's a way to cleanly handle that case without another hook. I think it's worth mapping out exactly what we want unbind() to look like anyways, e.g. right now the code subtly relies on private memslots being immutable.

Quoted 2 lines

KVM: x86: Add platform hook for initializing private memory

This should also be unnecessary. The call to kvm_gmem_get_pfn() is from arch code, KVM just needs to ensure the RMP is converted before acquiring mmu_lock, e.g. KVM has all the necessary info in kvm_tdp_mmu_page_fault().

The only reason to add another arch hook would be if we wanted to converted the RMP when _allocating_, e.g. to preconvert in response to fallocate() instead of waiting until #NPF. But I think I would rather add a generic ioctl() to allow userspace to effectively prefault guest memory, e.g. to setup the RMP before running a vCPU. Such an ioctl() would potentially be useful in other scenarios, e.g. on the dest during live migration to reduce jitter.

Quoted 2 lines

*fixup (kvm_gmem_solo): KVM: Fix end range calculation for MMU invalidations

There was another bug in this path. The math for handling a non-zero offsets into the file was wrong. The code now looks like:

	xa_for_each_range(&gmem->bindings, index, slot, start, end - 1) {
		struct kvm_gfn_range gfn_range = {
			.start = slot->base_gfn + start - slot->gmem.index,

			.end = slot->base_gfn + min(end - slot->gmem.index, slot->npages),
			.slot = slot,
			.pte = __pte(0),
			.may_block = true,
		};

		if (WARN_ON_ONCE(start < slot->gmem.index ||
				 end > slot->gmem.index + slot->npages))
			continue;

		kvm_mmu_invalidate_range_add(kvm, gfn_range.start, gfn_range.end);

		flush |= kvm_unmap_gfn_range(kvm, &gfn_range);
	}

Quoted 14 lines · Sean Christopherson

Quoted 12 lines · Michael Roth

Quoted 1 line · Sean Christopherson

On Thu, May 11, 2023, Michael Roth wrote:

I put together a tree with some fixups that are needed for against the kvm_gmem_solo base tree, and a set of hooks to handle invalidations, preparing the initial private state as suggested above, and a platform-configurable mask that the x86 MMU code can use for determining whether a fault is for private vs. shared pages.

KVM: x86: Determine shared/private faults using a configurable mask ^ for TDX we could trivially add an inverted analogue of the mask/logic
KVM: x86: Use full 64-bit error code for kvm_mmu_do_page_fault
KVM: x86: Add platform hooks for private memory invalidations

Quoted 4 lines

Hrm, I'd prefer to avoid adding another hook for this case, arch code already has a "hook" in the form of kvm_unmap_gfn_range(). We'd probably just need a kvm_gfn_range.is_private flag to communicate to arch/vendor code that the memory being zapped is private.

kvm_unmap_gfn_range() does however get called with kvm->mmu_lock held so it might be tricky to tie RMP updates into that path.

Quoted 5 lines

That'd leave a gap for the unbind() case because kvm_unmap_gfn_range() is invoked if and only if there's an overlapping memslot. I'll chew on that a bit to see if there's a way to cleanly handle that case without another hook. I think it's worth mapping out exactly what we want unbind() to look like anyways, e.g. right now the code subtly relies on private memslots being immutable.

I thought the direction you sort of driving at was to completely decouple RMP updates for physical pages from the KVM MMU map/unmap paths since the life-cycles of those backing pages and associated RMP state are somewhat separate from the state of the GFNs and kvm->mem_attr_array. It seems to make sense when dealing with things like this unbind() case.

There's also cases like userspaces that opt to not discard memory after conversions because they highly favor performance over memory usage. In those cases it would make sense to defer marking the pages as shared in the RMP until the FALLOC_FL_PUNCH_HOLE, rather than triggering it via KVM MMU invalidation path after a conversion.

Quoted 5 lines

Quoted 1 line

KVM: x86: Add platform hook for initializing private memory

This should also be unnecessary. The call to kvm_gmem_get_pfn() is from arch code, KVM just needs to ensure the RMP is converted before acquiring mmu_lock, e.g. KVM has all the necessary info in kvm_tdp_mmu_page_fault().

I think that approach would work fine. The way I was thinking of things is that KVM MMU would necessarily call kvm_gmem_get_pfn() to grab the page before mapping it into the guest, so moving it out into an explicit call should work just as well. That would also drop the need for the __kvm_gmem_get_pfn() stuff I needed to add for the initial case where we need to access the PFN prior to making it private.

Quoted 7 lines

The only reason to add another arch hook would be if we wanted to converted the RMP when _allocating_, e.g. to preconvert in response to fallocate() instead of waiting until #NPF. But I think I would rather add a generic ioctl() to allow userspace to effectively prefault guest memory, e.g. to setup the RMP before running a vCPU. Such an ioctl() would potentially be useful in other scenarios, e.g. on the dest during live migration to reduce jitter.

Agreed, deferring the RMPUPDATE until it's actually needed would give us more flexibility on optimizing for things like lazy-acceptance.

For less-common scenarios like preallocation it makes sense to make that an opt-in sort of thing for userspace to configure explicitly.

Quoted 9 lines

Quoted 2 lines

*fixup (kvm_gmem_solo): KVM: Fix end range calculation for MMU invalidations

There was another bug in this path. The math for handling a non-zero offsets into the file was wrong. The code now looks like:

	xa_for_each_range(&gmem->bindings, index, slot, start, end - 1) {
		struct kvm_gfn_range gfn_range = {
			.start = slot->base_gfn + start - slot->gmem.index,

Sorry if I'm missing something here, but isn't there a risk that:

start - slot->gmem.index

would be less than zero? E.g. starting GFN was 0, but current slot is bound at some non-zero offset in the same gmem instance. I guess the warning below shouldn't caught that, but it seems like a real scenario.

Since 'index' corresponds to the gmem offset of the current slot, is there any reason not to do something like this?:

.start = slot->base_gfn + index - slot->gmem.index,

But then, if that's the case, wouldn't index == slot->gmem.index? Suggesting we case just simplify to this?:

.start = slot->base_gfn,

I've ported selftests from Chao and I [1] while working on hugetlb support for guest_mem [2].

In the process, I found some bugs and have some suggestions for guest_mem. Please see separate commits at [3].

Here are some highlights/questions:

+ "KVM: guest_mem: Explain the use of the uptodate flag for gmem"
+ Generally, uptodate flags means that the contents of this page match the backing store. Since gmem is memory-backed, does "uptodate" for gmem mean "zeroed"?

+ "KVM: guest_mem: Don't re-mark accessed after getting a folio" and "KVM:
guest_mem: Don't set dirty flag for folio"
+ Do we need to folio_mark_accessed(), when it was created with FGP_ACCESSED?

+ What is the significance of these LRU flags when gmem doesn't support swapping/eviction?

+ "KVM: guest_mem: Align so that at least 1 page is allocated"
+ Bug in current implementation: without this alignment, fallocate() of a size less than the gmem page size will result in no allocation at all

+ Both shmem and hugetlbfs perform this alignment
+ "KVM: guest_mem: Add alignment checks"
+ Implemented the alignment checks for guest_mem because hugetlb on gmem would hit a BUG_ON without this check
+ "KVM: guest_mem: Prevent overflows in kvm_gmem_invalidate_begin()"
+ Sean fixed a bug in the offset-to-gfn conversion in kvm_gmem_invalidate_begin() earlier, adding a WARN_ON_ONCE()

+ Code will always hit WARN_ON_ONCE() when the entire file is closed and all offsets are invalidated, so WARN_ON_ONCE() should be removed
+ Vishal noticed that the conversion might result in an overflow, so I fixed that
+ And of course, hugetlb support! Please let me know what you think of the approach proposed at [2].

[1] https://lore.kernel.org/all/[email protected]/T/
[2] https://lore.kernel.org/lkml/[email protected]/T/
[3] https://github.com/googleprodkernel/linux-cc/tree/gmem-hugetlb-rfc-v1

...

Probably not. And as you note below, it's all pretty nonsensical anyways.

Quoted 3 lines

+ What is the significance of these LRU flags when gmem doesn't
support
swapping/eviction?

Likely none. I used the filemap APIs in my POC because it was easy, not because it was necessarily the best approach, i.e. that the folios/pages show up in the LRUs is an unwanted side effect, not a feature. If guest_memfd only needs a small subset of the filemap support, going with a true from-scratch implemenation on top of xarray might be cleaner overall, e.g. would avoid the need for a new flag to say "this folio can't be migrated even though it's on the LRUs".

Quoted 5 lines

+ "KVM: guest_mem: Align so that at least 1 page is allocated"
+ Bug in current implementation: without this alignment, fallocate()
of
a size less than the gmem page size will result in no allocation
at all

I'm not convinced this is a bug. I don't see any reason to allow allocating and punching holes in sub-page granularity.

Quoted 8 lines

+ Both shmem and hugetlbfs perform this alignment
+ "KVM: guest_mem: Add alignment checks"
+ Implemented the alignment checks for guest_mem because hugetlb on
gmem
would hit a BUG_ON without this check
+ "KVM: guest_mem: Prevent overflows in kvm_gmem_invalidate_begin()"
+ Sean fixed a bug in the offset-to-gfn conversion in kvm_gmem_invalidate_begin() earlier, adding a WARN_ON_ONCE()

As Mike pointed out, there's likely still a bug here[*]. I was planning on diving into that last week, but that never happened. If you or anyone else can take a peek and/or write a testcase, that would be awesome.

: Heh, only if there's a testcase for it. Assuming start >= the slot
offset does
: seem broken, e.g. if the range-to-invalidate overlaps multiple slots,
later slots
: will have index==slot->gmem.index > start.
:
: > Since 'index' corresponds to the gmem offset of the current slot, is
there any
: > reason not to do something like this?:
: >
: > .start = slot->base_gfn + index - slot->gmem.index,
: >
: > But then, if that's the case, wouldn't index == slot->gmem.index?
Suggesting
: > we case just simplify to this?:
: >
: > .start = slot->base_gfn,
:
: No, e.g. if start is partway through a memslot, there's no need to
invalidate
: the entire memslot. I'll stare at this tomorrow when my brain is
hopefully a
: bit more functional, I suspect there is a min() and/or max() needed
somewhere.

[*] https://lore.kernel.org/all/[email protected]

Quoted 1 line

...

Quoted 9 lines · David Hildenbrand

Quoted 1 line · Sean Christopherson

I want to start referring to the code/patches by its

syscall/implementation name

Quoted 1 line

instead of "UPM", as "UPM" is (a) very KVM centric, (b) refers to the

broader effort

Quoted 1 line

and not just the non-KVM code, and (c) will likely be confusing for

future reviewers

Quoted 2 lines

since there's nothing in the code that mentions "UPM" in any way.

Quoted 7 lines

Quoted 1 line

But typing out restrictedmem is quite tedious, and git grep shows

that "rmem" is

Quoted 3 lines

already used to refer to "reserved memory".

Renaming the syscall to "guardedmem"...

restrictedmem, guardedmem, ... all fairly "suboptimal" if you'd ask me ...

I'm definitely open to other suggestions, but I suspect it's going to be difficult to be more precise than something like "guarded".

E.g. we discussed "unmappable" at one point, but the memory can still be mapped, just not via mmap(). And it's not just about mappings, e.g. read() and its many variants are all disallowed too, despite the kernel direct map still being live (modulo SNP requirements).

Quoted 4 lines · Sean Christopherson

Quoted 3 lines · David Hildenbrand

Quoted 1 line · Sean Christopherson

I want to start referring to the code/patches by its

syscall/implementation name

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

instead of "UPM", as "UPM" is (a) very KVM centric, (b) refers to

the broader effort

Quoted 1 line

and not just the non-KVM code, and (c) will likely be confusing

for future reviewers

Quoted 3 lines

since there's nothing in the code that mentions "UPM" in any way.

But typing out restrictedmem is quite tedious, and git grep shows

that "rmem" is

Quoted 11 lines

Quoted 6 lines

Quoted 3 lines

already used to refer to "reserved memory".

Renaming the syscall to "guardedmem"...

restrictedmem, guardedmem, ... all fairly "suboptimal" if you'd ask me ...

I'm definitely open to other suggestions, but I suspect it's going to be difficult
to be more precise than something like "guarded".

Quoted 7 lines

E.g. we discussed "unmappable" at one point, but the memory can still be mapped,
just not via mmap(). And it's not just about mappings, e.g. read() and its many
variants are all disallowed too, despite the kernel direct map still being live
(modulo SNP requirements).

I'm for renaming the concept because restrictedmem is quite a mouthful. :)

How about "concealedmem" or "obscuredmem" to highlight the idea of this memory being hidden/unreadable/unmappable from userspace?

Guarded is better than restricted but doesn't really highlight how/in what way it is being guarded.

Quoted 9 lines · David Hildenbrand

Quoted 1 line · Sean Christopherson

I want to start referring to the code/patches by its

syscall/implementation name

Quoted 1 line

instead of "UPM", as "UPM" is (a) very KVM centric, (b) refers to the

broader effort

Quoted 1 line

and not just the non-KVM code, and (c) will likely be confusing for

future reviewers

Quoted 2 lines

since there's nothing in the code that mentions "UPM" in any way.

Quoted 7 lines

Quoted 1 line

But typing out restrictedmem is quite tedious, and git grep shows

that "rmem" is

Quoted 3 lines

already used to refer to "reserved memory".

Renaming the syscall to "guardedmem"...

restrictedmem, guardedmem, ... all fairly "suboptimal" if you'd ask me ...

I'm definitely open to other suggestions, but I suspect it's going to be difficult to be more precise than something like "guarded".

E.g. we discussed "unmappable" at one point, but the memory can still be mapped, just not via mmap(). And it's not just about mappings, e.g. read() and its many variants are all disallowed too, despite the kernel direct map still being live (modulo SNP requirements).

Quoted 1 line · Sean Christopherson

What do y'all think about renaming "restrictedmem" to "guardedmem"?

Yeay, let's add more confusion :D

If we're at renaming, I'd appreciate if we could find a terminology that does look/sound less horrible.

Quoted 6 lines

I want to start referring to the code/patches by its syscall/implementation name instead of "UPM", as "UPM" is (a) very KVM centric, (b) refers to the broader effort and not just the non-KVM code, and (c) will likely be confusing for future reviewers since there's nothing in the code that mentions "UPM" in any way.

Quoted 4 lines

But typing out restrictedmem is quite tedious, and git grep shows that "rmem" is already used to refer to "reserved memory".

Renaming the syscall to "guardedmem"...

restrictedmem, guardedmem, ... all fairly "suboptimal" if you'd ask me ...

Quoted 17 lines · David Hildenbrand

Quoted 1 line · Sean Christopherson

What do y'all think about renaming "restrictedmem" to "guardedmem"?

Yeay, let's add more confusion :D

If we're at renaming, I'd appreciate if we could find a terminology that does look/sound less horrible.

Quoted 9 lines

I want to start referring to the code/patches by its syscall/implementation name instead of "UPM", as "UPM" is (a) very KVM centric, (b) refers to the broader effort and not just the non-KVM code, and (c) will likely be confusing for future reviewers since there's nothing in the code that mentions "UPM" in any way.

But typing out restrictedmem is quite tedious, and git grep shows that "rmem" is already used to refer to "reserved memory".

Quoted 3 lines

Quoted 1 line

Renaming the syscall to "guardedmem"...

restrictedmem, guardedmem, ... all fairly "suboptimal" if you'd ask me ...

In the world of TEE's and confidential computing it is fairly common to call memory areas enclaves, even outside SGX context. So in that sense enclave memory would be the most correct terminology.

Quoted 22 lines · Jarkko Sakkinen

Quoted 20 lines · David Hildenbrand

Quoted 1 line · Sean Christopherson

What do y'all think about renaming "restrictedmem" to "guardedmem"?

Yeay, let's add more confusion :D

If we're at renaming, I'd appreciate if we could find a terminology that does look/sound less horrible.

Quoted 10 lines

I want to start referring to the code/patches by its syscall/implementation name instead of "UPM", as "UPM" is (a) very KVM centric, (b) refers to the broader effort and not just the non-KVM code, and (c) will likely be confusing for future reviewers since there's nothing in the code that mentions "UPM" in any way.

But typing out restrictedmem is quite tedious, and git grep shows that "rmem" is already used to refer to "reserved memory".

Renaming the syscall to "guardedmem"...

restrictedmem, guardedmem, ... all fairly "suboptimal" if you'd ask me ...

Quoted 3 lines

In the world of TEE's and confidential computing it is fairly common to call memory areas enclaves, even outside SGX context. So in that sense enclave memory would be the most correct terminology.

I was also thinking along the lines of isolated_mem or imem ... essentially, isolated from (unprivileged) user space.

... if we still want to have a common syscall for it.

Quoted 27 lines · David Hildenbrand

Quoted 25 lines · Jarkko Sakkinen

Quoted 14 lines · David Hildenbrand

Quoted 1 line · Sean Christopherson

What do y'all think about renaming "restrictedmem" to "guardedmem"?

Yeay, let's add more confusion :D

If we're at renaming, I'd appreciate if we could find a terminology that does look/sound less horrible.

Quoted 6 lines

I want to start referring to the code/patches by its syscall/implementation name instead of "UPM", as "UPM" is (a) very KVM centric, (b) refers to the broader effort and not just the non-KVM code, and (c) will likely be confusing for future reviewers since there's nothing in the code that mentions "UPM" in any way.

Quoted 6 lines

Quoted 4 lines

But typing out restrictedmem is quite tedious, and git grep shows that "rmem" is already used to refer to "reserved memory".

Renaming the syscall to "guardedmem"...

restrictedmem, guardedmem, ... all fairly "suboptimal" if you'd ask me ...

In the world of TEE's and confidential computing it is fairly common to call memory areas enclaves, even outside SGX context. So in that sense enclave memory would be the most correct terminology.

Quoted 4 lines

I was also thinking along the lines of isolated_mem or imem ... essentially, isolated from (unprivileged) user space.

... if we still want to have a common syscall for it.

I'm fan of the ioctl, if it has a chance of working out.

What do y'all think about renaming "restrictedmem" to "guardedmem"?

I want to start referring to the code/patches by its syscall/implementation name instead of "UPM", as "UPM" is (a) very KVM centric, (b) refers to the broader effort and not just the non-KVM code, and (c) will likely be confusing for future reviewers since there's nothing in the code that mentions "UPM" in any way.

But typing out restrictedmem is quite tedious, and git grep shows that "rmem" is already used to refer to "reserved memory".

Renaming the syscall to "guardedmem"...

1. Allows for a shorthand and namespace, "gmem", that isn't already in use by the kernel (see "reserved memory above").

2. Provides a stronger hint as to its purpose. "Restricted" conveys that the allocated memory is limited in some way, but doesn't capture how the memory is restricted, e.g. "restricted" could just as easily mean that the allocation can be restricted to certain types of backing stores or something. "Guarded" on the other hand captures that the memory has extra defenses of some form.

3. Is shorter to type and speak. Work smart, not hard :-)

4. Isn't totally wrong for the KVM use case if someone assumes the "g" means "guest" when reading mail and whatnot.

P.S. I trimmed the Cc/To substantially for this particular discussion to avoid
spamming folks that don't (yet) care about this stuff with another potentially lengthy thread. Feel free to add (back) any people/lists.

By default, the backing shmem file for a restrictedmem fd is created on shmem's kernel space mount.

With this patch, an optional tmpfs mount can be specified via an fd, which will be used as the mountpoint for backing the shmem file associated with a restrictedmem fd.

This will help restrictedmem fds inherit the properties of the provided tmpfs mounts, for example, hugepage allocation hints, NUMA binding hints, etc.

Permissions for the fd passed to memfd_restricted() is modeled after the openat() syscall, since both of these allow creation of a file upon a mount/directory.

Permission to reference the mount the fd represents is checked upon fd creation by other syscalls (e.g. fsmount(), open(), or open_tree(), etc) and any process that can present memfd_restricted() with a valid fd is expected to have obtained permission to use the mount represented by the fd. This behavior is intended to parallel that of the openat() syscall.

memfd_restricted() will check that the tmpfs superblock is writable, and that the mount is also writable, before attempting to create a restrictedmem file on the mount.

Signed-off-by Ackerley Tng <[email protected]>

---
  include/linux/syscalls.h           |  2 +-
  include/uapi/linux/restrictedmem.h |  8 ++++
  mm/restrictedmem.c                 | 74 +++++++++++++++++++++++++++---
  3 files changed, 77 insertions(+), 7 deletions(-)

create mode 100644 include/uapi/linux/restrictedmem.h

include/linux/syscalls.h +1 -1

index f9e9e0c820c5..a23c4c385cd3 100644
@@ -1056,7 +1056,7 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
  asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
  					    unsigned long home_node,
  					    unsigned long flags);
-asmlinkage long sys_memfd_restricted(unsigned int flags);
+asmlinkage long sys_memfd_restricted(unsigned int flags, int mount_fd);
 
  /*
   * Architecture-specific system calls

include/uapi/linux/restrictedmem.h new file +0 -0

new file mode 100644
index 000000000000..22d6f2285f6d

mm/restrictedmem.c +8 -5

+#include <linux/namei.h>
  #include <linux/pagemap.h>
  #include <linux/pseudo_fs.h>
  #include <linux/shmem_fs.h>
  #include <linux/syscalls.h>
  #include <uapi/linux/falloc.h>
  #include <uapi/linux/magic.h>
+#include <uapi/linux/restrictedmem.h>
  #include <linux/restrictedmem.h>
 
  struct restrictedmem {

@@ -189,19 +190,20 @@ static struct file *restrictedmem_file_create(struct file *memfd)
  	return file;
  }
 
-SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
+static int restrictedmem_create(struct vfsmount *mount)
  {
  	struct file *file, *restricted_file;
  	int fd, err;
 
-	if (flags)
-		return -EINVAL;
-
  	fd = get_unused_fd_flags(0);
  	if (fd < 0)
  		return fd;
 
-	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+	if (mount)
+		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0, VM_NORESERVE);
+	else
+		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+
  	if (IS_ERR(file)) {
  		err = PTR_ERR(file);
  		goto err_fd;

@@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
  	return err;
  }

mm/restrictedmem.c +39 -0

+
+static int restrictedmem_create_on_user_mount(int mount_fd)
+{
+	int ret;
+	struct fd f;
+	struct vfsmount *mnt;
+
+	f = fdget_raw(mount_fd);
+	if (!f.file)
+		return -EBADF;
+
+	ret = -EINVAL;
+	if (!is_mount_root(f.file))
+		goto out;
+
+	mnt = f.file->f_path.mnt;
+	if (!is_shmem_mount(mnt))
+		goto out;
+
+	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);
+	if (ret)
+		goto out;
+
+	ret = mnt_want_write(mnt);
+	if (unlikely(ret))
+		goto out;
+
+	ret = restrictedmem_create(mnt);
+
+	mnt_drop_write(mnt);
+out:
+	fdput(f);
+
+	return ret;
+}
+
+SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
+{
+	if (flags & ~RMFD_USERMNT)

Quoted 1 line · Ackerley Tng

...

include/uapi/linux/restrictedmem.h line 6

Quoted 12 lines

include/uapi/linux/restrictedmem.h new file +6 -0

new file mode 100644
index 000000000000..22d6f2285f6d
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
+#define _UAPI_LINUX_RESTRICTEDMEM_H
+
+/* flags for memfd_restricted */
+#define RMFD_USERMNT		0x0001U

I wonder if we can come up with a more expressive prefix than RMFD. Sounds more like "rm fd" ;) Maybe it should better match the "memfd_restricted" syscall name, like "MEMFD_RSTD_USERMNT".

mm/restrictedmem.c line 2

Quoted 9 lines

include/uapi/linux/restrictedmem.h new file +2 -0

+
+#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */

mm/restrictedmem.c +0 -1

index c5d869d8c2d8..f7b62364a31a 100644
@@ -1,11 +1,12 @@
   // SPDX-License-Identifier: GPL-2.0
-#include "linux/sbitmap.h"

Looks like an unrelated change?

Quoted 43 lines

mm/restrictedmem.c +8 -5

+#include <linux/namei.h>
   #include <linux/pagemap.h>
   #include <linux/pseudo_fs.h>
   #include <linux/shmem_fs.h>
   #include <linux/syscalls.h>
   #include <uapi/linux/falloc.h>
   #include <uapi/linux/magic.h>
+#include <uapi/linux/restrictedmem.h>
   #include <linux/restrictedmem.h>
 
   struct restrictedmem {

@@ -189,19 +190,20 @@ static struct file  
 *restrictedmem_file_create(struct file *memfd)
   	return file;
   }
 
-SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
+static int restrictedmem_create(struct vfsmount *mount)
   {
   	struct file *file, *restricted_file;
   	int fd, err;
 
-	if (flags)
-		return -EINVAL;
-
   	fd = get_unused_fd_flags(0);
   	if (fd < 0)
   		return fd;
 
-	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+	if (mount)
+		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0,  
 VM_NORESERVE);
+	else
+		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+
   	if (IS_ERR(file)) {

   		err = PTR_ERR(file);
   		goto err_fd;

mm/restrictedmem.c +0 -0

@@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int,  
 flags)
   	return err;
   }

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 236

Quoted 9 lines

mm/restrictedmem.c +9 -0

+static bool is_shmem_mount(struct vfsmount *mnt)
+{
+	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;
+}
+
+static bool is_mount_root(struct file *file)
+{
+	return file->f_path.dentry == file->f_path.mnt->mnt_root;
+}

I'd inline at least that function, pretty self-explaining.

Quoted 39 lines

mm/restrictedmem.c +39 -0

+
+static int restrictedmem_create_on_user_mount(int mount_fd)
+{
+	int ret;
+	struct fd f;
+	struct vfsmount *mnt;
+
+	f = fdget_raw(mount_fd);
+	if (!f.file)
+		return -EBADF;
+
+	ret = -EINVAL;
+	if (!is_mount_root(f.file))
+		goto out;
+
+	mnt = f.file->f_path.mnt;
+	if (!is_shmem_mount(mnt))
+		goto out;
+
+	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);
+	if (ret)
+		goto out;
+
+	ret = mnt_want_write(mnt);
+	if (unlikely(ret))
+		goto out;
+
+	ret = restrictedmem_create(mnt);
+
+	mnt_drop_write(mnt);
+out:
+	fdput(f);
+
+	return ret;
+}
+
+SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
+{
+	if (flags & ~RMFD_USERMNT)

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 285

Quoted 10 lines

mm/restrictedmem.c +10 -0

+		return -EINVAL;
+
+	if (flags == RMFD_USERMNT) {
+		if (mount_fd < 0)
+			return -EINVAL;
+
+		return restrictedmem_create_on_user_mount(mount_fd);
+	} else {
+		return restrictedmem_create(NULL);
+	}

You can drop the else case:

if (flags == RMFD_USERMNT) {
	...
	return restrictedmem_create_on_user_mount(mount_fd);
}

return restrictedmem_create(NULL);

I do wonder if you want to properly check for a flag instead of comparing values. Results in a more natural way to deal with flags:

if (flags & RMFD_USERMNT) {

}

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 290

Quoted 5 lines

mm/restrictedmem.c +2 -0

+}
+
   int restrictedmem_bind(struct file *file, pgoff_t start, pgoff_t end,
   		       struct restrictedmem_notifier *notifier, bool exclusive)
   {

The "memfd_restricted" vs. "restrictedmem" terminology is a bit unfortunate, but not your fault here.

I'm not a FS person, but it does look good to me.

By default, the backing shmem file for a restrictedmem fd is created on shmem's kernel space mount.

With this patch, an optional tmpfs mount can be specified via an fd, which will be used as the mountpoint for backing the shmem file associated with a restrictedmem fd.

This will help restrictedmem fds inherit the properties of the provided tmpfs mounts, for example, hugepage allocation hints, NUMA binding hints, etc.

Permissions for the fd passed to memfd_restricted() is modeled after the openat() syscall, since both of these allow creation of a file upon a mount/directory.

Permission to reference the mount the fd represents is checked upon fd creation by other syscalls (e.g. fsmount(), open(), or open_tree(), etc) and any process that can present memfd_restricted() with a valid fd is expected to have obtained permission to use the mount represented by the fd. This behavior is intended to parallel that of the openat() syscall.

memfd_restricted() will check that the tmpfs superblock is writable, and that the mount is also writable, before attempting to create a restrictedmem file on the mount.

Signed-off-by Ackerley Tng <[email protected]>

---
 include/linux/syscalls.h           |  2 +-
 include/uapi/linux/restrictedmem.h |  8 ++++
 mm/restrictedmem.c                 | 74 +++++++++++++++++++++++++++---
 3 files changed, 77 insertions(+), 7 deletions(-)
 create mode 100644 include/uapi/linux/restrictedmem.h

include/linux/syscalls.h +1 -1

index f9e9e0c820c5..a23c4c385cd3 100644
@@ -1056,7 +1056,7 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
 asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
 					    unsigned long home_node,
 					    unsigned long flags);
-asmlinkage long sys_memfd_restricted(unsigned int flags);
+asmlinkage long sys_memfd_restricted(unsigned int flags, int mount_fd);
 
 /*
  * Architecture-specific system calls

include/uapi/linux/restrictedmem.h new file +8 -0

new file mode 100644
index 000000000000..22d6f2285f6d
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
+#define _UAPI_LINUX_RESTRICTEDMEM_H
+
+/* flags for memfd_restricted */
+#define RMFD_USERMNT		0x0001U
+
+#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */

mm/restrictedmem.c +27 -6

index c5d869d8c2d8..f7b62364a31a 100644
@@ -1,11 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0
-#include "linux/sbitmap.h"
+#include <linux/namei.h>
 #include <linux/pagemap.h>
 #include <linux/pseudo_fs.h>
 #include <linux/shmem_fs.h>
 #include <linux/syscalls.h>
 #include <uapi/linux/falloc.h>
 #include <uapi/linux/magic.h>
+#include <uapi/linux/restrictedmem.h>
 #include <linux/restrictedmem.h>
 
 struct restrictedmem {

@@ -189,19 +190,20 @@ static struct file *restrictedmem_file_create(struct file *memfd)
 	return file;
 }
 
-SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
+static int restrictedmem_create(struct vfsmount *mount)
 {
 	struct file *file, *restricted_file;
 	int fd, err;
 
-	if (flags)
-		return -EINVAL;
-
 	fd = get_unused_fd_flags(0);
 	if (fd < 0)
 		return fd;
 
-	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+	if (mount)
+		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0, VM_NORESERVE);
+	else
+		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+
 	if (IS_ERR(file)) {
 		err = PTR_ERR(file);
 		goto err_fd;

@@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
 	return err;
 }
 
+static bool is_shmem_mount(struct vfsmount *mnt)
+{
+	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;
+}
+
+static bool is_mount_root(struct file *file)
+{
+	return file->f_path.dentry == file->f_path.mnt->mnt_root;
+}
+
+static int restrictedmem_create_on_user_mount(int mount_fd)
+{
+	int ret;
+	struct fd f;
+	struct vfsmount *mnt;
+
+	f = fdget_raw(mount_fd);
+	if (!f.file)
+		return -EBADF;

mm/restrictedmem.c +5 -0

+	if (ret)
+		goto out;
+
+	ret = mnt_want_write(mnt);
+	if (unlikely(ret))

mm/restrictedmem.c +4 -0

+
+SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
+{
+	if (flags & ~RMFD_USERMNT)

mm/restrictedmem.c +2 -1

+}
+
 int restrictedmem_bind(struct file *file, pgoff_t start, pgoff_t end,
 		       struct restrictedmem_notifier *notifier, bool exclusive)
 {
--

2.40.0.348.gf938b09366-goog

Quoted 15 lines · Ackerley Tng

...

mm/restrictedmem.c +13 -0

+static int restrictedmem_create_on_user_mount(int mount_fd)
+{
+	int ret;
+	struct fd f;
+	struct vfsmount *mnt;
+
+	f = fdget_raw(mount_fd);
+	if (!f.file)
+		return -EBADF;
+
+	ret = -EINVAL;
+	if (!is_mount_root(f.file))
+		goto out;

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 256

Quoted 6 lines

mm/restrictedmem.c +6 -0

+
+	mnt = f.file->f_path.mnt;
+	if (!is_shmem_mount(mnt))
+		goto out;
+
+	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);

Why MAY_EXEC?

Quoted 5 lines

mm/restrictedmem.c +5 -0

+	if (ret)
+		goto out;
+
+	ret = mnt_want_write(mnt);
+	if (unlikely(ret))

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 271

Quoted 10 lines

mm/restrictedmem.c +10 -0

+		goto out;
+
+	ret = restrictedmem_create(mnt);
+
+	mnt_drop_write(mnt);
+out:
+	fdput(f);
+
+	return ret;
+}

We need review from fs folks. Look mostly sensible, but I have no experience in fs.

Quoted 4 lines

mm/restrictedmem.c +4 -0

+
+SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
+{
+	if (flags & ~RMFD_USERMNT)

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 285

Quoted 10 lines

mm/restrictedmem.c +10 -0

+		return -EINVAL;
+
+	if (flags == RMFD_USERMNT) {
+		if (mount_fd < 0)
+			return -EINVAL;
+
+		return restrictedmem_create_on_user_mount(mount_fd);
+	} else {
+		return restrictedmem_create(NULL);
+	}

Maybe restructure with single restrictedmem_create() call?

	struct vfsmount *mnt = NULL;

	if (flags == RMFD_USERMNT) {
		...
		mnt = ...();
	}

	return restrictedmem_create(mnt);

Quoted 7 lines

mm/restrictedmem.c +2 -1

+}
+
  int restrictedmem_bind(struct file *file, pgoff_t start, pgoff_t end,
  		       struct restrictedmem_notifier *notifier, bool exclusive)
  {
--

2.40.0.348.gf938b09366-goog

By default, the backing shmem file for a restrictedmem fd is created on shmem's kernel space mount.

With this patch, an optional tmpfs mount can be specified via an fd, which will be used as the mountpoint for backing the shmem file associated with a restrictedmem fd.

This will help restrictedmem fds inherit the properties of the provided tmpfs mounts, for example, hugepage allocation hints, NUMA binding hints, etc.

Permissions for the fd passed to memfd_restricted() is modeled after the openat() syscall, since both of these allow creation of a file upon a mount/directory.

Permission to reference the mount the fd represents is checked upon fd creation by other syscalls (e.g. fsmount(), open(), or open_tree(), etc) and any process that can present memfd_restricted() with a valid fd is expected to have obtained permission to use the mount represented by the fd. This behavior is intended to parallel that of the openat() syscall.

memfd_restricted() will check that the tmpfs superblock is writable, and that the mount is also writable, before attempting to create a restrictedmem file on the mount.

Signed-off-by Ackerley Tng <[email protected]>

---
 include/linux/syscalls.h           |  2 +-
 include/uapi/linux/restrictedmem.h |  8 ++++
 mm/restrictedmem.c                 | 74 +++++++++++++++++++++++++++---
 3 files changed, 77 insertions(+), 7 deletions(-)
 create mode 100644 include/uapi/linux/restrictedmem.h

include/linux/syscalls.h +1 -1

index f9e9e0c820c5..a23c4c385cd3 100644
@@ -1056,7 +1056,7 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
 asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
 					    unsigned long home_node,
 					    unsigned long flags);
-asmlinkage long sys_memfd_restricted(unsigned int flags);
+asmlinkage long sys_memfd_restricted(unsigned int flags, int mount_fd);
 
 /*
  * Architecture-specific system calls

include/uapi/linux/restrictedmem.h new file +8 -0

new file mode 100644
index 000000000000..22d6f2285f6d
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
+#define _UAPI_LINUX_RESTRICTEDMEM_H
+
+/* flags for memfd_restricted */
+#define RMFD_USERMNT		0x0001U
+
+#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */

mm/restrictedmem.c +2 -1

index c5d869d8c2d8..f7b62364a31a 100644
@@ -1,11 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0
-#include "linux/sbitmap.h"
+#include <linux/namei.h>
 #include <linux/pagemap.h>
 #include <linux/pseudo_fs.h>
 #include <linux/shmem_fs.h>
 #include <linux/syscalls.h>
 #include <uapi/linux/falloc.h>
 #include <uapi/linux/magic.h>
+#include <uapi/linux/restrictedmem.h>
 #include <linux/restrictedmem.h>
 
 struct restrictedmem {

@@ -189,19 +190,20 @@ static struct file *restrictedmem_file_create(struct file *memfd)
 	return file;
 }
 

mm/restrictedmem.c +5 -1

 	if (fd < 0)
 		return fd;
 
-	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+	if (mount)
+		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0, VM_NORESERVE);
+	else
+		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+
 	if (IS_ERR(file)) {
 		err = PTR_ERR(file);
 		goto err_fd;

mm/restrictedmem.c +11 -0

+}
+
+static int restrictedmem_create_on_user_mount(int mount_fd)
+{
+	int ret;
+	struct fd f;
+	struct vfsmount *mnt;
+
+	f = fdget_raw(mount_fd);
+	if (!f.file)
+		return -EBADF;

mm/restrictedmem.c +12 -0

+	if (ret)
+		goto out;
+
+	ret = mnt_want_write(mnt);
+	if (unlikely(ret))
+		goto out;
+
+	ret = restrictedmem_create(mnt);
+
+	mnt_drop_write(mnt);
+out:
+	fdput(f);

Quoted 83 lines · Ackerley Tng

By default, the backing shmem file for a restrictedmem fd is created on shmem's kernel space mount.

With this patch, an optional tmpfs mount can be specified via an fd, which will be used as the mountpoint for backing the shmem file associated with a restrictedmem fd.

This will help restrictedmem fds inherit the properties of the provided tmpfs mounts, for example, hugepage allocation hints, NUMA binding hints, etc.

Permissions for the fd passed to memfd_restricted() is modeled after the openat() syscall, since both of these allow creation of a file upon a mount/directory.

Permission to reference the mount the fd represents is checked upon fd creation by other syscalls (e.g. fsmount(), open(), or open_tree(), etc) and any process that can present memfd_restricted() with a valid fd is expected to have obtained permission to use the mount represented by the fd. This behavior is intended to parallel that of the openat() syscall.

memfd_restricted() will check that the tmpfs superblock is writable, and that the mount is also writable, before attempting to create a restrictedmem file on the mount.

Signed-off-by Ackerley Tng <[email protected]>

---
 include/linux/syscalls.h           |  2 +-
 include/uapi/linux/restrictedmem.h |  8 ++++
 mm/restrictedmem.c                 | 74 +++++++++++++++++++++++++++---
 3 files changed, 77 insertions(+), 7 deletions(-)
 create mode 100644 include/uapi/linux/restrictedmem.h

include/linux/syscalls.h +1 -1

index f9e9e0c820c5..a23c4c385cd3 100644
@@ -1056,7 +1056,7 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
 asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
 					    unsigned long home_node,
 					    unsigned long flags);
-asmlinkage long sys_memfd_restricted(unsigned int flags);
+asmlinkage long sys_memfd_restricted(unsigned int flags, int mount_fd);
 
 /*
  * Architecture-specific system calls

include/uapi/linux/restrictedmem.h new file +8 -0

new file mode 100644
index 000000000000..22d6f2285f6d
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_RESTRICTEDMEM_H
+#define _UAPI_LINUX_RESTRICTEDMEM_H
+
+/* flags for memfd_restricted */
+#define RMFD_USERMNT		0x0001U
+
+#endif /* _UAPI_LINUX_RESTRICTEDMEM_H */

mm/restrictedmem.c +2 -1

index c5d869d8c2d8..f7b62364a31a 100644
@@ -1,11 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0
-#include "linux/sbitmap.h"
+#include <linux/namei.h>
 #include <linux/pagemap.h>
 #include <linux/pseudo_fs.h>
 #include <linux/shmem_fs.h>
 #include <linux/syscalls.h>
 #include <uapi/linux/falloc.h>
 #include <uapi/linux/magic.h>
+#include <uapi/linux/restrictedmem.h>
 #include <linux/restrictedmem.h>
 
 struct restrictedmem {

@@ -189,19 +190,20 @@ static struct file *restrictedmem_file_create(struct file *memfd)
 	return file;
 }
 

mm/restrictedmem.c static struct file *restrictedmem_file_create(struct file *memfd) · line 198

Quoted 10 lines

mm/restrictedmem.c +1 -4

-SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
+static int restrictedmem_create(struct vfsmount *mount)
 {
 	struct file *file, *restricted_file;
 	int fd, err;
 
-	if (flags)
-		return -EINVAL;
-
 	fd = get_unused_fd_flags(0);

Any reasons the file descriptors aren't O_CLOEXEC by default? I don't see any reasons why we should introduce new fdtypes that aren't O_CLOEXEC by default. The "don't mix-and-match" train has already left the station anyway as we do have seccomp noitifer fds and pidfds both of which are O_CLOEXEC by default.

Quoted 13 lines

mm/restrictedmem.c +5 -1

 	if (fd < 0)
 		return fd;
 
-	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+	if (mount)
+		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0, VM_NORESERVE);
+	else
+		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+
 	if (IS_ERR(file)) {
 		err = PTR_ERR(file);
 		goto err_fd;

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 230

Quoted 6 lines

mm/restrictedmem.c +3 -0

@@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
 	return err;
 }
 
+static bool is_shmem_mount(struct vfsmount *mnt)
+{
+	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;

This can just be if (mnt->mnt_sb->s_magic == TMPFS_MAGIC).

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 235

Quoted 5 lines

mm/restrictedmem.c +5 -0

+}
+
+static bool is_mount_root(struct file *file)
+{
+	return file->f_path.dentry == file->f_path.mnt->mnt_root;

mount -t tmpfs tmpfs /mnt
touch /mnt/bla
touch /mnt/ble
mount --bind /mnt/bla /mnt/ble
fd = open("/mnt/ble")
fd_restricted = memfd_restricted(fd)

IOW, this doesn't restrict it to the tmpfs root. It only restricts it to paths that refer to the root of any tmpfs mount. To exclude bind-mounts that aren't bind-mounts of the whole filesystem you want:

path->dentry == path->mnt->mnt_root &&
path->mnt->mnt_root == path->mnt->mnt_sb->s_root

Quoted 11 lines

mm/restrictedmem.c +11 -0

+}
+
+static int restrictedmem_create_on_user_mount(int mount_fd)
+{
+	int ret;
+	struct fd f;
+	struct vfsmount *mnt;
+
+	f = fdget_raw(mount_fd);
+	if (!f.file)
+		return -EBADF;

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 256

Quoted 10 lines

mm/restrictedmem.c +10 -0

+
+	ret = -EINVAL;
+	if (!is_mount_root(f.file))
+		goto out;
+
+	mnt = f.file->f_path.mnt;
+	if (!is_shmem_mount(mnt))
+		goto out;
+
+	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);

With the current semantics you're asking whether you have write permissions on the /mnt/ble file in order to get answer to the question whether you're allowed to create an unlinked restricted memory file. That doesn't make much sense afaict.

Quoted 12 lines

mm/restrictedmem.c +12 -0

+	if (ret)
+		goto out;
+
+	ret = mnt_want_write(mnt);
+	if (unlikely(ret))
+		goto out;
+
+	ret = restrictedmem_create(mnt);
+
+	mnt_drop_write(mnt);
+out:
+	fdput(f);

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 278

Quoted 10 lines

mm/restrictedmem.c +10 -0

+
+	return ret;
+}
+
+SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
+{
+	if (flags & ~RMFD_USERMNT)
+		return -EINVAL;
+
+	if (flags == RMFD_USERMNT) {

Why do you even need this flag? It seems that @mount_fd being < 0 is sufficient to indicate that a new restricted memory fd is supposed to be created in the system instance.

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 286

Quoted 8 lines

mm/restrictedmem.c +8 -0

+		if (mount_fd < 0)
+			return -EINVAL;
+
+		return restrictedmem_create_on_user_mount(mount_fd);
+	} else {
+		return restrictedmem_create(NULL);
+	}
+}

I have to say that I'm very confused by all of this the more I look at it.

Effectively memfd restricted functions as a wrapper filesystem around the tmpfs filesystem. This is basically a weird overlay filesystem. You're allocating tmpfs files that you stash in restrictedmem files. I have to say that this seems very hacky. I didn't get this at all at first.

So what does the caller get if they call statx() on a restricted memfd? Do they get the device number of the tmpfs mount and the inode numbers of the tmpfs mount? Because it looks like they would:

static int restrictedmem_getattr(struct user_namespace *mnt_userns,
const struct path *path, struct kstat *stat, u32 request_mask, unsigned int query_flags)

{
	struct inode *inode = d_inode(path->dentry);
	struct restrictedmem *rm = inode->i_mapping->private_data;
	struct file *memfd = rm->memfd;

	return memfd->f_inode->i_op->getattr(mnt_userns, path, stat,

					     request_mask, query_flags);

That @memfd would be a struct file allocated in a tmpfs instance, no? So you'd be calling the inode operation of the tmpfs file meaning that struct kstat will be filled up with the info from the tmpfs instance.

But then if I call statfs() and check the fstype I would get RESTRICTEDMEM_MAGIC, no? This is... unorthodox?

I'm honestly puzzled and this sounds really strange. There must be a better way to implement all of this.

Shouldn't you try and make this a part of tmpfs proper? Make a really separate filesystem and add a memfs library that both tmpfs and restrictedmemfs can use? Add a mount option to tmpfs that makes it a restricted tmpfs?

Quoted 10 lines · Christian Brauner

Quoted 9 lines · Ackerley Tng

...

mm/restrictedmem.c +1 -1

-SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
+static int restrictedmem_create(struct vfsmount *mount)
 {
 	struct file *file, *restricted_file;
 	int fd, err;

Quoted 10 lines

mm/restrictedmem.c static struct file *restrictedmem_file_create(struct file *memfd) · line 198

Quoted 4 lines

mm/restrictedmem.c +0 -3

-	if (flags)
-		return -EINVAL;
-
 	fd = get_unused_fd_flags(0);

Any reasons the file descriptors aren't O_CLOEXEC by default? I don't see any reasons why we should introduce new fdtypes that aren't O_CLOEXEC by default. The "don't mix-and-match" train has already left the station anyway as we do have seccomp noitifer fds and pidfds both of which are O_CLOEXEC by default.

Quoted 15 lines

mm/restrictedmem.c static struct file *restrictedmem_file_create(struct file *memfd)

Quoted 6 lines

mm/restrictedmem.c +2 -1

 	if (fd < 0)
 		return fd;
 
-	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+	if (mount)
+		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem", 0,  

VM_NORESERVE);

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int,

Quoted 7 lines

mm/restrictedmem.c +3 -0

+	else
+		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+
 	if (IS_ERR(file)) {
 		err = PTR_ERR(file);
 		goto err_fd;

@@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned int,  

flags)

Quoted 8 lines

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 230

Quoted 6 lines

mm/restrictedmem.c +3 -0

 	return err;
 }
 
+static bool is_shmem_mount(struct vfsmount *mnt)
+{
+	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;

This can just be if (mnt->mnt_sb->s_magic == TMPFS_MAGIC).

Quoted 12 lines

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 235

Quoted 5 lines

mm/restrictedmem.c +5 -0

+}
+
+static bool is_mount_root(struct file *file)
+{
+	return file->f_path.dentry == file->f_path.mnt->mnt_root;

mount -t tmpfs tmpfs /mnt
touch /mnt/bla
touch /mnt/ble
mount --bind /mnt/bla /mnt/ble
fd = open("/mnt/ble")
fd_restricted = memfd_restricted(fd)

Quoted 6 lines

IOW, this doesn't restrict it to the tmpfs root. It only restricts it to paths that refer to the root of any tmpfs mount. To exclude bind-mounts that aren't bind-mounts of the whole filesystem you want:

path->dentry == path->mnt->mnt_root &&
path->mnt->mnt_root == path->mnt->mnt_sb->s_root

Quoted 18 lines

Quoted 18 lines

mm/restrictedmem.c +18 -0

+}
+
+static int restrictedmem_create_on_user_mount(int mount_fd)
+{
+	int ret;
+	struct fd f;
+	struct vfsmount *mnt;
+
+	f = fdget_raw(mount_fd);
+	if (!f.file)
+		return -EBADF;
+
+	ret = -EINVAL;
+	if (!is_mount_root(f.file))
+		goto out;
+
+	mnt = f.file->f_path.mnt;
+	if (!is_shmem_mount(mnt))

Quoted 8 lines

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 256

Quoted 3 lines

mm/restrictedmem.c +3 -0

+		goto out;
+
+	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);

With the current semantics you're asking whether you have write permissions on the /mnt/ble file in order to get answer to the question whether you're allowed to create an unlinked restricted memory file. That doesn't make much sense afaict.

Quoted 18 lines

Quoted 18 lines

mm/restrictedmem.c +18 -0

+	if (ret)
+		goto out;
+
+	ret = mnt_want_write(mnt);
+	if (unlikely(ret))
+		goto out;
+
+	ret = restrictedmem_create(mnt);
+
+	mnt_drop_write(mnt);
+out:
+	fdput(f);
+
+	return ret;
+}
+
+SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
+{

Quoted 8 lines

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 278

Quoted 4 lines

mm/restrictedmem.c +4 -0

+	if (flags & ~RMFD_USERMNT)
+		return -EINVAL;
+
+	if (flags == RMFD_USERMNT) {

Why do you even need this flag? It seems that @mount_fd being < 0 is sufficient to indicate that a new restricted memory fd is supposed to be created in the system instance.

Quoted 24 lines

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 286

Quoted 8 lines

mm/restrictedmem.c +8 -0

+		if (mount_fd < 0)
+			return -EINVAL;
+
+		return restrictedmem_create_on_user_mount(mount_fd);
+	} else {
+		return restrictedmem_create(NULL);
+	}
+}

I have to say that I'm very confused by all of this the more I look at it.

Effectively memfd restricted functions as a wrapper filesystem around the tmpfs filesystem. This is basically a weird overlay filesystem. You're allocating tmpfs files that you stash in restrictedmem files. I have to say that this seems very hacky. I didn't get this at all at first.

So what does the caller get if they call statx() on a restricted memfd? Do they get the device number of the tmpfs mount and the inode numbers of the tmpfs mount? Because it looks like they would:

static int restrictedmem_getattr(struct user_namespace *mnt_userns,
const struct path *path, struct kstat *stat, u32 request_mask, unsigned int query_flags)

Quoted 6 lines

{
	struct inode *inode = d_inode(path->dentry);
	struct restrictedmem *rm = inode->i_mapping->private_data;
	struct file *memfd = rm->memfd;

	return memfd->f_inode->i_op->getattr(mnt_userns, path, stat,

This is pretty broken btw, because @path refers to a restrictedmem path which you're passing to a tmpfs iop...

I see that in

	return memfd->f_inode->i_op->getattr(mnt_userns, &memfd->f_path, stat,
					     request_mask, query_flags);

this if fixed but still, this is... not great.

Quoted 8 lines

					     request_mask, query_flags);

That @memfd would be a struct file allocated in a tmpfs instance, no? So you'd be calling the inode operation of the tmpfs file meaning that struct kstat will be filled up with the info from the tmpfs instance.

But then if I call statfs() and check the fstype I would get RESTRICTEDMEM_MAGIC, no? This is... unorthodox?

Quoted 7 lines

I'm honestly puzzled and this sounds really strange. There must be a better way to implement all of this.

Shouldn't you try and make this a part of tmpfs proper? Make a really separate filesystem and add a memfs library that both tmpfs and restrictedmemfs can use? Add a mount option to tmpfs that makes it a restricted tmpfs?

Thanks again for your review!

Quoted 11 lines · Christian Brauner

Quoted 10 lines · Christian Brauner

Quoted 9 lines · Ackerley Tng

...

mm/restrictedmem.c +1 -1

-SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags)
+static int restrictedmem_create(struct vfsmount *mount)
 {
 	struct file *file, *restricted_file;
 	int fd, err;

Quoted 10 lines

Quoted 10 lines

mm/restrictedmem.c static struct file *restrictedmem_file_create(struct file *memfd) · line 198

Quoted 4 lines

mm/restrictedmem.c +0 -3

-	if (flags)
-		return -EINVAL;
-
 	fd = get_unused_fd_flags(0);

Any reasons the file descriptors aren't O_CLOEXEC by default? I don't see any reasons why we should introduce new fdtypes that aren't O_CLOEXEC by default. The "don't mix-and-match" train has already left the station anyway as we do have seccomp noitifer fds and pidfds both of which are O_CLOEXEC by default.

Thanks for pointing this out. I agree with using O_CLOEXEC, but didn’t notice this before. Let us discuss this under the original series at [1].

Quoted 15 lines

Quoted 15 lines

mm/restrictedmem.c static struct file *restrictedmem_file_create(struct file *memfd)

Quoted 6 lines

mm/restrictedmem.c +2 -1

 	if (fd < 0)
 		return fd;
 
-	file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+	if (mount)
+		file = shmem_file_setup_with_mnt(mount, "memfd:restrictedmem",

0, VM_NORESERVE);

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned

Quoted 7 lines

mm/restrictedmem.c +3 -0

+	else
+		file = shmem_file_setup("memfd:restrictedmem", 0, VM_NORESERVE);
+
 	if (IS_ERR(file)) {
 		err = PTR_ERR(file);
 		goto err_fd;

@@ -223,6 +225,66 @@ SYSCALL_DEFINE1(memfd_restricted, unsigned

int, flags)

Quoted 8 lines

Quoted 8 lines

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 230

Quoted 6 lines

mm/restrictedmem.c +3 -0

 	return err;
 }
 
+static bool is_shmem_mount(struct vfsmount *mnt)
+{
+	return mnt && mnt->mnt_sb && mnt->mnt_sb->s_magic == TMPFS_MAGIC;

This can just be if (mnt->mnt_sb->s_magic == TMPFS_MAGIC).

Will simplify this in the next revision.

Quoted 13 lines

Quoted 12 lines

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 235

Quoted 5 lines

mm/restrictedmem.c +5 -0

+}
+
+static bool is_mount_root(struct file *file)
+{
+	return file->f_path.dentry == file->f_path.mnt->mnt_root;

mount -t tmpfs tmpfs /mnt
touch /mnt/bla
touch /mnt/ble
mount --bind /mnt/bla /mnt/ble
fd = open("/mnt/ble")
fd_restricted = memfd_restricted(fd)

Quoted 6 lines

Quoted 6 lines

IOW, this doesn't restrict it to the tmpfs root. It only restricts it to paths that refer to the root of any tmpfs mount. To exclude bind-mounts that aren't bind-mounts of the whole filesystem you want:

path->dentry == path->mnt->mnt_root &&
path->mnt->mnt_root == path->mnt->mnt_sb->s_root

Will adopt this in the next revision and add a selftest to check this. Thanks for pointing this out!

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

mm/restrictedmem.c +18 -0

+}
+
+static int restrictedmem_create_on_user_mount(int mount_fd)
+{
+	int ret;
+	struct fd f;
+	struct vfsmount *mnt;
+
+	f = fdget_raw(mount_fd);
+	if (!f.file)
+		return -EBADF;
+
+	ret = -EINVAL;
+	if (!is_mount_root(f.file))
+		goto out;
+
+	mnt = f.file->f_path.mnt;
+	if (!is_shmem_mount(mnt))

Quoted 8 lines

Quoted 8 lines

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 256

Quoted 3 lines

mm/restrictedmem.c +3 -0

+		goto out;
+
+	ret = file_permission(f.file, MAY_WRITE | MAY_EXEC);

With the current semantics you're asking whether you have write permissions on the /mnt/ble file in order to get answer to the question whether you're allowed to create an unlinked restricted memory file. That doesn't make much sense afaict.

That's true. Since mnt_want_write() already checks for write permissions and this syscall creates an unlinked file on the mount, we don't have to check permissions on the file then. Will remove this in the next revision!

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

mm/restrictedmem.c +18 -0

+	if (ret)
+		goto out;
+
+	ret = mnt_want_write(mnt);
+	if (unlikely(ret))
+		goto out;
+
+	ret = restrictedmem_create(mnt);
+
+	mnt_drop_write(mnt);
+out:
+	fdput(f);
+
+	return ret;
+}
+
+SYSCALL_DEFINE2(memfd_restricted, unsigned int, flags, int, mount_fd)
+{

Quoted 8 lines

Quoted 8 lines

mm/restrictedmem.c SYSCALL_DEFINE1(memfd_restricted, unsigned int, flags) · line 278

Quoted 4 lines

mm/restrictedmem.c +4 -0

+	if (flags & ~RMFD_USERMNT)
+		return -EINVAL;
+
+	if (flags == RMFD_USERMNT) {

Why do you even need this flag? It seems that @mount_fd being < 0 is sufficient to indicate that a new restricted memory fd is supposed to be created in the system instance.

I'm hoping to have this patch series merged after Chao's patch series introduces the memfd_restricted() syscall [1].

Quoted 4 lines · Ackerley Tng

...

Quoted 2 lines

Quoted 2 lines

Why do you even need this flag? It seems that @mount_fd being < 0 is sufficient to indicate that a new restricted memory fd is supposed

Quoted 6 lines

to be

Quoted 1 line

Quoted 1 line

created in the system instance.

I'm hoping to have this patch series merged after Chao's patch series introduces the memfd_restricted() syscall [1].

I'm curious, is there an LSFMM session for this?

Quoted 1 line · Christian Brauner

I'm curious, is there an LSFMM session for this?

As far as I know, there is no LSFMM session for this.

For memfd_restricted() calls without a userspace mount, the backing file should be the shmem mount in the kernel, and the size of backing pages should be as defined by system-wide shmem configuration.

If a userspace mount is provided, the size of backing pages should be as defined in the mount.

Also includes negative tests for invalid inputs, including fds representing read-only superblocks/mounts.

Signed-off-by Ackerley Tng <[email protected]>

---
  tools/testing/selftests/Makefile              |   1 +
  .../selftests/restrictedmem/.gitignore        |   3 +
  .../testing/selftests/restrictedmem/Makefile  |  15 +
  .../testing/selftests/restrictedmem/common.c  |   9 +
  .../testing/selftests/restrictedmem/common.h  |   8 +
  .../restrictedmem_hugepage_test.c             | 486 ++++++++++++++++++
  6 files changed, 522 insertions(+)

create mode 100644 tools/testing/selftests/restrictedmem/.gitignore create mode 100644 tools/testing/selftests/restrictedmem/Makefile
create mode 100644 tools/testing/selftests/restrictedmem/common.c
create mode 100644 tools/testing/selftests/restrictedmem/common.h
create mode 100644 tools/testing/selftests/restrictedmem/restrictedmem_hugepage_test.c

tools/testing/selftests/Makefile +1 -0

index f07aef7c592c..44078eeefb79 100644
@@ -60,6 +60,7 @@ TARGETS += pstore
  TARGETS += ptrace
  TARGETS += openat2
  TARGETS += resctrl
+TARGETS += restrictedmem
  TARGETS += rlimits
  TARGETS += rseq
  TARGETS += rtc

tools/testing/selftests/restrictedmem/.gitignore new file +3 -0

new file mode 100644
index 000000000000..2581bcc8ff29
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+restrictedmem_hugepage_test

tools/testing/selftests/restrictedmem/Makefile new file +15 -0

new file mode 100644
index 000000000000..8e5378d20226
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0
+
+CFLAGS = $(KHDR_INCLUDES)
+CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -std=gnu99
+
+TEST_GEN_PROGS += restrictedmem_hugepage_test
+
+include ../lib.mk
+
+EXTRA_CLEAN = $(OUTPUT)/common.o
+
+$(OUTPUT)/common.o: common.c
+	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c -ffreestanding $< -o $@
+
+$(TEST_GEN_PROGS): $(OUTPUT)/common.o

tools/testing/selftests/restrictedmem/common.c new file +9 -0

new file mode 100644
index 000000000000..03dac843404f
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <sys/syscall.h>
+#include <unistd.h>
+
+int memfd_restricted(unsigned int flags, int mount_fd)
+{
+	return syscall(__NR_memfd_restricted, flags, mount_fd);
+}

tools/testing/selftests/restrictedmem/common.h new file +8 -0

new file mode 100644
index 000000000000..06284ed86baf
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef SELFTESTS_RESTRICTEDMEM_COMMON_H
+#define SELFTESTS_RESTRICTEDMEM_COMMON_H
+
+int memfd_restricted(unsigned int flags, int mount_fd);
+
+#endif  // SELFTESTS_RESTRICTEDMEM_COMMON_H

tools/testing/selftests/restrictedmem/restrictedmem_hugepage_test.c new file +486 -0

new file mode 100644
index 000000000000..9ed319b83cb8
@@ -0,0 +1,486 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define _GNU_SOURCE /* for O_PATH */
+#define _POSIX_C_SOURCE /* for PATH_MAX */
+#include <limits.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "linux/restrictedmem.h"
+
+#include "common.h"
+#include "../kselftest_harness.h"
+
+/*
+ * Expect policy to be one of always, within_size, advise, never,
+ * deny, force
+ */
+#define POLICY_BUF_SIZE 12
+
+static int get_hpage_pmd_size(void)
+{
+	FILE *fp;
+	char buf[100];
+	char *ret;
+	int size;
+
+	fp = fopen("/sys/kernel/mm/transparent_hugepage/hpage_pmd_size", "r");
+	if (!fp)
+		return -1;
+
+	ret = fgets(buf, 100, fp);
+	if (ret != buf) {
+		size = -1;
+		goto out;
+	}
+
+	if (sscanf(buf, "%d\n", &size) != 1)
+		size = -1;
+
+out:
+	fclose(fp);
+
+	return size;
+}
+
+static bool is_valid_shmem_thp_policy(char *policy)
+{
+	if (strcmp(policy, "always") == 0)
+		return true;
+	if (strcmp(policy, "within_size") == 0)
+		return true;
+	if (strcmp(policy, "advise") == 0)
+		return true;
+	if (strcmp(policy, "never") == 0)
+		return true;
+	if (strcmp(policy, "deny") == 0)
+		return true;
+	if (strcmp(policy, "force") == 0)
+		return true;
+
+	return false;
+}
+
+static int get_shmem_thp_policy(char *policy)
+{
+	FILE *fp;
+	char buf[100];
+	char *left = NULL;
+	char *right = NULL;
+	int ret = -1;
+
+	fp = fopen("/sys/kernel/mm/transparent_hugepage/shmem_enabled", "r");
+	if (!fp)
+		return -1;
+
+	if (fgets(buf, 100, fp) != buf)
+		goto out;
+
+	/*
+	 * Expect shmem_enabled to be of format like "always within_size advise
+	 * [never] deny force"
+	 */
+	left = memchr(buf, '[', 100);
+	if (!left)
+		goto out;
+
+	right = memchr(buf, ']', 100);
+	if (!right)
+		goto out;
+
+	memcpy(policy, left + 1, right - left - 1);
+
+	ret = !is_valid_shmem_thp_policy(policy);
+
+out:
+	fclose(fp);
+	return ret;
+}
+
+static int write_string_to_file(const char *path, const char *string)
+{
+	FILE *fp;
+	size_t len = strlen(string);
+	int ret = -1;
+
+	fp = fopen(path, "w");
+	if (!fp)
+		return ret;
+
+	if (fwrite(string, 1, len, fp) != len)
+		goto out;
+
+	ret = 0;
+
+out:
+	fclose(fp);
+	return ret;
+}
+
+static int set_shmem_thp_policy(char *policy)
+{
+	int ret = -1;
+	/* +1 for newline */
+	char to_write[POLICY_BUF_SIZE + 1] = { 0 };
+
+	if (!is_valid_shmem_thp_policy(policy))
+		return ret;
+
+	ret = snprintf(to_write, POLICY_BUF_SIZE + 1, "%s\n", policy);
+	if (ret != strlen(policy) + 1)
+		return -1;
+
+	ret = write_string_to_file(
+		"/sys/kernel/mm/transparent_hugepage/shmem_enabled", to_write);
+
+	return ret;
+}
+
+FIXTURE(reset_shmem_enabled)
+{
+	char shmem_enabled[POLICY_BUF_SIZE];
+};
+
+FIXTURE_SETUP(reset_shmem_enabled)
+{
+	memset(self->shmem_enabled, 0, POLICY_BUF_SIZE);
+	ASSERT_EQ(get_shmem_thp_policy(self->shmem_enabled), 0);
+}
+
+FIXTURE_TEARDOWN(reset_shmem_enabled)
+{
+	ASSERT_EQ(set_shmem_thp_policy(self->shmem_enabled), 0);
+}
+
+TEST_F(reset_shmem_enabled, restrictedmem_fstat_shmem_enabled_never)
+{
+	int fd = -1;
+	struct stat stat;
+
+	ASSERT_EQ(set_shmem_thp_policy("never"), 0);
+
+	fd = memfd_restricted(0, -1);
+	ASSERT_GT(fd, 0);
+
+	ASSERT_EQ(fstat(fd, &stat), 0);
+
+	/*
+	 * st_blksize is set based on the superblock's s_blocksize_bits. For
+	 * shmem, this is set to PAGE_SHIFT
+	 */
+	ASSERT_EQ(stat.st_blksize, getpagesize());
+
+	close(fd);
+}
+
+TEST_F(reset_shmem_enabled, restrictedmem_fstat_shmem_enabled_always)
+{
+	int fd = -1;
+	struct stat stat;
+
+	ASSERT_EQ(set_shmem_thp_policy("always"), 0);
+
+	fd = memfd_restricted(0, -1);
+	ASSERT_GT(fd, 0);
+
+	ASSERT_EQ(fstat(fd, &stat), 0);
+
+	ASSERT_EQ(stat.st_blksize, get_hpage_pmd_size());
+
+	close(fd);
+}
+
+TEST(restrictedmem_tmpfile_invalid_fd)
+{
+	int fd = memfd_restricted(RMFD_USERMNT, -2);
+
+	ASSERT_EQ(fd, -1);
+	ASSERT_EQ(errno, EINVAL);
+}
+
+TEST(restrictedmem_tmpfile_fd_not_a_mount)
+{
+	int fd = memfd_restricted(RMFD_USERMNT, STDOUT_FILENO);
+
+	ASSERT_EQ(fd, -1);
+	ASSERT_EQ(errno, EINVAL);
+}
+
+TEST(restrictedmem_tmpfile_not_tmpfs_mount)
+{
+	int fd = -1;
+	int mfd = -1;
+
+	mfd = open("/proc", O_PATH);
+	ASSERT_NE(mfd, -1);
+
+	fd = memfd_restricted(RMFD_USERMNT, mfd);
+
+	ASSERT_EQ(fd, -1);
+	ASSERT_EQ(errno, EINVAL);
+}
+
+FIXTURE(tmpfs_hugepage_sfd)
+{
+	int sfd;
+};
+
+FIXTURE_SETUP(tmpfs_hugepage_sfd)
+{
+	self->sfd = fsopen("tmpfs", 0);
+	ASSERT_NE(self->sfd, -1);
+}
+
+FIXTURE_TEARDOWN(tmpfs_hugepage_sfd)
+{
+	EXPECT_EQ(close(self->sfd), 0);
+}
+
+TEST_F(tmpfs_hugepage_sfd, restrictedmem_fstat_tmpfs_huge_always)
+{
+	int ret = -1;
+	int fd = -1;
+	int mfd = -1;
+	struct stat stat;
+
+	fsconfig(self->sfd, FSCONFIG_SET_STRING, "huge", "always", 0);
+	fsconfig(self->sfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0);
+
+	mfd = fsmount(self->sfd, 0, 0);
+	ASSERT_NE(mfd, -1);
+
+	fd = memfd_restricted(RMFD_USERMNT, mfd);
+	ASSERT_GT(fd, 0);
+
+	/* User can close reference to mount */
+	ret = close(mfd);
+	ASSERT_EQ(ret, 0);
+
+	ret = fstat(fd, &stat);
+	ASSERT_EQ(ret, 0);
+	ASSERT_EQ(stat.st_blksize, get_hpage_pmd_size());
+
+	close(fd);
+}
+
+TEST_F(tmpfs_hugepage_sfd, restrictedmem_fstat_tmpfs_huge_never)
+{
+	int ret = -1;
+	int fd = -1;
+	int mfd = -1;
+	struct stat stat;
+
+	fsconfig(self->sfd, FSCONFIG_SET_STRING, "huge", "never", 0);
+	fsconfig(self->sfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0);
+
+	mfd = fsmount(self->sfd, 0, 0);
+	ASSERT_NE(mfd, -1);
+
+	fd = memfd_restricted(RMFD_USERMNT, mfd);
+	ASSERT_GT(fd, 0);
+
+	/* User can close reference to mount */
+	ret = close(mfd);
+	ASSERT_EQ(ret, 0);
+
+	ret = fstat(fd, &stat);
+	ASSERT_EQ(ret, 0);
+	ASSERT_EQ(stat.st_blksize, getpagesize());
+
+	close(fd);
+}
+
+TEST_F(tmpfs_hugepage_sfd, restrictedmem_check_mount_flags)
+{
+	int ret = -1;
+	int fd = -1;
+	int mfd = -1;
+
+	fsconfig(self->sfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0);
+
+	mfd = fsmount(self->sfd, 0, MOUNT_ATTR_RDONLY);
+	ASSERT_NE(mfd, -1);
+
+	fd = memfd_restricted(RMFD_USERMNT, mfd);
+	ASSERT_EQ(fd, -1);
+	ASSERT_EQ(errno, EROFS);
+
+	ret = close(mfd);
+	ASSERT_EQ(ret, 0);
+}
+
+TEST_F(tmpfs_hugepage_sfd, restrictedmem_check_superblock_flags)
+{
+	int ret = -1;
+	int fd = -1;
+	int mfd = -1;
+
+	fsconfig(self->sfd, FSCONFIG_SET_FLAG, "ro", NULL, 0);
+	fsconfig(self->sfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0);
+
+	mfd = fsmount(self->sfd, 0, 0);
+	ASSERT_NE(mfd, -1);
+
+	fd = memfd_restricted(RMFD_USERMNT, mfd);
+	ASSERT_EQ(fd, -1);
+	ASSERT_EQ(errno, EROFS);
+
+	ret = close(mfd);
+	ASSERT_EQ(ret, 0);
+}
+
+static bool directory_exists(const char *path)
+{
+	struct stat sb;
+
+	return stat(path, &sb) == 0 && S_ISDIR(sb.st_mode);
+}
+
+FIXTURE(tmpfs_hugepage_mount_path)
+{
+	char *mount_path;
+};
+
+FIXTURE_SETUP(tmpfs_hugepage_mount_path)
+{
+	int ret = -1;
+
+	/* /tmp is an FHS-mandated world-writable directory */
+	self->mount_path = "/tmp/restrictedmem-selftest-mnt";
+
+	if (!directory_exists(self->mount_path)) {
+		ret = mkdir(self->mount_path, 0777);
+		ASSERT_EQ(ret, 0);
+	}
+}
+
+FIXTURE_TEARDOWN(tmpfs_hugepage_mount_path)
+{
+	int ret = -1;
+
+	if (!directory_exists(self->mount_path))
+		return;
+
+	ret = umount2(self->mount_path, MNT_FORCE);
+	EXPECT_EQ(ret, 0);
+	if (ret == -1 && errno == EINVAL)
+		fprintf(stderr, "  %s was not mounted\n", self->mount_path);
+
+	ret = rmdir(self->mount_path);
+	EXPECT_EQ(ret, 0);
+	if (ret == -1)
+		fprintf(stderr, "  rmdir(%s) failed: %m\n", self->mount_path);
+}
+
+/*
+ * memfd_restricted() syscall can only be used with the fd of the root of the
+ * mount. When the restrictedmem's fd is open, a user should not be able to
+ * unmount or remove the mounted directory
+ */
+TEST_F(tmpfs_hugepage_mount_path, restrictedmem_umount_rmdir_while_file_open)
+{
+	int ret = -1;
+	int fd = -1;
+	int mfd = -1;
+	struct stat stat;
+
+	ret = mount("name", self->mount_path, "tmpfs", 0, "huge=always");
+	ASSERT_EQ(ret, 0);
+
+	mfd = open(self->mount_path, O_PATH);
+	ASSERT_NE(mfd, -1);
+
+	fd = memfd_restricted(RMFD_USERMNT, mfd);
+	ASSERT_GT(fd, 0);
+
+	/* We don't need this reference to the mount anymore */
+	ret = close(mfd);
+	ASSERT_EQ(ret, 0);
+
+	/* restrictedmem's fd should still be usable */
+	ret = fstat(fd, &stat);
+	ASSERT_EQ(ret, 0);
+	ASSERT_EQ(stat.st_blksize, get_hpage_pmd_size());
+
+	/* User should not be able to unmount directory */
+	ret = umount2(self->mount_path, MNT_FORCE);
+	ASSERT_EQ(ret, -1);
+	ASSERT_EQ(errno, EBUSY);
+
+	ret = rmdir(self->mount_path);
+	ASSERT_EQ(ret, -1);
+	ASSERT_EQ(errno, EBUSY);
+
+	close(fd);
+}
+
+/* The fd of a file on the mount cannot be provided as mount_fd */
+TEST_F(tmpfs_hugepage_mount_path, restrictedmem_provide_fd_of_file)
+{
+	int ret = -1;
+	int fd = -1;
+	int ffd = -1;
+	char tmp_file_path[PATH_MAX] = { 0 };
+
+	ret = mount("name", self->mount_path, "tmpfs", 0, "huge=always");
+	ASSERT_EQ(ret, 0);
+
+	snprintf(tmp_file_path, PATH_MAX, "%s/tmp-file", self->mount_path);
+	ret = write_string_to_file(tmp_file_path, "filler\n");
+	ASSERT_EQ(ret, 0);
+
+	ffd = open(tmp_file_path, O_RDWR);
+	ASSERT_GT(ffd, 0);
+
+	fd = memfd_restricted(RMFD_USERMNT, ffd);
+	ASSERT_LT(fd, 0);
+	ASSERT_EQ(errno, EINVAL);
+
+	ret = close(ffd);
+	ASSERT_EQ(ret, 0);
+
+	close(fd);
+	remove(tmp_file_path);
+}
+
+/* The fd of files on the mount cannot be provided as mount_fd */
+TEST_F(tmpfs_hugepage_mount_path, restrictedmem_provide_fd_of_file_in_subdir)
+{
+	int ret = -1;
+	int fd = -1;
+	int ffd = -1;
+	char tmp_dir_path[PATH_MAX] = { 0 };
+	char tmp_file_path[PATH_MAX] = { 0 };
+
+	ret = mount("name", self->mount_path, "tmpfs", 0, "huge=always");
+	ASSERT_EQ(ret, 0);
+
+	snprintf(tmp_dir_path, PATH_MAX, "%s/tmp-subdir", self->mount_path);
+	ret = mkdir(tmp_dir_path, 0777);
+	ASSERT_EQ(ret, 0);
+
+	snprintf(tmp_file_path, PATH_MAX, "%s/tmp-subdir/tmp-file",
+		 self->mount_path);
+	ret = write_string_to_file(tmp_file_path, "filler\n");
+	ASSERT_EQ(ret, 0);
+
+	ffd = open(tmp_file_path, O_RDWR);
+	ASSERT_NE(ffd, -1);
+
+	fd = memfd_restricted(RMFD_USERMNT, ffd);
+	ASSERT_LT(fd, 0);
+	ASSERT_EQ(errno, EINVAL);
+
+	ret = close(ffd);
+	ASSERT_EQ(ret, 0);
+
+	close(fd);
+	remove(tmp_file_path);
+	rmdir(tmp_dir_path);
+}
+
+TEST_HARNESS_MAIN

Quoted 6 lines · Ackerley Tng

For memfd_restricted() calls without a userspace mount, the backing file should be the shmem mount in the kernel, and the size of backing pages should be as defined by system-wide shmem configuration.

If a userspace mount is provided, the size of backing pages should be as defined in the mount.

Quoted 2 lines

Also includes negative tests for invalid inputs, including fds representing read-only superblocks/mounts.

When you talk about "hugepage" in this patch, do you mean THP or hugetlb? I suspect thp, so it might be better to spell that out. IIRC, there are plans to support actual huge pages in the future, at which point "hugepage" terminology could be misleading.

Quoted 17 lines

Signed-off-by Ackerley Tng <[email protected]>

---
   tools/testing/selftests/Makefile              |   1 +
   .../selftests/restrictedmem/.gitignore        |   3 +
   .../testing/selftests/restrictedmem/Makefile  |  15 +
   .../testing/selftests/restrictedmem/common.c  |   9 +
   .../testing/selftests/restrictedmem/common.h  |   8 +
   .../restrictedmem_hugepage_test.c             | 486 ++++++++++++++++++
   6 files changed, 522 insertions(+)

create mode 100644 tools/testing/selftests/restrictedmem/.gitignore create mode 100644 tools/testing/selftests/restrictedmem/Makefile create mode 100644 tools/testing/selftests/restrictedmem/common.c create mode 100644 tools/testing/selftests/restrictedmem/common.h create mode 100644
tools/testing/selftests/restrictedmem/restrictedmem_hugepage_test.c

...