Re: [net-next,v23,11/14] virtio_net: Implement IPv4 ethtool flow rules

[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] 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