Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] hv/mshv: unkillable task in mshv_partition_ioctl_import_isolated_pages() - [High] hv/mshv: soft lockups in unbounded memory region processing loops --
commit c18842e6b577b1aad2cca89b02c8c1764f3d001e Author: Wei Liu <[email protected]> mshv: wire SEV-SNP partition ioctls This commit wires the SEV-SNP UAPI into partition creation, memory ownership, and isolated-page import operations. It introduces tracking for host READ/WRITE permissions and ownership for every pinned 4K page. It also implements the necessary UAPI layers to handle guest requests and manage AP creation. > diff --git a/drivers/hv/mshv_regions.c b/drivers/hv/mshv_regions.c > index b08b2709bf115..8920340942ce5 100644 > --- a/drivers/hv/mshv_regions.c > +++ b/drivers/hv/mshv_regions.c [ ... ] > +int mshv_region_check_duplicate_pfns(struct mshv_mem_region *region) > +{ > + struct mshv_partition *partition = region->partition; > + struct mshv_mem_region *existing; > + DEFINE_XARRAY(pfns); > + u64 i; > + int ret = 0; > + > + lockdep_assert_held(&partition->pt_mutex); > + > + /* > + * Use a temporary index so alias rejection has no permanent per-page > + * memory cost. xa_insert() also rejects duplicate PFNs within region. > + */ > + for (i = 0; i < region->nr_pages; i++) { > + ret = xa_insert(&pfns, page_to_pfn(region->mreg_pages[i]), [Severity: High] Does this code risk soft lockups during unbounded processing loops? If a user maps a massive memory region, this loop can iterate over millions of pages doing xa_insert() without yielding. Would it be better to add a cond_resched() periodically? [ ... ] > +int > +mshv_apply_region_host_access(struct mshv_mem_region *region, u64 index, > + u64 count, u32 host_access, u32 flags, > + struct mshv_host_access_result *result) > +{ > + u64 i; > + > + mshv_host_access_normalize_result(result, count); > + for (i = 0; i < result->completed; i++) { > + u8 old_state = mshv_region_host_access_get(region, index + i); [Severity: High] Similarly, can this loop cause a CPU stall if updating a large region? It modifies the bitmaps for potentially millions of completed pages without yielding. [ ... ] > +int mshv_region_share(struct mshv_mem_region *region) > +{ > + u64 offset = 0; > + int state_error = 0; > + int ret; > + > + while (offset < region->nr_pages) { > + u64 count; > + u32 flags; > + u8 state = mshv_region_host_access_get(region, offset); [ ... ] > + /* One call may cover only pages with identical restore state. > */ > + for (count = 1; offset + count < region->nr_pages; count++) { > + u8 next = mshv_region_host_access_get(region, > + offset + count); [Severity: High] Could this inner loop also cause soft lockups when scanning the entire region size to find contiguous state blocks without yielding? > diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c > index cb57275293557..c2a629bdef339 100644 > --- a/drivers/hv/mshv_root_hv_call.c > +++ b/drivers/hv/mshv_root_hv_call.c [ ... ] > @@ -1213,31 +1284,34 @@ int hv_call_modify_spa_host_access(u64 partition_id, > struct page **pages, > page_count >>= large_shift; > } > > + ret = mshv_validate_host_access_flags(flags, acquire); > + if (ret) > + return ret; > + > while (done < page_count) { > - ulong i, completed, remain = page_count - done; > - int rep_count = min(remain, > - > HV_MODIFY_SPARSE_SPA_PAGE_HOST_ACCESS_MAX_PAGE_COUNT); > + u64 i, completed, remain = page_count - done; > + unsigned int rep_count = min_t(u64, remain, > + HV_MODIFY_SPARSE_SPA_PAGE_HOST_ACCESS_MAX_PAGE_COUNT); [Severity: High] Is there a risk of monopolizing the CPU here when batching hypercalls for an extremely large page count without periodically yielding? > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c > index de6519ac677b3..cb090c380b97f 100644 > --- a/drivers/hv/mshv_root_main.c > +++ b/drivers/hv/mshv_root_main.c [ ... ] > +static long > +mshv_partition_ioctl_import_isolated_pages(struct mshv_partition *partition, > + void __user *user_args) > +{ > + struct mshv_import_isolated_pages args; > + u64 __user *guest_pfns; > + u64 *pages = NULL; > + u64 input_completed; > + u64 offset = 0; > + long ret = 0; [ ... ] > + while (offset < args.page_count) { > + u64 count = min_t(u64, args.page_count - offset, > + MSHV_USER_PAGE_BATCH_SIZE); > + u64 completed = 0; [ ... ] > + ret = hv_call_import_isolated_pages(partition->pt_id, pages, > + count, &completed, > + args.page_type, > + args.page_size, > + mshv_async_hvcall_handler, > + partition); [ ... ] > + offset += completed; > + if (ret) { > + if (ret == -EPROTO) > + mshv_quarantine_partition(partition); > + break; > + } > + if (completed != count) { > + ret = -EPROTO; > + mshv_quarantine_partition(partition); > + break; > + } > + cond_resched(); > + } [Severity: High] Can an unkillable task hold the partition mutex indefinitely here? Although this loop calls cond_resched(), it processes based on an unbounded args.page_count without checking for fatal signals like fatal_signal_pending(). If a user passes a massive page count, could this loop run for hours without responding to SIGKILL and effectively cause a denial of service? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
