Re: [net-next,v23,12/14] virtio_net: Add support for IPv6 ethtool steering

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The new IPv6 code overlays `struct ipv6hdr` (alignment 4, from
  the embedded struct in6_addr / __be32 s6_addr32[]) on byte-packed…
- [Low] Commit message claims "Implement support for IPV6_USER_FLOW type
  rules" but the implementation only supports src-ip/dst-ip matching:…

--

virtio_net: Add support for IPv6 ethtool steering

This adds IPV6_USER_FLOW support to the virtio_net ethtool flow filter
code: a new validate_ip6_mask() for the IPv6 selector mask, a new
parse_ip6() to build the key/mask pair, and IPv6 handling in
setup_ip_key_mask(), supported_flow_type() and calculate_flow_sizes().

Two questions below, one about alignment of the new IPv6 accesses and one
about the scope described in the changelog.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index c33bbdda75590..bfe8f0bc2581a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -5843,6 +5843,40 @@ static bool validate_ip4_mask(const struct virtnet_ff *ff,
>  	return true;
>  }
>  
> +static bool validate_ip6_mask(const struct virtnet_ff *ff,
> +			      const struct virtio_net_ff_selector *sel,
> +			      const struct virtio_net_ff_selector *sel_cap)
> +{
> +	bool partial_mask = !!(sel_cap->flags & VIRTIO_NET_FF_MASK_F_PARTIAL_MASK);
> +	struct in6_addr tmp;
> +	struct ipv6hdr *cap, *mask;
> +
> +	cap = (struct ipv6hdr *)&sel_cap->mask;
> +	mask = (struct ipv6hdr *)&sel->mask;
> +
> +	/* mask->saddr/daddr may be unaligned; copy to aligned tmp for
> +	 * ipv6_addr_any().
> +	 */
> +	memcpy(&tmp, &mask->saddr, sizeof(tmp));
> +	if (!ipv6_addr_any(&tmp) &&
> +	    !check_mask_vs_cap(&mask->saddr, &cap->saddr,
> +			       sizeof(cap->saddr), partial_mask))
> +		return false;
> +
> +	memcpy(&tmp, &mask->daddr, sizeof(tmp));
> +	if (!ipv6_addr_any(&tmp) &&
> +	    !check_mask_vs_cap(&mask->daddr, &cap->daddr,
> +			       sizeof(cap->daddr), partial_mask))
> +		return false;
> +
> +	if (mask->nexthdr &&
> +	    !check_mask_vs_cap(&mask->nexthdr, &cap->nexthdr,
> +			       sizeof(cap->nexthdr), partial_mask))
> +		return false;
> +
> +	return true;
> +}
> +

[ ... ]

> @@ -5884,11 +5921,33 @@ static void parse_ip4(struct iphdr *mask, struct iphdr *key,
>  	}
>  }
>  
> +static void parse_ip6(struct ipv6hdr *mask, struct ipv6hdr *key,
> +		      const struct ethtool_rx_flow_spec *fs)
> +{
> +	const struct ethtool_usrip6_spec *l3_mask = &fs->m_u.usr_ip6_spec;
> +	const struct ethtool_usrip6_spec *l3_val  = &fs->h_u.usr_ip6_spec;
> +
> +	if (!ipv6_addr_any((struct in6_addr *)l3_mask->ip6src)) {
> +		memcpy(&mask->saddr, l3_mask->ip6src, sizeof(mask->saddr));
> +		memcpy(&key->saddr, l3_val->ip6src, sizeof(key->saddr));
> +	}
> +
> +	if (!ipv6_addr_any((struct in6_addr *)l3_mask->ip6dst)) {
> +		memcpy(&mask->daddr, l3_mask->ip6dst, sizeof(mask->daddr));
> +		memcpy(&key->daddr, l3_val->ip6dst, sizeof(key->daddr));
> +	}
> +}

[Severity: Medium]
Are the destinations of these copies guaranteed to be 4-byte aligned?

struct in6_addr contains __be32 s6_addr32[4], so struct ipv6hdr has
4-byte alignment, but the buffers these pointers overlay are byte packed
in the virtio flow filter layout:

  build_and_insert() -> setup_ip_key_mask(next_selector(selector),
                                          key + sizeof(struct ethhdr), fs)

