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
