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
