Page MenuHomeFreeBSD

Replace kmem_free()'s arena parameter
ClosedPublic

Authored by alc on Aug 22 2018, 3:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 8 2024, 12:04 AM
Unknown Object (File)
Dec 13 2023, 4:25 AM
Unknown Object (File)
Nov 10 2023, 12:36 AM
Unknown Object (File)
Nov 8 2023, 3:23 AM
Unknown Object (File)
Nov 7 2023, 2:57 PM
Unknown Object (File)
Nov 1 2023, 11:35 PM
Unknown Object (File)
Oct 8 2023, 11:27 PM
Unknown Object (File)
Oct 7 2023, 2:14 AM
Subscribers

Details

Summary

Replace kmem_free()'s arena parameter with a flag.

This revision has one sort of functional change. The hyperv driver was allocating memory with the newish flag M_EXEC, but passing the wrong arena to both kmem_malloc() and kmem_free(). Passing the wrong arena to kmem_malloc() was harmless because the argument was ignored. However, the arena argument is currently used by kmem_free() as essentially a flag to distinguish executable from non-executable memory. I said, "sort of," because I suspect that the kmem_free() call would only occur if the driver were unloaded or failed during initialization, which seems unlikely for a virtualization driver.

That said, I really wish that the flags parameter did not exist, because it still allows for mistakes like we see in the hyperv driver. Alternatively, we could set a flag in the vm_page structure, indicating that the page was specially mapped for execute access, and have _kmem_unback() return the appropriate arena rather than a domain number. Lastly, kmem_back_domain() would have to set this flag when mapping a page for execute access. Thoughts?

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

I like the idea with a PG_KMEM_EXEC flag and do not like the flags parameter addition to kmem_free(). Alternatively, you can add kmem_free_exec(9).

In D16845#358703, @kib wrote:

I like the idea with a PG_KMEM_EXEC flag and do not like the flags parameter addition to kmem_free(). Alternatively, you can add kmem_free_exec(9).

I too prefer the notion of using a per-page flag.

Eliminate the arena parameter to kmem_free().

Introduce a per-page flag, VPO_KMEM_EXEC, to mark physical pages that are mapped in kmem with execute permissions. Use this flag to determine which arena the kmem virtual addresses are returned to.

Eliminate UMA_SLAB_KRWX. The introduction of VPO_KMEM_EXEC makes it redundant.

Update the nearby comment for UMA_SLAB_KERNEL.

Adding jtl@ because this change replaces the per-slab executable flag that he introduced by a per-page flag.

vm/vm_page.h
237

/* kmem mapping allows execution */

This revision is now accepted and ready to land.Aug 23 2018, 7:22 PM

Update to reflect the reinstatement of drm{,2}.

This revision now requires review to proceed.Aug 24 2018, 3:49 AM
This revision is now accepted and ready to land.Aug 24 2018, 5:37 AM
vm/vm_page.h
237

Why VPO_ and not PG_ ? IMO it somewhat abuses the fact that it is always the kernel object and it is locked when the pages are allocated.

vm/vm_page.h
237

I can't foresee this flag being used in conjunction with any other object than the kernel "kmem" object, where we are guaranteed that there is only the one special kernel mapping of the page. Ordinarily, we would not store a mapping attribute as part of the physical page state.

Given the infrequency with which this flag is used, I did not want to burden vm_page_alloc() with setting it. In other words, I did not want to add a VM_ALLOC_* flag to vm_page_alloc() that would set a PG_* flag before insertion of the page into the object.

At the same time, the comments describing PG_* flags say

* Page flags.  If changed at any other time than page allocation or                                                    
* freeing, the modification must be protected by the vm_page lock.

If I followed this rule, I would have to acquire the page lock in kmem_back_domain() to set a PG_KMEM_EXEC flag. I didn't see the point of that.

By the same argument that this flag is infrequently set, I could (and would happily) make this a PGA_KMEM_EXEC flag.

vm/vm_page.h
237

... and there aren't many free bits in aflags, currently.

This change was committed.