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

[Paolo Abeni <pabeni@redhat.com>]

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