For the classifier mask, the first selector is an 8-byte
struct virtio_net_ff_selector header plus a 14-byte struct ethhdr mask,
so the second selector starts at obj+30 and its mask[] at obj+38, which
makes &v6_m->saddr land at obj+46, i.e. 2 mod 4. The key buffer is the
same: it is offset by sizeof(struct ethhdr), so &v6_k->saddr is
base+22, again 2 mod 4.

The IPv4 sibling in the same file goes through the unaligned helpers for
exactly these members:

drivers/net/virtio_net.c:parse_ip4() {
	if (l3_mask->ip4src) {
		put_unaligned(l3_mask->ip4src, &mask->saddr);
		put_unaligned(l3_val->ip4src, &key->saddr);
	}
	...
}

and validate_ip4_mask() uses get_unaligned(&mask->saddr) for the read
side. Since the pointer expressions here are typed struct in6_addr *,
can the compiler expand these 16-byte copies into word or multi-word
accesses (for example LDRD/STRD or LDM/STM on 32-bit ARM) on an address
that is 2 mod 4, and fault on strict-alignment architectures?

The comment in validate_ip6_mask() already notes "mask->saddr/daddr may
be unaligned", but the aligned tmp only fixes the destination of that one
copy; the source (&mask->saddr) and the copies in parse_ip6() are still
misaligned accesses through a 4-byte-aligned pointer type. Would using
the unaligned accessors, or plain u8 pointers, for these fields keep the
IPv6 path consistent with the IPv4 one?

[ ... ]

> @@ -6114,20 +6179,38 @@ static int setup_ip_key_mask(struct virtio_net_ff_selector *selector,
>  			     u8 *key,
>  			     const struct ethtool_rx_flow_spec *fs)
>  {
> +	struct ipv6hdr *v6_m = (struct ipv6hdr *)&selector->mask;
>  	struct iphdr *v4_m = (struct iphdr *)&selector->mask;
> +	struct ipv6hdr *v6_k = (struct ipv6hdr *)key;
>  	struct iphdr *v4_k = (struct iphdr *)key;
>  
> -	selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
> -	selector->length = sizeof(struct iphdr);
> +	if (has_ipv6(fs->flow_type)) {
> +		selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV6;
> +		selector->length = sizeof(struct ipv6hdr);
> +
> +		/* exclude tclass, it's not exposed directly in struct ipv6hdr */
> +		if (fs->h_u.usr_ip6_spec.l4_4_bytes ||
> +		    fs->m_u.usr_ip6_spec.l4_4_bytes ||
> +		    fs->h_u.usr_ip6_spec.tclass ||
> +		    fs->m_u.usr_ip6_spec.tclass ||
> +		    fs->h_u.usr_ip6_spec.l4_proto ||
> +		    fs->m_u.usr_ip6_spec.l4_proto)
> +			return -EINVAL;
>  
> -	if (fs->h_u.usr_ip4_spec.l4_4_bytes ||
> -	    fs->h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4 ||
> -	    fs->m_u.usr_ip4_spec.l4_4_bytes ||
> -	    fs->m_u.usr_ip4_spec.ip_ver ||
> -	    fs->m_u.usr_ip4_spec.proto)
> -		return -EINVAL;
> +		parse_ip6(v6_m, v6_k, fs);

[Severity: Low]
This isn't a bug, but should the changelog say which ip6 match fields are
actually implemented?

The commit message says:

    Implement support for IPV6_USER_FLOW type rules.

but setup_ip_key_mask() rejects any non-zero tclass, l4_proto or
l4_4_bytes, in the value as well as in the mask, and parse_ip6() only
ever fills in saddr/daddr. So of the struct ethtool_usrip6_spec fields
only the addresses work, while supported_flow_type() advertises
IPV6_USER_FLOW as a whole:

	case IPV6_USER_FLOW:
		return true;

A request such as:

    ethtool -U ens9 flow-type ip6 src-ip fe80::2 tclass 8 action 3

fails with "Invalid argument" and there is nothing in the log describing
the limitation.

There is also an asymmetry with the IPv4 support added in the previous
patch: parse_ip4() does translate the IPv4 tos field:

	if (l3_mask->tos) {
		mask->tos = l3_mask->tos;
		key->tos = l3_val->tos;
	}

while the IPv6 path refuses the equivalent tclass, even though
struct ipv6hdr carries the traffic class in the priority/flow_lbl bits.
Could the changelog mention that only src-ip/dst-ip matching is
supported for ip6 flows, and why tclass parity with the IPv4 path is not
provided?

> +	} else {
> +		selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
> +		selector->length = sizeof(struct iphdr);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com