Re: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state

[netdev-bot+sashiko@kernel.org]

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

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

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] This patch silently fixes a packet-drop regression introduced
  by the previous commit in the series, "net: resolve uncommon partial…
- [Medium] tun_get_ethtool_stats() assigns `data[0] =
  atomic_long_read(&tun->rx_csum_sw);` straight into the u64 ethtool…
- [Low] The trailer `Fixes: f43798c27684 ("tun: Allow GSO using
  virtio_net_hdr")` names a July 2008 commit by Rusty Russell (verified…

--

net: account for ingress-resolved virtio net hdr state

After this commit, virtio_net_hdr_to_skb() returns 1 when it completes an
uncommon checksum in software at ingress. The converter call sites in tap,
tun, virtio_net and af_packet now test for a negative errno. tun counts
these events per device as rx_csum_sw and exposes the counter through
ethtool -S.

> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

[Severity: Low]
Is this the right Fixes: target? f43798c27684 is the 2008 commit that
added GSO via virtio_net_hdr to tun.

Software checksum completion in the converter first appears in the
previous patch in this series, "net: resolve uncommon partial checksums
at ingress". So does the positive return value this patch accounts for.
At the baseline there is no skb_checksum_help() call in
include/linux/virtio_net.h.

Stable tooling that follows this tag could offer the patch to every
stable branch since 2.6.27. On those branches the "return err ?: 1"
hunk has nothing to apply to, and rx_csum_sw would always read 0.

Should the tag be dropped? If it is meant to cover the call site fix
below, should it point at the previous commit in this series instead?

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 8b82a7fe6ec7f..b8961229737fd 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -1935,11 +1936,19 @@ 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;
>  	}

[Severity: Medium]
Does this change also fix a regression from the previous patch in the
series, "net: resolve uncommon partial checksums at ingress"?

That commit already made virtio_net_hdr_tnl_to_skb() return 1 on the
UDP tunnel path after a successful software checksum:

include/linux/virtio_net.h:virtio_net_hdr_tnl_to_skb() {
    ...
	ret = skb_checksum_help(skb);
	return ret ?: 1;
}

At that commit, the tunnel converter callers still treated any nonzero
return as a failure:

tun_get_user():
	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
		atomic_long_inc(&tun->rx_frame_errors);
		err = -EINVAL;
		goto free_skb;
	}

tun_xdp_one() had the same pattern. virtnet_receive_done() went to
frame_err and printed a "bad gso" warning.

Take a UDP tunnel header with these properties:

  - NEEDS_CSUM is set.
  - The payload is no larger than gso_size, so skb_is_gso() is false.
  - The inner csum offsets fail virtio_net_csum_partial_ok_at_offset().

With only the previous commit applied, such a packet would have its
checksum completed and then be dropped as a frame error. tun write()
would return -EINVAL for it.

This commit message describes the "< 0" changes only as accounting
plumbing. To keep the series bisectable, could the call site changes be
folded into "net: resolve uncommon partial checksums at ingress"?
Alternatively, could this commit message say that it fixes that commit?

> +	if (err > 0)
> +		atomic_long_inc(&tun->rx_csum_sw);

[ ... ]

> @@ -3739,7 +3751,35 @@ static void tun_get_channels(struct net_device *dev,

[ ... ]

> +static void tun_get_ethtool_stats(struct net_device *dev,
> +				  struct ethtool_stats *stats, u64 *data)
> +{
> +	struct tun_struct *tun = netdev_priv(dev);
> +
> +	data[0] = atomic_long_read(&tun->rx_csum_sw);

[Severity: Medium]
On 32-bit builds, atomic_long_t is an atomic_t and atomic_long_read()
returns a signed long. Once rx_csum_sw passes 2^31, can the implicit
conversion to u64 sign-extend the value? ethtool -S would then report
something like 0xffffffff80000000.

tun_net_get_stats64() in the same file avoids this for the sibling
counter:

	stats->rx_frame_errors +=
		(unsigned long)atomic_long_read(&tun->rx_frame_errors);

Should this read use the same (unsigned long) cast?

> +}

[ ... ]

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