On Tue, Mar 31, 2026 at 04:39:02PM +0200, Paolo Abeni wrote:
> On 3/31/26 3:29 PM, Srujana Challa wrote:
> >> On 3/26/26 3:23 PM, Srujana Challa wrote:
> >>> rss_max_key_size in the virtio spec is the maximum key size supported
> >>> by the device, not a mandatory size the driver must use. Also the
> >>> value 40 is a spec minimum, not a spec maximum.
> >>>
> >>> The current code rejects RSS and can fail probe when the device
> >>> reports a larger rss_max_key_size than the driver buffer limit.
> >>> Instead, clamp the effective key length to min(device
> >>> rss_max_key_size, NETDEV_RSS_KEY_LEN) and keep RSS enabled.
> >>>
> >>> This keeps probe working on devices that advertise larger maximum key
> >>> sizes while respecting the netdev RSS key buffer size limit.
> >>>
> >>> Fixes: 3f7d9c1964fc ("virtio_net: Add hash_key_length check")
> >>> Cc: [email protected]
> >>> Signed-off-by: Srujana Challa <[email protected]>
> >>> ---
> >>> v3:
> >>> - Moved RSS key validation checks to virtnet_validate.
> >>> - Add fixes: tag and CC -stable
> >>> v4:
> >>> - Use NETDEV_RSS_KEY_LEN instead of type_max for the maximum rss key
> >> size.
> >>> v5:
> >>> - Interpret rss_max_key_size as a maximum and clamp it to
> >> NETDEV_RSS_KEY_LEN.
> >>> - Do not disable RSS/HASH_REPORT when device rss_max_key_size exceeds
> >> NETDEV_RSS_KEY_LEN.
> >>> - Drop the separate patch that replaced the runtime check with
> >> BUILD_BUG_ON.
> >>>
> >>> drivers/net/virtio_net.c | 20 +++++++++-----------
> >>> 1 file changed, 9 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> >>> 022f60728721..b241c8dbb4e1 100644
> >>> --- a/drivers/net/virtio_net.c
> >>> +++ b/drivers/net/virtio_net.c
> >>> @@ -373,8 +373,6 @@ struct receive_queue {
> >>> struct xdp_buff **xsk_buffs;
> >>> };
> >>>
> >>> -#define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
> >>> -
> >>> /* Control VQ buffers: protected by the rtnl lock */ struct
> >>> control_buf {
> >>> struct virtio_net_ctrl_hdr hdr;
> >>> @@ -478,7 +476,7 @@ struct virtnet_info {
> >>>
> >>> /* Must be last as it ends in a flexible-array member. */
> >>> TRAILING_OVERLAP(struct virtio_net_rss_config_trailer, rss_trailer,
> >> hash_key_data,
> >>> - u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> >>> + u8 rss_hash_key_data[NETDEV_RSS_KEY_LEN];
> >>> );
> >>> };
> >>> static_assert(offsetof(struct virtnet_info,
> >>> rss_trailer.hash_key_data) == @@ -6717,6 +6715,7 @@ static int
> >> virtnet_probe(struct virtio_device *vdev)
> >>> struct virtnet_info *vi;
> >>> u16 max_queue_pairs;
> >>> int mtu = 0;
> >>> + u16 key_sz;
> >>>
> >>> /* Find if host supports multiqueue/rss virtio_net device */
> >>> max_queue_pairs = 1;
> >>> @@ -6851,14 +6850,13 @@ static int virtnet_probe(struct virtio_device
> >> *vdev)
> >>> }
> >>>
> >>> if (vi->has_rss || vi->has_rss_hash_report) {
> >>> - vi->rss_key_size =
> >>> - virtio_cread8(vdev, offsetof(struct virtio_net_config,
> >> rss_max_key_size));
> >>> - if (vi->rss_key_size > VIRTIO_NET_RSS_MAX_KEY_SIZE) {
> >>> - dev_err(&vdev->dev, "rss_max_key_size=%u exceeds
> >> the limit %u.\n",
> >>> - vi->rss_key_size,
> >> VIRTIO_NET_RSS_MAX_KEY_SIZE);
> >>> - err = -EINVAL;
> >>> - goto free;
> >>> - }
> >>> + key_sz = virtio_cread8(vdev, offsetof(struct virtio_net_config,
> >>> +rss_max_key_size));
> >>> +
> >>> + vi->rss_key_size = min_t(u16, key_sz, NETDEV_RSS_KEY_LEN);
> >>> + if (key_sz > vi->rss_key_size)
> >>> + dev_warn(&vdev->dev,
> >>> + "rss_max_key_size=%u exceeds driver limit
> >> %u, clamping\n",
> >>> + key_sz, vi->rss_key_size);
> >>
> >> NETDEV_RSS_KEY_LEN is 256 and virtio_cread8() returns a u8. The check is
> >> not needed, and the warning will never be printed. I think that the
> >> BUILD_BUG_ON() you used in v4 would be better than the above chunk.
> >>
> > Thank you for the feedback. In net-next, NETDEV_RSS_KEY_LEN is 256. This
> > fix is
> > also intended for stable kernels, where NETDEV_RSS_KEY_LEN is 52, and
> > I added the message to make clamping visible in that case.
> > I will remove the check and send the next version.
>
> I'm sorry, I haven't looked at the historical context when I wrote my
> previous reply.
>
> IMHO the additional check does not make sense in the current net tree.
> On the flip side stable trees will need it. I suggest:
>
> - dropping the check for the 'net' patch
> - also dropping CC: stable tag
> - explicitly sending to stable the fix variant including the size check.
>
> @Michael: WDYT?
>
> /P
I was the one who suggested it, the extra check is harmless, I'm
inclined to always have it. Less work than maintaining two patches.
--
MST
