On 9/4/25 23:31, Michael S. Tsirkin wrote:
On Thu, Sep 04, 2025 at 08:53:31PM +0200, Gustavo A. R. Silva wrote:
On 9/4/25 11:13, Simon Horman wrote:
On Wed, Sep 03, 2025 at 09:36:13PM +0200, Gustavo A. R. Silva wrote:
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are
getting ready to enable it, globally.
Use the new TRAILING_OVERLAP() helper to fix the following warning:
drivers/net/virtio_net.c:429:46: warning: structure containing a flexible array
member is not at the end of another structure [-Wflex-array-member-not-at-end]
This helper creates a union between a flexible-array member (FAM)
and a set of members that would otherwise follow it (in this case
`u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];`). This
overlays the trailing members (rss_hash_key_data) onto the FAM
(hash_key_data) while keeping the FAM and the start of MEMBERS aligned.
The static_assert() ensures this alignment remains, and it's
intentionally placed inmediately after `struct virtnet_info` (no
blank line in between).
Notice that due to tail padding in flexible `struct
virtio_net_rss_config_trailer`, `rss_trailer.hash_key_data`
(at offset 83 in struct virtnet_info) and `rss_hash_key_data` (at
offset 84 in struct virtnet_info) are misaligned by one byte. See
below:
struct virtio_net_rss_config_trailer {
__le16 max_tx_vq; /* 0 2 */
__u8 hash_key_length; /* 2 1 */
__u8 hash_key_data[]; /* 3 0 */
/* size: 4, cachelines: 1, members: 3 */
/* padding: 1 */
/* last cacheline: 4 bytes */
};
struct virtnet_info {
...
struct virtio_net_rss_config_trailer rss_trailer; /* 80 4 */
/* XXX last struct has 1 byte of padding */
u8 rss_hash_key_data[40]; /* 84 40 */
...
/* size: 832, cachelines: 13, members: 48 */
/* sum members: 801, holes: 8, sum holes: 31 */
/* paddings: 2, sum paddings: 5 */
};
After changes, those members are correctly aligned at offset 795:
struct virtnet_info {
...
union {
struct virtio_net_rss_config_trailer rss_trailer; /* 792
4 */
struct {
unsigned char __offset_to_hash_key_data[3]; /* 792
3 */
u8 rss_hash_key_data[40]; /* 795 40 */
}; /* 792 43 */
}; /* 792 44 */
...
/* size: 840, cachelines: 14, members: 47 */
/* sum members: 801, holes: 8, sum holes: 35 */
/* padding: 4 */
/* paddings: 1, sum paddings: 4 */
/* last cacheline: 8 bytes */
};
As a last note `struct virtio_net_rss_config_hdr *rss_hdr;` is also
moved to the end, since it seems those three members should stick
around together. :)
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
This should probably include the following tag:
Fixes: ed3100e90d0d ("virtio_net: Use new RSS config structs")
but I'd like to hear some feedback, first.
I tend to agree given that:
On the one hand:
1) in virtnet_init_default_rss(), netdev_rss_key_fill() is used
to write random data to .rss_hash_key_data
2) In virtnet_set_rxfh() key data written to .rss_hash_key_data
While
3) In virtnet_commit_rss_command() virtio_net_rss_config_trailer,
including the contents of .hash_key_data based on the length of
that data provided in .hash_key_length is copied.
It seems to me that step 3 will include 1 byte of uninitialised data
at the start of .hash_key_data. And, correspondingly, truncate
.rss_hash_key_data by one byte.
It's unclear to me what the effect of this - perhaps they key works
regardless. But it doesn't seem intended. And while the result may be
neutral, I do suspect this reduces the quality of the key. And I more
strongly suspect it doesn't have any positive outcome.
So I would lean towards playing it safe and considering this as a bug.
Of course, other's may have better insight as to the actual effect of this.
Yeah, in the meantime I'll prepare v2 with both the 'Fixes' and 'stable'
tags.
Thanks for the feedback!
-Gustavo
I agree. It looks like that commit completely broke RSS
configuration. Akihiko do you mind sharing how that was
tested? Maybe help testing the fix? Thanks!
I've been waiting for Akihiko's comments on this, but I
guess I'll now go ahead and submit v2 with the Fixes and
stable tags included.
Thanks for the feedback, Michael.
-Gustavo
