Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] mm: alloc_cc_shared_pages_node fails to strip __GFP_MOVABLE - [Medium] mm: alloc_cc_shared_pages_node fails to strip __GFP_HIGHMEM in fallback path - [Medium] mm: cc_make_shared and cc_make_private are not exported for modular callers --
commit 3978c8ce72bbc2aa23e57a460b585808665262a4 Author: Aneesh Kumar K.V (Arm) <[email protected]> mm: Add an allocator for CoCo shared memory This commit introduces a common allocator for confidential-computing guests to allocate shared memory. It provides helpers to allocate properly aligned contiguous pages and transition them to shared state. It also adds helpers for transitioning memory states when callers manage their own backing memory. > diff --git a/mm/cc_shared.c b/mm/cc_shared.c > new file mode 100644 > index 0000000000000..85e16f4504b8f > --- /dev/null > +++ b/mm/cc_shared.c [ ... ] > +int cc_make_shared(void *addr, size_t size) > +{ > + int ret = cc_validate_transition(addr, size); > + > + if (ret) > + return ret; > + > + return set_memory_decrypted((unsigned long)addr, size >> PAGE_SHIFT); > +} [Severity: Medium] Will this function be unavailable to dynamically loadable modules since it is missing an EXPORT_SYMBOL_GPL() macro? The commit message states it should be usable by callers managing their own backing memory, but virtio or guest drivers won't be able to link to it without an export. > + > +int cc_make_private(void *addr, size_t size) > +{ > + int ret = cc_validate_transition(addr, size); > + > + if (ret) > + return ret; > + > + return set_memory_encrypted((unsigned long)addr, size >> PAGE_SHIFT); > +} [Severity: Medium] Does this function also need an EXPORT_SYMBOL_GPL() macro so that modules can successfully link and call it? [ ... ] > +static int __alloc_cc_shared_pages_node(int nid, gfp_t gfp, > + size_t requested, > + struct cc_shared_pages *mem) > +{ [ ... ] > + /* > + * State transitions require a linear-map address and may modify memory. > + * Allocate from low memory and defer requested zeroing until > afterwards. > + */ > + gfp &= ~(__GFP_HIGHMEM | __GFP_ZERO); [Severity: High] Does this code inadvertently leave __GFP_MOVABLE intact if passed by the caller? Pages are returned as shared memory, but they lack migration hooks (movable_operations) to handle encryption state transitions during page migration. If a caller passes __GFP_MOVABLE, the page might be placed in ZONE_MOVABLE. Since the kernel migration code does not know how to handle encryption state transitions, the page cannot be safely migrated. Could this permanently pin the movable zone, thereby breaking memory hot-unplug and compaction? > + if (nid == NUMA_NO_NODE) > + page = alloc_pages(gfp, order); [ ... ] > +int alloc_cc_shared_pages_node(int nid, gfp_t gfp, > + size_t requested, > + struct cc_shared_pages *mem) > +{ [ ... ] > + if (cc_platform_has(CC_ATTR_MEM_ENCRYPT)) > + return __alloc_cc_shared_pages_node(nid, gfp, requested, mem); > + > + order = get_order(requested); > + if (order > MAX_PAGE_ORDER) > + return -EINVAL; > + > + if (nid == NUMA_NO_NODE) > + page = alloc_pages(gfp, order); > + else > + page = alloc_pages_node(nid, gfp, order); [Severity: Medium] Can this fallback path inadvertently allocate a highmem page? The function's kernel-doc explicitly guarantees: "A memory-state transition requires a valid linear-map address, so such allocations never come from high memory". In the non-encrypted fallback path, the caller's unmodified gfp flags (potentially containing __GFP_HIGHMEM) are passed directly to alloc_pages() or alloc_pages_node() without any masking. If a caller passes a flag mask containing __GFP_HIGHMEM (e.g., GFP_HIGHUSER) on a system without memory encryption, the fallback path will allocate a highmem page. Any subsequent attempt by the caller to access the memory via page_address() could return NULL and cause a crash, breaking the API contract. Should the gfp flags be sanitized in this path as well? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
