Re: [PATCH net v5 1/7] net: account for ingress-resolved virtio net hdr state

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The concern: the newly checked virtio_net_hdr_to_skb() call in
  raw_verify_header() (arch/um/drivers/vector_transports.c:213) runs on…
- [Low] This patch quietly includes a real behaviour fix for the UML
  vector raw transport.
- [Low] The concern: at this commit, virtio_net_hdr_to_skb(),
  __virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() in…

--

net: account for ingress-resolved virtio net hdr state

This commit changes callers of virtio_net_hdr_to_skb() and
virtio_net_hdr_tnl_to_skb() to treat only negative return values as errors.
It prepares for a later patch that returns a positive value when the
checksum is completed in software. It also makes the UML vector raw
transport check the return value, and adds a tun ethtool -S counter,
rx_csum_sw, for the positive case.

> diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> index e1fb2a76fdff5..8b75484586635 100644
> --- a/arch/um/drivers/vector_transports.c
> +++ b/arch/um/drivers/vector_transports.c
> @@ -210,7 +210,9 @@ static int raw_verify_header(
>  		return 1;
>  
>  	skb_set_network_header(skb, ETH_HLEN);
> -	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
> +	if (virtio_net_hdr_to_skb(skb, vheader,
> +			virtio_legacy_is_little_endian()) < 0)
> +		return -1;
>  	return 0;
>  }

[Severity: Medium]
Can this drop valid frames at this point in the series? The skb passed in
here has not been trimmed to the received length yet. prep_skb() sizes it
to the whole receive buffer:

arch/um/drivers/vector_kern.c:prep_skb() {
    ...
	skb_put(result, vp->max_packet);
	result->data_len = len - vp->max_packet;
	result->len += len - vp->max_packet;
    ...
}

Both vector_legacy_rx() and vector_mmsg_rx() call verify_header() before
pskb_trim():

arch/um/drivers/vector_kern.c:vector_legacy_rx() {
    ...
				header_check = vp->verify_header(
					vp->header_rxbuffer, skb, vp);
				if (header_check < 0) {
    ...
			pskb_trim(skb, pkt_len - vp->rx_header_size);
    ...
}

That means the length checks in __virtio_net_hdr_to_skb() compare against
the buffer size (65536 with GSO), not the packet length. Take the USO check:

	if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
		return -EINVAL;

With a 64K buffer, this rejects a valid USO frame whose gso_size is below
about 511. Before this patch the error was ignored. Now raw_verify_header()
returns -1, and the caller frees the skb and increments rx_encaps_errors.

The same ordering also lets csum_start/csum_offset values that point past
the real packet, but still inside the buffer, pass validation.

The next patch in the series, "um: vector: trim skbs before verifying the
header", moves pskb_trim() ahead of verify_header() in both RX paths. That
fixes this, so only intermediate bisect points are affected. Could the trim
come before this change, or be folded into it?

[Severity: Low]
This hunk looks like a real behaviour fix for the UML raw transport, not
accounting preparation. The commit message says:

    The um vector raw transport even ignored the return value
    altogether: rejected packets went on into the stack with their
    metadata unvalidated, so check it there as well.

The return value has been ignored since commit 49da7e64f33e ("High
Performance UML Vector Network Driver").

For example, __virtio_net_hdr_to_skb() can call skb_partial_csum_set(),
which sets CHECKSUM_PARTIAL along with csum_start/csum_offset, and then
return -EINVAL from a later check. The old code still delivered that skb.

Should this be a separate UML patch, ordered together with the trim fix
from "um: vector: trim skbs before verifying the header", and carrying:

Fixes: 49da7e64f33e ("High Performance UML Vector Network Driver")

Right now the fix sits in a patch whose subject reads as accounting-only
and which also adds a new tun ethtool string. That makes it hard to pick
up cleanly for stable.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 7cca1a6f72ff4..133923317eae8 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -1939,10 +1940,14 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		break;
>  	}
>  
> -	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
> +	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
> +	if (err < 0) {
>  		atomic_long_inc(&tun->rx_frame_errors);
>  		err = -EINVAL;
>  		goto free_skb;
> +	} else if (err > 0) {
> +		atomic64_inc(&tun->rx_csum_sw);
> +		err = 0;
>  	}

[Severity: Low]
Can err be positive here at this commit? At this revision,
virtio_net_hdr_to_skb(), __virtio_net_hdr_to_skb() and
virtio_net_hdr_tnl_to_skb() in include/linux/virtio_net.h appear to return
only 0 or -EINVAL.

If so, this branch and the matching ret > 0 branch in tun_xdp_one() can't
be reached. The new rx_csum_sw value reported by tun_get_ethtool_stats()
would always read 0.

The later patch "net: resolve uncommon partial checksums at ingress"
documents the tri-state return and adds
__virtio_net_resolve_csum_partial_mismatch(), which returns
skb_checksum_help(skb) ?: 1. That makes these branches live at the end of
the series, so this only affects intermediate bisect points.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com