Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] The error path for a failed second hypercall erroneously frees pages already deposited to the hypervisor and double-frees `lastpfn`. - [High] If the deposit hypercall fails with `U64_MAX`, the error handler skips freeing pages, leaking up to 511 pages per call. --
commit 03b27f8f1637e85a6319eb9bbc84a63e9b9df5c5 Author: Mukesh R <[email protected]> mshv: Redesign hypervisor memory deposit logic This commit redesigns the hypervisor memory deposit logic to improve performance and fix contiguous range requirements. It changes the logic to attempt allocating a 2MB contiguous range by default and reduces the allocations per deposit. > diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c > index 57864bb5bcd8c..0ebede0bc8b4f 100644 > --- a/drivers/hv/hv_proc.c > +++ b/drivers/hv/hv_proc.c [ ... ] > +static int hv_call_deposit_pages(int node, u64 partition_id, bool contiguous) > +{ > + struct hv_deposit_memory *hc_input; > + int i, rc, num_pages; > + u64 status, *pfna, lastpfn = 0; > + bool trunc_extra = false; [ ... ] > + if (lastpfn) { > + hc_input->gpa_page_list[0] = lastpfn; > + status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY, 1, 0, > + hc_input, NULL); > + > + if (!hv_result_success(status)) { > + if (contiguous) > + goto err_free_dep_pages; > + > + /* We deposited lot earlier, so give it a go */ > + __free_page(pfn_to_page(lastpfn)); > + } > + } > + > + free_page((unsigned long)hc_input); > + return 0; > + > +err_free_dep_pages: > + hv_status_err(status, "\n"); > + rc = hv_result_to_errno(status); > + > + for (i = hv_repcomp(status); i < num_pages; i++) > + __free_page(pfn_to_page(pfna[i])); [Severity: High] If hv_do_rep_hypercall() fails and returns U64_MAX, doesn't hv_repcomp(status) evaluate to 4095? Since 4095 is always greater than num_pages (which is capped at 511), this loop condition would evaluate to false immediately, bypassing the cleanup and causing an unconditional leak of the allocated pages. [Severity: Critical] If the first hypercall succeeds but the second hypercall fails while contiguous is true, the code jumps to err_free_dep_pages. At this point, num_pages is still from the first hypercall, but status is from the failed second hypercall, so hv_repcomp(status) is likely 0. Does this mean the loop will iterate from 0 to 511, mistakenly returning pages to the buddy allocator that the hypervisor successfully assumed ownership of during the first hypercall? Furthermore, because hc_input->gpa_page_list[0] was replaced with lastpfn just before the second hypercall, the loop will free lastpfn at index 0. Could the subsequent block: > + if (lastpfn) > + __free_page(pfn_to_page(lastpfn)); then free lastpfn a second time, resulting in a double-free? > + > +out_free: > + free_page((unsigned long)hc_input); > + return rc; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
