Re: [EXTERNAL] Re: [PATCH net,v5] virtio_net: clamp rss_max_key_size to NETDEV_RSS_KEY_LEN

["Michael S. Tsirkin" <mst@redhat.com>]

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: stable@vger.kernel.org
> >>> Signed-off-by: Srujana Challa <schalla@marvell.com>
> >>> ---
> >>> 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