Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 1
- [Medium] IP_USER_FLOW support silently ignores the ethtool `proto`
match field.
- [Low] While adding the `int num_hdrs` parameter, the patch also de-
indents the two pre-existing continuation lines of…
--
virtio_net: Implement IPv4 ethtool flow rules
This adds support for ethtool IP_USER_FLOW rules to the virtio_net flow
filter code, building an extra IPv4 selector after the ethernet selector
and matching on source/destination address and tos. The ethernet selector
is forced to match ethertype IPv4 whenever an IPv4 header follows.
(No Link: tags in the commit header.)
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6c07eab047afe..c33bbdda75590 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
[ ... ]
> @@ -5809,6 +5810,39 @@ static bool validate_eth_mask(const struct virtnet_ff
> *ff,
> return true;
> }
>
> +static bool validate_ip4_mask(const struct virtnet_ff *ff,
> + const struct virtio_net_ff_selector *sel,
> + const struct virtio_net_ff_selector *sel_cap)
> +{
[ ... ]
> + if (mask->protocol &&
> + !check_mask_vs_cap(&mask->protocol, &cap->protocol,
> + sizeof(u8), partial_mask))
> + return false;
> +
[ ... ]
> @@ -5820,11 +5854,41 @@ static bool validate_mask(const struct virtnet_ff *ff,
[ ... ]
> +static void parse_ip4(struct iphdr *mask, struct iphdr *key,
> + const struct ethtool_rx_flow_spec *fs)
> +{
> + const struct ethtool_usrip4_spec *l3_mask = &fs->m_u.usr_ip4_spec;
> + const struct ethtool_usrip4_spec *l3_val = &fs->h_u.usr_ip4_spec;
> +
> + if (l3_mask->ip4src) {
> + put_unaligned(l3_mask->ip4src, &mask->saddr);
> + put_unaligned(l3_val->ip4src, &key->saddr);
> + }
> +
> + if (l3_mask->ip4dst) {
> + put_unaligned(l3_mask->ip4dst, &mask->daddr);
> + put_unaligned(l3_val->ip4dst, &key->daddr);
> + }
> +
> + if (l3_mask->tos) {
> + mask->tos = l3_mask->tos;
> + key->tos = l3_val->tos;
> + }
> +}
[ ... ]
> @@ -6016,8 +6091,9 @@ static void calculate_flow_sizes(struct
> ethtool_rx_flow_spec *fs,
> }
>
> static void setup_eth_hdr_key_mask(struct virtio_net_ff_selector *selector,
> - u8 *key,
> - const struct ethtool_rx_flow_spec *fs)
> + u8 *key,
> + const struct ethtool_rx_flow_spec *fs,
> + int num_hdrs)
[Severity: Low]
This isn't a bug, but the two pre-existing continuation lines lost one
space of indentation here while the new num_hdrs parameter was added.
"static void setup_eth_hdr_key_mask(" puts the open parenthesis one column
further right, so the continuation lines no longer line up with it and
checkpatch reports "Alignment should match open parenthesis". Was the
re-indent intentional? Only the "int num_hdrs" line seems to need
touching. The same alignment persists through the end of the series.
> {
> struct ethhdr *eth_m = (struct ethhdr *)&selector->mask;
> struct ethhdr *eth_k = (struct ethhdr *)key;
> @@ -6025,8 +6101,35 @@ static void setup_eth_hdr_key_mask(struct
> virtio_net_ff_selector *selector,
[ ... ]
> +static int setup_ip_key_mask(struct virtio_net_ff_selector *selector,
> + u8 *key,
> + const struct ethtool_rx_flow_spec *fs)
> +{
> + struct iphdr *v4_m = (struct iphdr *)&selector->mask;
> + struct iphdr *v4_k = (struct iphdr *)key;
> +
> + selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
> + selector->length = sizeof(struct iphdr);
> +
> + 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_ip4(v4_m, v4_k, fs);
> +
> + return 0;
> }
[Severity: Medium]
What happens to h_u.usr_ip4_spec.proto here? Every other unsupported
IPv4 field is rejected, but a non-zero h_u.usr_ip4_spec.proto is neither
rejected nor programmed.
include/uapi/linux/ethtool.h documents the field for struct
ethtool_usrip4_spec as:
* @proto: Transport protocol number; mask must be 0
So a conforming userspace passes the protocol in h_u with a zero mask in
m_u, which is exactly the combination that passes all five checks above.
parse_ip4() then only copies ip4src, ip4dst and tos, leaving
key->protocol and mask->protocol zero in the zeroed key/classifier
buffers.
Does this mean
ethtool -U ethX flow-type ip4 src-ip 192.168.51.101 l4proto 6 action -1
installs a rule that matches ethertype IPv4 plus src-ip only, and
therefore drops every protocol from that address rather than just TCP,
without returning an error? Would it be better to either program
iphdr->protocol from h_u.usr_ip4_spec.proto, or return -EINVAL when it is
non-zero?
There also seems to be a disagreement between the two new functions: the
protocol check in validate_ip4_mask()
if (mask->protocol &&
!check_mask_vs_cap(&mask->protocol, &cap->protocol,
sizeof(u8), partial_mask))
return false;
looks unreachable for IP_USER_FLOW, since a non-zero
m_u.usr_ip4_spec.proto always returns -EINVAL in setup_ip_key_mask()
first and parse_ip4() never sets mask->protocol. Is that intended?
Checking the rest of the series, at 04f0a1ab the behaviour looks
unchanged: setup_ip_key_mask() still returns -EINVAL for
m_u.usr_ip4_spec.proto with num_hdrs == 2, parse_ip4() still never writes
iphdr->protocol, and the protocol key is only filled in for
TCP_V4_FLOW/UDP_V4_FLOW.
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com
