[email protected] writes:
> 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?
>
I guess we don't want the cc_make_shared() and cc_make_private() APIs to be
called by modules. Modules that need CoCo shared memory should use
alloc_cc_shared_pages().
>
> [ ... ]
>> +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?
>
I don't think we want CoCo shared memory allocations to be movable, but
I'm also not sure whether we should filter out all of these GFP flags.
>
>> + 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".
>
Only when a private-to-shared transition is required.
>
> 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?
>
>From the allocation API's perspective, both GFP flags are supported. The
only restriction is that memory cannot come from highmem when the
allocation requires a private-to-shared transition.
-aneesh
