RE: [EXTERNAL] Re: [PATCH net-next,2/2] virtio_net: replace RSS key size max check with BUILD_BUG_ON

[Srujana Challa <schalla@marvell.com>]

> > > ZjQcmQRYFpfptBannerEnd
> > > On Wed, Feb 25, 2026 at 05:52:29PM +0800, Xuan Zhuo wrote:
> > > > On Wed, 25 Feb 2026 04:47:22 -0500, "Michael S. Tsirkin"
> > > <mst@redhat.com> wrote:
> > > > > On Wed, Feb 25, 2026 at 05:36:06PM +0800, Xuan Zhuo wrote:
> > > > > > On Wed, 25 Feb 2026 04:33:57 -0500, "Michael S. Tsirkin"
> > > <mst@redhat.com> wrote:
> > > > > > > On Wed, Feb 25, 2026 at 05:30:33PM +0800, Xuan Zhuo wrote:
> > > > > > > > On Wed, 25 Feb 2026 04:24:14 -0500, "Michael S. Tsirkin"
> > > <mst@redhat.com> wrote:
> > > > > > > > > On Wed, Feb 25, 2026 at 05:11:42PM +0800, Xuan Zhuo wrote:
> > > > > > > > > > On Tue, 24 Feb 2026 12:28:50 +0530, Srujana Challa
> > > <schalla@marvell.com> wrote:
> > > > > > > > > > > Since NETDEV_RSS_KEY_LEN was increased to 256 in
> > > > > > > > > > > net-next, use BUILD_BUG_ON to enforce the limit at
> > > > > > > > > > > compile time and remove the redundant runtime max check.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Srujana Challa <schalla@marvell.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/net/virtio_net.c | 8 +-------
> > > > > > > > > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/net/virtio_net.c
> > > > > > > > > > > b/drivers/net/virtio_net.c index
> > > > > > > > > > > eeefe8abc122..768ad5523dfa 100644
> > > > > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > > > > @@ -6639,13 +6639,7 @@ static int
> > > > > > > > > > > virtnet_validate(struct
> > > virtio_device *vdev)
> > > > > > > > > > >  			__virtio_clear_bit(vdev,
> VIRTIO_NET_F_RSS);
> > > > > > > > > > >  			__virtio_clear_bit(vdev,
> > > VIRTIO_NET_F_HASH_REPORT);
> > > > > > > > > > >  		}
> > > > > > > > > > > -		if (key_sz > NETDEV_RSS_KEY_LEN) {
> > > > > > > > > > > -			dev_warn(&vdev->dev,
> > > > > > > > > > > -				 "rss_max_key_size=%u
> exceeds driver
> > > limit %u, disabling RSS\n",
> > > > > > > > > > > -				 key_sz,
> NETDEV_RSS_KEY_LEN);
> > > > > > > > > > > -			__virtio_clear_bit(vdev,
> VIRTIO_NET_F_RSS);
> > > > > > > > > > > -			__virtio_clear_bit(vdev,
> > > VIRTIO_NET_F_HASH_REPORT);
> > > > > > > > > > > -		}
> > > > > > > > > > > +		BUILD_BUG_ON(type_max(key_sz) >=
> > > NETDEV_RSS_KEY_LEN);
> > > > > > > > > >
> > > > > > > > > > Do we really need this check?
> > > > > > > > > >
> > > > > > > > > > If I understand correctly, the intention is to cap
> > > > > > > > > > key_sz at 256. However, since key_sz is of type u8,
> > > > > > > > > > its maximum value is inherently 255, making this check
> > > > > > > > > > redundant. This is not only limited by this kernel
> > > > > > > > > > code, the virtio-net spec defines
> > > this.
> > > > > > > > >
> > > > > > > > > That's why it's BUILD_BUG_ON. It checks it has the right type.
> > > > > > > > >
> > > > > > > > > We never *need* BUILD_BUG_ON by definition, what this
> > > > > > > > > does is document the assumption.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > Moreover, if NETDEV_RSS_KEY_LEN is ever reduced to a
> > > > > > > > > > value smaller than 256 in the future, this check would
> > > > > > > > > > no longer enforce
> > > the intended limit correctly.
> > > > > > > > >
> > > > > > > > > then it would fail build.
> > > > > > > >
> > > > > > > > So, does this mean we don't need to account for the case
> > > > > > > > where NETDEV_RSS_KEY_LEN is 128, but the key_sz reported
> > > > > > > > by the device is
> > > 64?
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > >
> > > > > > >
> > > > > > > yes.
> > > > > >
> > > > > > Why?
> > > > > >
> > > > > > If NETDEV_RSS_KEY_LEN is 128 but the device reports a key_sz
> > > > > > of 64, does this violate the spec?
> > > > >
> > > > > not the value of key_sz. If type of key_sz
> > > > >
> > > > >
> > > > > i actually do not understand the question. this is not what
> > > > > BUILD_BUG_ON checks.
> > > >
> > > > So this is the issue. Originally, the code checked whether the
> > > > value of key_sz was less than NETDEV_RSS_KEY_LEN. However,
> > > > switching to a type_max check means it no longer covers the scenario I
> described.
> > > > Therefore, I think this is unreasonable.
> > > >
> > > > Thanks
> > >
> > >
> > > patch 1 is unreasonable i think.
> >
> > Patch 1 is targeted for net, addressing an issue where
> > VIRTIO_NET_RSS_MAX_KEY_SIZE is fixed at 40, which is only the minimum
> > required by the spec. This led to virtio‑net probe failures when
> > devices reported an RSS key size greater than 40. However, the driver
> > also cannot handle keys larger than NETDEV_RSS_KEY_LEN (previously 52)
> > due to the BUG_ON(len > sizeof(netdev_rss_key)) in netdev_rss_key_fill. To
> resolve both issues, VIRTIO_NET_RSS_MAX_KEY_SIZE has been replaced with
> NETDEV_RSS_KEY_LEN.
> 
> but where would driver *get* keys larger than sizeof(netdev_rss_key), even if
> device supports more.
In virtio‑net, we rely on netdev_rss_key_fill() during virtnet_init_default_rss() to populate the RSS hash key,
which uses device supported length.
The code path is:
static void virtnet_init_default_rss(struct virtnet_info *vi)
{
        vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_supported);
        vi->rss_hash_types_saved = vi->rss_hash_types_supported;
        vi->rss_hdr->indirection_table_mask = vi->rss_indir_table_size
                                                ? cpu_to_le16(vi->rss_indir_table_size - 1) : 0;
        vi->rss_hdr->unclassified_queue = 0;

        virtnet_rss_update_by_qpairs(vi, vi->curr_queue_pairs);

        vi->rss_trailer.hash_key_length = vi->rss_key_size;

        netdev_rss_key_fill(vi->rss_hash_key_data, vi->rss_key_size);
}
> 
> 
> 
> > Patch 2 is added as per your suggestion to address following warning
> > in net-next after  NETDEV_RSS_KEY_LEN was increased to 256.
> > +../drivers/net/virtio_net.c:6642:14: warning: result of comparison of
> constant 256 with expression of type 'u8' (aka 'unsigned char') is always false
> [-Wtautological-constant-out-of-range-compare]
> > + 6642 |                 if (key_sz > NETDEV_RSS_KEY_LEN) {
> > +      |                     ~~~~~~ ^ ~~~~~~~~~~~~~~~~~~
> > +1 warning generated.
> > Sorry, I forgot the cover page.
> >
> > Thanks!
> > >
> > > which is why patchsets should have a cover letter btw so one can
> > > reply to just patch 1.
> > >
> > >
> > > > >
> > > > > > > the code makes assumptions but it documents them and not
> > > > > > > just documents them, build will fail if they are violated.
> > > > > >
> > > > > > About this, I am ok.
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Moreover, you should add a cover letter.
> > > > > > > > > >
> > > > > > > > > > Thanks.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >  	}
> > > > > > > > > > >
> > > > > > > > > > >  	return 0;
> > > > > > > > > > > --
> > > > > > > > > > > 2.25.1
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> >