Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Memory leak of all un-deposited pages when `hv_do_rep_hypercall` returns `U64_MAX`. --
commit 68b286f4de61fdc1a888242cfc4b1a1ea3d187dc Author: Mukesh R <[email protected]> mshv: Redesign hypervisor memory deposit logic This commit redesigns the hypervisor memory deposit logic to optimize performance and address preexisting issues. It defaults to allocating and depositing 2M chunks when possible to reduce overhead and avoids unnecessary interrupt disabling during the process. > diff --git a/drivers/hv/hv_proc.c b/drivers/hv/hv_proc.c > index 57864bb5bcd8c..df39a5c587ca2 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) > +{ [ ... ] > + /* Not using hyperv_pcpu_input_arg, so no need to disable interrupts */ > + > + status = hv_do_rep_hypercall(HVCALL_DEPOSIT_MEMORY, num_pages, 0, > + hc_input, NULL); > + if (!hv_result_success(status)) > + goto err_free_dep_pages; > + > + 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) && hv_repcomp(status) == 0) [Severity: Medium] Will this leak the page pointed to by lastpfn if hv_do_rep_hypercall() returns U64_MAX? If the hypervisor is unavailable or hypercalls are unsupported, hv_do_rep_hypercall() can return the sentinel value U64_MAX, which is explicitly handled by hv_result_to_errno() elsewhere. When status is U64_MAX, the hv_repcomp(status) macro extracts bits 43:32, which results in 4095 instead of 0. Does this cause the condition to evaluate to false and skip freeing the page? > + /* 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++) [Severity: Medium] Will this loop fail to free any pages in the pfna[] array when status is U64_MAX? Similar to the check above, if status is U64_MAX, hv_repcomp(status) evaluates to 4095. Since num_pages is bounded to a maximum of 511 (HV_DEPOSIT_INP_MAX) in this path, the loop initialization sets i = 4095, making the condition i < num_pages immediately false. Does this cause the loop to terminate without executing, silently leaking up to 511 allocated pages per failed deposit attempt? > + __free_page(pfn_to_page(pfna[i])); > + if (lastpfn) > + __free_page(pfn_to_page(lastpfn)); > + > +out_free: > + free_page((unsigned long)hc_input); > + return rc; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
