Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] mm: memory leak of successfully restored pages on error path in hv_call_deposit_pages() - [Low] build: invocation of non-existent function set_direct_map_valid_noflush() --
commit 702a398988acd774f1ea0597061a05c15e6ab245 Author: Magnus Kulke <[email protected]> drivers/hv: remove deposited pages from direct map This commit removes deposited pages from the kernel direct map before passing them to the hypervisor to prevent #GP faults on accidental access. It also restores the direct map entries when the pages are returned. > diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c > index 57b2c64197cb9..b056051a5ac59 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 cause a build failure? The function set_direct_map_valid_noflush() does not appear to exist in the kernel API. The set_memory.h header provides set_direct_map_invalid_noflush() and set_direct_map_default_noflush(), rather than a single multiplexed function with a boolean flag. > + if (err && !ret) > + ret = err; > + } > + > + if (valid) > + return ret; [ ... ] > @@ -90,12 +125,23 @@ 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: > + /* > + * We don't want to return pages to the allocator if weren't able to > + * mark them valid in the direct map. > + */ > + if (hv_deposit_update_direct_map(pages, counts, num_allocations, true)) > { > + WARN(1, "leaking %d page block(s) that could not be set to > valid\n", > + num_allocations); > + goto free_buf; > + } [Severity: Medium] Does this error path leak successfully restored pages? In hv_deposit_update_direct_map(), batch processing continues even if one block fails, but the function returns the first error it encounters. If a non-zero error is returned here, the goto skips the err_free_allocations loop. This permanently leaks all pages in the batch, including the ones that were successfully marked as valid in the direct map again. > + > err_free_allocations: > for (i = 0; i < num_allocations; ++i) { > base_pfn = page_to_pfn(pages[i]); [ ... ] > @@ -110,6 +156,41 @@ 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, j, end, n, ret; > + struct page *page; > + > + for (i = 0; i < count; i = end) { > + page = pfn_to_page(pfns[i]); [ ... ] > + for (j = i; j < end; ++j) { > + page = pfn_to_page(pfns[j]); > + /* > + * Retry individually on failed batch. We don't want > + * to return pages to the allocator that we couldn't > mark > + * valid in the direct map. > + */ > + if (ret && > + WARN_ON_ONCE(set_direct_map_valid_noflush(page, 1, > true))) [Severity: Low] As mentioned above, set_direct_map_valid_noflush() does not exist in the kernel API. Should this call set_direct_map_default_noflush() instead? > + continue; > + __free_page(page); > + } > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
