Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Critical] `memcpy()` overruns the single-page mapping created by `kmap_local_page()` when an SKB fragment spans multiple pages. Pre-existing issues: - [High] Concurrent writers to the per-channel `tx_stats.syncp` will corrupt the statistics seqcount, deadlocking readers on 32-bit systems. -- commit f885c2ff3318238c254b9deff5078d8c10f79e15 Author: Anton Leontev <[email protected]> hv_netvsc: use kmap_local_page in netvsc_copy_to_send_buf This commit replaces phys_to_virt() with kmap_local_page() to safely access highmem SKB fragments on 32-bit systems, preventing page faults. > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index 59e95341f9b1e..2038d9f5c9f9a 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c [ ... ] > @@ -965,11 +966,14 @@ static void netvsc_copy_to_send_buf(struct > netvsc_device *net_device, > } > > for (i = 0; i < page_count; i++) { > - char *src = phys_to_virt(pb[i].pfn << HV_HYP_PAGE_SHIFT); > - u32 offset = pb[i].offset; > + phys_addr_t paddr = pb[i].pfn << HV_HYP_PAGE_SHIFT; > + struct page *page = phys_to_page(paddr); > + u32 offset = offset_in_page(paddr) + pb[i].offset; > u32 len = pb[i].len; > + char *src = kmap_local_page(page); > > memcpy(dest, (src + offset), len); [Severity: Critical] Does this memcpy() risk overrunning the single-page mapping? kmap_local_page() creates a temporary thread-local virtual mapping of exactly one native page (4096 bytes on x86). However, the length of the fragment (len) can exceed a single page. For example, init_page_array() maps the entire linear skb->data into a single hv_page_buffer entry, and skb_headlen() can exceed 4096 bytes with jumbo frames or GRO/TSO packets. If pb[i].len + offset > PAGE_SIZE, this will read beyond the mapped 4096-byte window and access unmapped fixmap slots, potentially triggering a page fault on the transmit softirq path. > + kunmap_local(src); > dest += len; > } > [Severity: High] This is a pre-existing issue, but can concurrent writers to the per-channel tx_stats.syncp corrupt the statistics seqcount? Looking at netvsc_ndoxdp_xmit() in drivers/net/hyperv/netvsc_bpf.c, it selects a transmit queue using q_idx = smp_processor_id() % queues and updates the stats: drivers/net/hyperv/netvsc_bpf.c:netvsc_ndoxdp_xmit() { ... tx_stats = &nvsc_dev->chan_table[q_idx].tx_stats; u64_stats_update_begin(&tx_stats->syncp); tx_stats->xdp_xmit += count; u64_stats_update_end(&tx_stats->syncp); ... } If num_online_cpus() > real_num_tx_queues, multiple CPUs can simultaneously execute XDP transmit for the same queue. Additionally, the NAPI TX completion path independently updates the same syncp on the CPU bound to the VMBus interrupt: drivers/net/hyperv/netvsc.c:netvsc_send_tx_complete() { ... tx_stats = &net_device->chan_table[q_idx].tx_stats; u64_stats_update_begin(&tx_stats->syncp); tx_stats->packets += packet->total_packets; tx_stats->bytes += packet->total_bytes; u64_stats_update_end(&tx_stats->syncp); ... } Since u64_stats_update_begin() does not use locks, concurrent updates could leave the seqcount odd forever. This would cause readers like ndo_get_stats64() to spin infinitely in u64_stats_fetch_retry(), deadlocking the CPU on 32-bit systems. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
