Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Failing to restore direct map entries leads to unconditionally freeing unmapped pages into the buddy allocator, causing kernel crashes upon reallocation. - [Low] The patch calls a non-existent kernel function `set_direct_map_valid_noflush`, which will cause a build failure. Pre-existing issues: - [High] Partial success of HVCALL_DEPOSIT_MEMORY causes deposited pages to be erroneously freed back to the buddy allocator. -- commit 73ea756bd40436738445dd3d28edbd2b8f61584f Author: Magnus Kulke <[email protected]> drivers/hv: remove deposited pages from direct map This patch removes deposited pages from the kernel direct map before passing them to the hypervisor, and restores them when the hypervisor returns them. This aims to prevent kernel panics if code like load_unaligned_zeropad() accidentally accesses these deposited pages. > diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c > index 57b2c64197cb9..3ab02adf7efb4 100644 > --- a/drivers/hv/hv_proc.c > +++ b/drivers/hv/hv_proc.c [ ... ] > +static int hv_deposit_update_direct_map(struct page **pages, int *counts, > + int num_allocations, bool valid) > +{ > + int i, err, ret = 0; > + > + for (i = 0; i < num_allocations; ++i) { > + err = set_direct_map_valid_noflush(pages[i], counts[i], valid); [Severity: Low] Does this code call a function that doesn't exist? The kernel API does not appear to have a set_direct_map_valid_noflush() function that takes three arguments, which will cause a build failure. > + if (err && !ret) > + ret = err; > + } > + > + if (valid) > + return ret; [ ... ] > @@ -90,12 +125,15 @@ int hv_call_deposit_pages(int node, u64 partition_id, > u32 num_pages) > if (!hv_result_success(status)) { > hv_status_err(status, "\n"); > ret = hv_result_to_errno(status); > - goto err_free_allocations; > + goto err_restore_direct_map; > } > > ret = 0; > goto free_buf; > > +err_restore_direct_map: > + hv_deposit_update_direct_map(pages, counts, num_allocations, true); > + > err_free_allocations: > for (i = 0; i < num_allocations; ++i) { > base_pfn = page_to_pfn(pages[i]); [Severity: High] This isn't a bug introduced by this patch, but does this error path properly handle partially deposited pages? If the HVCALL_DEPOSIT_MEMORY hypercall is interrupted or partially fails, the hypervisor may have already taken ownership of some pages. The code does not appear to check hv_repcomp(status) for partially deposited pages before freeing them all to the buddy allocator. If the kernel dispenses these pages again, any access could trigger a #GP fault from the hypervisor. [Severity: High] If restoring the direct map fails (for instance, due to an ENOMEM when trying to split page tables), the error from hv_deposit_update_direct_map() is ignored and execution proceeds to err_free_allocations. Could this free unmapped pages to the buddy allocator, causing a crash when the kernel later allocates and accesses them? > for (j = 0; j < counts[i]; ++j) > __free_page(pfn_to_page(base_pfn + j)); > } [ ... ] > @@ -110,6 +148,19 @@ int hv_call_deposit_pages(int node, u64 partition_id, > u32 num_pages) > } > EXPORT_SYMBOL_GPL(hv_call_deposit_pages); > > +/* > + * Put withdrawn pages back in the direct map. Counterpart to the direct map > + * removal done by hv_call_deposit_pages(). > + */ > +void hv_restore_withdrawn_pages(const u64 *pfns, int count) > +{ > + int i; > + > + for (i = 0; i < count; ++i) > + set_direct_map_valid_noflush(pfn_to_page(pfns[i]), 1, true); [Severity: Low] Does this code call a non-existent function? Similar to the other location in hv_deposit_update_direct_map(), this appears to use a non-existent set_direct_map_valid_noflush() API with 3 arguments, which will break the build. [Severity: High] Does this ignore the return value of the mapping restoration? If the direct map restoration fails, the pages would remain unmapped in the kernel's direct map. Ignoring this failure can cause the pages to be freed unmapped into the buddy allocator by the caller. > +} > +EXPORT_SYMBOL_GPL(hv_restore_withdrawn_pages); > + > int hv_deposit_memory_node(int node, u64 partition_id, > u64 hv_status) > { > diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c > index cb55d4d4be2e5..8c21ad4ba3b07 100644 > --- a/drivers/hv/mshv_root_hv_call.c > +++ b/drivers/hv/mshv_root_hv_call.c > @@ -69,6 +69,8 @@ int hv_call_withdraw_memory(u64 count, int node, u64 > partition_id) > > completed = hv_repcomp(status); > > + hv_restore_withdrawn_pages(output_page->gpa_page_list, > completed); > + > for (i = 0; i < completed; i++) > __free_page(pfn_to_page(output_page->gpa_page_list[i])); > [Severity: High] If hv_restore_withdrawn_pages() encounters an error, this loop will return unmapped pages to the buddy allocator. Could this lead to a #PF and a kernel panic when the buddy allocator dispenses these unmapped pages to another subsystem? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
