Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 (2) · Medium: 0 · Low: 2
- [Low] The new `struct gve_device_info` introduces `max_tx_queues` /
`max_rx_queues` members that no code in the tree ever writes or reads,…
- [Low] `gve_set_buf_sizes()`
(drivers/net/ethernet/google/gve/gve_main.c) turns the baseline's…
Pre-existing issues:
- [High] The device-descriptor option parser trusts device-controlled
length metadata and can read far outside the 4096-byte DMA buffer.
- [High] Unbounded device-provided RSS sizes plus a u16 truncation let
the RSS query read past the 4096-byte response buffer and hand the…
--
gve: add struct gve_device_info to hold device properties
This adds struct gve_device_info as the single place holding the
properties negotiated with the device, redirects the AdminQ describe
path to fill it, and moves the code that applies those properties to
struct gve_priv into gve_init_priv().
No Link: tags in the commit header.
A few questions below.
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index c280ff35ee771..021adb9108dff 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -797,6 +797,34 @@ struct gve_ptp {
> struct gve_priv *priv;
> };
>
> +struct gve_device_info {
> + enum gve_queue_format queue_format;
> + u16 default_tx_queues;
> + u16 default_rx_queues;
> + u16 max_tx_queues;
> + u16 max_rx_queues;
^^^^^^^^^^^^^
[Severity: Low]
Is anything writing or reading max_tx_queues and max_rx_queues?
gve_fill_device_info() fills mac, mtu, counters, the default and the
min/max ring sizes and the default tx/rx queue counts, but leaves these two
at zero, and the only consumer of the queue counts, gve_set_num_queues(),
still takes the maximums from priv:
priv->tx_cfg.max_queues =
min_t(int, priv->tx_cfg.max_queues, priv->num_ntfy_blks / 2);
...
if (device_info->default_tx_queues > 0)
priv->tx_cfg.num_queues = min_t(int,
device_info->default_tx_queues,
priv->tx_cfg.num_queues);
In the other direction, this patch removes the last writers of
priv->default_num_queues and priv->default_min_ring_size in struct gve_priv,
but both declarations stay with no readers left. priv->default_min_ring_size
now also shadows the identically named and typed field in gve_device_info
that gve_parse_device_option() writes.
Would it be cleaner to drop those two now unused gve_priv fields, and either
fill max_tx_queues/max_rx_queues here or add them when MailboxQ starts using
them? Following the rest of the series (up to 70ee8341a456) the state is
unchanged: still no writer or reader for either field.
> + u16 default_tx_ring_size;
> + u16 default_rx_ring_size;
> + u16 max_tx_ring_size;
> + u16 max_rx_ring_size;
> + u16 min_tx_ring_size;
> + u16 min_rx_ring_size;
> + u16 max_mtu;
> + u8 mac[ETH_ALEN];
> + u16 max_rx_buffer_size;
> + u16 header_buf_size;
> + u32 max_flow_rules;
> + u16 rss_key_size;
> + u16 rss_lut_size;
> + u16 tx_pages_per_qpl;
> + u16 num_event_counters;
> + u64 max_registered_pages;
> + bool default_min_ring_size;
> + bool nic_timestamp_supported;
> + bool modify_ring_size_enabled;
> + bool cache_rss_config;
> +};
> +
[ ... ]
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index f05f4895f4c74..512349c5517f4 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
[ ... ]
> @@ -190,7 +190,7 @@ void gve_parse_device_option(struct gve_priv *priv,
>
> /* device has not provided min ring size */
> if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE)
> - priv->default_min_ring_size = true;
> + priv->device_info.default_min_ring_size = true;
> break;
[Severity: High]
This isn't a bug introduced by this patch, but since the option parser is
being restructured here: can this parser read outside the 4096-byte DMA
buffer?
gve_adminq_describe_device() allocates the descriptor from priv->adminq_pool,
which is created with GVE_ADMINQ_BUFFER_SIZE (4096), and tells the device
available_length = GVE_ADMINQ_BUFFER_SIZE. Nothing validates
descriptor->total_length, a __be16 that can be up to 65535:
gve_adminq.c:gve_get_next_option() {
option_end = (void *)(option + 1) + be16_to_cpu(option->option_length);
descriptor_end = (void *)descriptor + be16_to_cpu(descriptor->total_length);
return option_end > descriptor_end ? NULL : (struct gve_device_option *)option_end;
}
With total_length set to 65535 and num_device_options >= 2, does the loop in
gve_process_device_options() advance dev_opt up to roughly 61 KB past the end
of the allocation?
gve_parse_device_option() then dereferences option->option_id,
option->option_length and option->required_features_mask at that address, and
stores (void *)(option + 1) into the dev_op_* pointers that
gve_enable_supported_features() dereferences afterwards to set queue format,
MTU, RSS sizes and ring size ranges.
gve_get_next_option() also reads option->option_length before it is
established that a complete 8-byte option header lies inside the buffer.
Would clamping total_length to GVE_ADMINQ_BUFFER_SIZE and bounds checking
each option header before dereferencing it be appropriate here?
[ ... ]
> @@ -946,77 +949,105 @@ static void gve_enable_supported_features(struct gve_priv *priv,
[ ... ]
> if (dev_op_rss_config &&
> (supported_features_mask & GVE_SUP_RSS_CONFIG_MASK)) {
> - priv->rss_key_size =
> + info->rss_key_size =
> be16_to_cpu(dev_op_rss_config->hash_key_size);
> - priv->rss_lut_size =
> + info->rss_lut_size =
> be16_to_cpu(dev_op_rss_config->hash_lut_size);
> - priv->cache_rss_config = false;
> + info->cache_rss_config = false;
[Severity: High]
This is a pre-existing issue as well, not introduced by this patch, but the
sizes recorded here are taken from the device with no maximum and no check
that key plus LUT bytes fit in a response buffer. They reach
priv->rss_key_size / priv->rss_lut_size through gve_init_priv().
gve_adminq_query_rss_config() allocates one 4096-byte adminq_pool object and
passes available_length = GVE_ADMINQ_BUFFER_SIZE, then:
gve_adminq.c:gve_adminq_process_rss_query() {
u16 hash_lut_length;
...
hash_lut_length = priv->rss_lut_size * sizeof(*rxfh->indir);
if (sizeof(*descriptor) + priv->rss_key_size + hash_lut_length != total_memory_length) {
...
return -EINVAL;
}
...
for (i = 0; i < priv->rss_lut_size; i++)
rxfh->indir[i] = be32_to_cpu(lut[i]);
}
With rss_lut_size = 16384, 16384 * 4 == 65536 wraps to 0 in the u16
hash_lut_length, so the check reduces to 8 + rss_key_size ==
total_memory_length. Does the loop then read 65536 bytes out of the
4096-byte descriptor?
Since the RSS device option also sets cache_rss_config = false,
gve_get_rxfh() uses this device query path, and gve_get_rxfh_indir_size()
returns priv->rss_lut_size, so the ethtool core copies the whole indir array
back to userspace. Even without the wrap, any key plus LUT size totalling
more than 4096 passes as long as the device supplies a matching total_length.
Should rss_key_size and rss_lut_size be clamped where they are recorded, and
the computed length be validated against GVE_ADMINQ_BUFFER_SIZE rather than
only against the device supplied total_length?
[ ... ]
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 9cc343a162712..d3882de584e36 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -2398,57 +2398,71 @@ static const struct xdp_metadata_ops gve_xdp_metadata_ops = {
[ ... ]
> +static void gve_set_buf_sizes(struct gve_priv *priv)
> +{
> + struct gve_device_info *device_info = &priv->device_info;
> +
> + if (device_info->max_rx_buffer_size)
> + priv->max_rx_buffer_size = device_info->max_rx_buffer_size;
> +
> + if (gve_is_dqo(priv) &&
> + priv->max_rx_buffer_size > GVE_DEFAULT_RX_BUFFER_SIZE)
> + priv->rx_cfg.packet_buffer_size = priv->max_rx_buffer_size;
> +
> + if (device_info->header_buf_size)
> + priv->header_buf_size = device_info->header_buf_size;
> +}
[Severity: Low]
The commit message says:
Most of these changes are refactors only, except for one:
with the introduction of the central struct gve_device_info,
in AdminQ mode, gve_set_mtu() now also validates the device
provided mtu value from jumbo frames device option.
Is the buffer size handling a second behaviour change? Before this patch
gve_enable_supported_features() wrote both fields unconditionally:
priv->max_rx_buffer_size =
be16_to_cpu(dev_op_buffer_sizes->packet_buffer_size);
priv->header_buf_size =
be16_to_cpu(dev_op_buffer_sizes->header_buffer_size);
so a device advertising the buffer sizes option with packet_buffer_size == 0
ended up with priv->max_rx_buffer_size == 0, overwriting the
GVE_DEFAULT_RX_BUFFER_SIZE value that gve_probe() installed. With the new
guards in gve_set_buf_sizes() the probe-time default is kept instead.
The v5 changelog entry "honor device provided rx buffer size correctly"
describes this, but it sits below the --- line and does not end up in git
history. Could the commit message call out this change too?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916165533.2187434-1-hramamurthy%40google.com
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.