Re: [PATCH net v2] hv_netvsc: use kmap_local_page in netvsc_copy_to_send_buf

[sashiko-bot@kernel.org]

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 <leontyevantony@gmail.com>

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/20260603172543.19230-1-leontyevantony@gmail.com?part=1