[PATCH] virtio_net: roll back RSS state on control failure

[PATCH] virtio_net: roll back RSS state on control failure

[Xiong Weimin]

The ethtool RSS and RXHASH paths update the driver's cached RSS state
before committing the change to the device.  If the control virtqueue
command fails, the cached hash types, key or indirection table can then
report a configuration that the device did not accept.

Preserve the previous local state around RSS/hash control commands and
restore it when the device update fails, while propagating the error to
the caller.

Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>
---
 drivers/net/virtio_net.c | 54 ++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3e2a5876c..ccd96315a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -4295,6 +4295,7 @@ static int virtnet_set_hashflow(struct net_device *dev,
 				struct netlink_ext_ack *extack)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
+	u32 old_hashtypes = vi->rss_hash_types_saved;
 	u32 new_hashtypes = vi->rss_hash_types_saved;
 	bool is_disable = info->data & RXH_DISCARD;
 	bool is_l4 = info->data == (RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3);
@@ -4350,9 +4351,13 @@ static int virtnet_set_hashflow(struct net_device *dev,
 	if (new_hashtypes != vi->rss_hash_types_saved) {
 		vi->rss_hash_types_saved = new_hashtypes;
 		vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
-		if (vi->dev->features & NETIF_F_RXHASH)
-			if (!virtnet_commit_rss_command(vi))
+		if (vi->dev->features & NETIF_F_RXHASH) {
+			if (!virtnet_commit_rss_command(vi)) {
+				vi->rss_hash_types_saved = old_hashtypes;
+				vi->rss_hdr->hash_types = cpu_to_le32(old_hashtypes);
 				return -EINVAL;
+			}
+		}
 	}
 
 	return 0;
@@ -5546,6 +5551,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
 			    struct netlink_ext_ack *extack)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
+	struct virtio_net_rss_config_hdr *old_rss_hdr = NULL;
+	u8 old_rss_key[NETDEV_RSS_KEY_LEN];
 	bool update = false;
 	int i;
 
@@ -5553,14 +5560,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
 	    rxfh->hfunc != ETH_RSS_HASH_TOP)
 		return -EOPNOTSUPP;
 
-	if (rxfh->indir) {
-		if (!vi->has_rss)
-			return -EOPNOTSUPP;
-
-		for (i = 0; i < vi->rss_indir_table_size; ++i)
-			vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
-		update = true;
-	}
+	if (rxfh->indir && !vi->has_rss)
+		return -EOPNOTSUPP;
 
 	if (rxfh->key) {
 		/* If either _F_HASH_REPORT or _F_RSS are negotiated, the
@@ -5569,13 +5570,36 @@ static int virtnet_set_rxfh(struct net_device *dev,
 		 */
 		if (!vi->has_rss && !vi->has_rss_hash_report)
 			return -EOPNOTSUPP;
+	}
+
+	if (rxfh->indir) {
+		old_rss_hdr = kmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
+				      GFP_KERNEL);
+		if (!old_rss_hdr)
+			return -ENOMEM;
+
+		for (i = 0; i < vi->rss_indir_table_size; ++i)
+			vi->rss_hdr->indirection_table[i] =
+				cpu_to_le16(rxfh->indir[i]);
+		update = true;
+	}
 
+	if (rxfh->key) {
+		memcpy(old_rss_key, vi->rss_hash_key_data, vi->rss_key_size);
 		memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
 		update = true;
 	}
 
-	if (update)
-		virtnet_commit_rss_command(vi);
+	if (update && !virtnet_commit_rss_command(vi)) {
+		if (old_rss_hdr)
+			memcpy(vi->rss_hdr, old_rss_hdr, virtnet_rss_hdr_size(vi));
+		if (rxfh->key)
+			memcpy(vi->rss_hash_key_data, old_rss_key, vi->rss_key_size);
+		kfree(old_rss_hdr);
+		return -EINVAL;
+	}
+
+	kfree(old_rss_hdr);
 
 	return 0;
 }
@@ -6171,13 +6195,17 @@ static int virtnet_set_features(struct net_device *dev,
 	}
 
 	if ((dev->features ^ features) & NETIF_F_RXHASH) {
+		__le32 hash_types = vi->rss_hdr->hash_types;
+
 		if (features & NETIF_F_RXHASH)
 			vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
 		else
 			vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
 
-		if (!virtnet_commit_rss_command(vi))
+		if (!virtnet_commit_rss_command(vi)) {
+			vi->rss_hdr->hash_types = hash_types;
 			return -EINVAL;
+		}
 	}
 
 	return 0;
-- 
2.43.0

Re: [PATCH] virtio_net: roll back RSS state on control failure

[Jakub Kicinski]

On Tue,  4 Aug 2026 15:36:54 +0800 Xiong Weimin wrote:
> The ethtool RSS and RXHASH paths update the driver's cached RSS state
> before committing the change to the device.  If the control virtqueue
> command fails, the cached hash types, key or indirection table can then
> report a configuration that the device did not accept.
> 
> Preserve the previous local state around RSS/hash control commands and
> restore it when the device update fails, while propagating the error to
> the caller.

I guess.. saving the data and then copying it back doesn't seem super
clean but I guess since we DMA directly from the info struct..

Michael, Jason, looks okay? 

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 3e2a5876c..ccd96315a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -4295,6 +4295,7 @@ static int virtnet_set_hashflow(struct net_device *dev,
>  				struct netlink_ext_ack *extack)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
> +	u32 old_hashtypes = vi->rss_hash_types_saved;
>  	u32 new_hashtypes = vi->rss_hash_types_saved;
>  	bool is_disable = info->data & RXH_DISCARD;
>  	bool is_l4 = info->data == (RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3);
> @@ -4350,9 +4351,13 @@ static int virtnet_set_hashflow(struct net_device *dev,
>  	if (new_hashtypes != vi->rss_hash_types_saved) {
>  		vi->rss_hash_types_saved = new_hashtypes;
>  		vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> -		if (vi->dev->features & NETIF_F_RXHASH)
> -			if (!virtnet_commit_rss_command(vi))
> +		if (vi->dev->features & NETIF_F_RXHASH) {
> +			if (!virtnet_commit_rss_command(vi)) {
> +				vi->rss_hash_types_saved = old_hashtypes;
> +				vi->rss_hdr->hash_types = cpu_to_le32(old_hashtypes);
>  				return -EINVAL;
> +			}
> +		}
>  	}
>  
>  	return 0;
> @@ -5546,6 +5551,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
>  			    struct netlink_ext_ack *extack)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
> +	struct virtio_net_rss_config_hdr *old_rss_hdr = NULL;
> +	u8 old_rss_key[NETDEV_RSS_KEY_LEN];
>  	bool update = false;
>  	int i;
>  
> @@ -5553,14 +5560,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
>  	    rxfh->hfunc != ETH_RSS_HASH_TOP)
>  		return -EOPNOTSUPP;
>  
> -	if (rxfh->indir) {
> -		if (!vi->has_rss)
> -			return -EOPNOTSUPP;
> -
> -		for (i = 0; i < vi->rss_indir_table_size; ++i)
> -			vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
> -		update = true;
> -	}
> +	if (rxfh->indir && !vi->has_rss)
> +		return -EOPNOTSUPP;
>  
>  	if (rxfh->key) {
>  		/* If either _F_HASH_REPORT or _F_RSS are negotiated, the
> @@ -5569,13 +5570,36 @@ static int virtnet_set_rxfh(struct net_device *dev,
>  		 */
>  		if (!vi->has_rss && !vi->has_rss_hash_report)
>  			return -EOPNOTSUPP;
> +	}
> +
> +	if (rxfh->indir) {
> +		old_rss_hdr = kmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
> +				      GFP_KERNEL);
> +		if (!old_rss_hdr)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < vi->rss_indir_table_size; ++i)
> +			vi->rss_hdr->indirection_table[i] =
> +				cpu_to_le16(rxfh->indir[i]);
> +		update = true;
> +	}
>  
> +	if (rxfh->key) {
> +		memcpy(old_rss_key, vi->rss_hash_key_data, vi->rss_key_size);
>  		memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
>  		update = true;
>  	}
>  
> -	if (update)
> -		virtnet_commit_rss_command(vi);
> +	if (update && !virtnet_commit_rss_command(vi)) {
> +		if (old_rss_hdr)
> +			memcpy(vi->rss_hdr, old_rss_hdr, virtnet_rss_hdr_size(vi));
> +		if (rxfh->key)
> +			memcpy(vi->rss_hash_key_data, old_rss_key, vi->rss_key_size);
> +		kfree(old_rss_hdr);
> +		return -EINVAL;
> +	}
> +
> +	kfree(old_rss_hdr);
>  
>  	return 0;
>  }
> @@ -6171,13 +6195,17 @@ static int virtnet_set_features(struct net_device *dev,
>  	}
>  
>  	if ((dev->features ^ features) & NETIF_F_RXHASH) {
> +		__le32 hash_types = vi->rss_hdr->hash_types;
> +
>  		if (features & NETIF_F_RXHASH)
>  			vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
>  		else
>  			vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
>  
> -		if (!virtnet_commit_rss_command(vi))
> +		if (!virtnet_commit_rss_command(vi)) {
> +			vi->rss_hdr->hash_types = hash_types;
>  			return -EINVAL;
> +		}
>  	}
>  
>  	return 0;

Re: [PATCH] virtio_net: roll back RSS state on control failure

[Michael S. Tsirkin]

On Fri, Aug 07, 2026 at 06:19:16PM -0700, Jakub Kicinski wrote:
> On Tue,  4 Aug 2026 15:36:54 +0800 Xiong Weimin wrote:
> > The ethtool RSS and RXHASH paths update the driver's cached RSS state
> > before committing the change to the device.  If the control virtqueue
> > command fails, the cached hash types, key or indirection table can then
> > report a configuration that the device did not accept.
> > 
> > Preserve the previous local state around RSS/hash control commands and
> > restore it when the device update fails, while propagating the error to
> > the caller.
> 
> I guess.. saving the data and then copying it back doesn't seem super
> clean but I guess since we DMA directly from the info struct..
> 
> Michael, Jason, looks okay? 
> 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 3e2a5876c..ccd96315a 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -4295,6 +4295,7 @@ static int virtnet_set_hashflow(struct net_device *dev,
> >  				struct netlink_ext_ack *extack)
> >  {
> >  	struct virtnet_info *vi = netdev_priv(dev);
> > +	u32 old_hashtypes = vi->rss_hash_types_saved;
> >  	u32 new_hashtypes = vi->rss_hash_types_saved;
> >  	bool is_disable = info->data & RXH_DISCARD;
> >  	bool is_l4 = info->data == (RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3);
> > @@ -4350,9 +4351,13 @@ static int virtnet_set_hashflow(struct net_device *dev,
> >  	if (new_hashtypes != vi->rss_hash_types_saved) {
> >  		vi->rss_hash_types_saved = new_hashtypes;
> >  		vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> > -		if (vi->dev->features & NETIF_F_RXHASH)
> > -			if (!virtnet_commit_rss_command(vi))
> > +		if (vi->dev->features & NETIF_F_RXHASH) {
> > +			if (!virtnet_commit_rss_command(vi)) {
> > +				vi->rss_hash_types_saved = old_hashtypes;
> > +				vi->rss_hdr->hash_types = cpu_to_le32(old_hashtypes);
> >  				return -EINVAL;
> > +			}
> > +		}
> >  	}
> >  
> >  	return 0;
> > @@ -5546,6 +5551,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
> >  			    struct netlink_ext_ack *extack)
> >  {
> >  	struct virtnet_info *vi = netdev_priv(dev);
> > +	struct virtio_net_rss_config_hdr *old_rss_hdr = NULL;
> > +	u8 old_rss_key[NETDEV_RSS_KEY_LEN];
> >  	bool update = false;
> >  	int i;
> >  
> > @@ -5553,14 +5560,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
> >  	    rxfh->hfunc != ETH_RSS_HASH_TOP)
> >  		return -EOPNOTSUPP;
> >  
> > -	if (rxfh->indir) {
> > -		if (!vi->has_rss)
> > -			return -EOPNOTSUPP;
> > -
> > -		for (i = 0; i < vi->rss_indir_table_size; ++i)
> > -			vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
> > -		update = true;
> > -	}
> > +	if (rxfh->indir && !vi->has_rss)
> > +		return -EOPNOTSUPP;
> >  
> >  	if (rxfh->key) {
> >  		/* If either _F_HASH_REPORT or _F_RSS are negotiated, the
> > @@ -5569,13 +5570,36 @@ static int virtnet_set_rxfh(struct net_device *dev,
> >  		 */
> >  		if (!vi->has_rss && !vi->has_rss_hash_report)
> >  			return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (rxfh->indir) {
> > +		old_rss_hdr = kmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
> > +				      GFP_KERNEL);

kvmemdup maybe, just in case - I think it can get as high as 128k after all.

> > +		if (!old_rss_hdr)
> > +			return -ENOMEM;
> > +
> > +		for (i = 0; i < vi->rss_indir_table_size; ++i)
> > +			vi->rss_hdr->indirection_table[i] =
> > +				cpu_to_le16(rxfh->indir[i]);
> > +		update = true;
> > +	}
> >  
> > +	if (rxfh->key) {
> > +		memcpy(old_rss_key, vi->rss_hash_key_data, vi->rss_key_size);
> >  		memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
> >  		update = true;
> >  	}
> >  
> > -	if (update)
> > -		virtnet_commit_rss_command(vi);
> > +	if (update && !virtnet_commit_rss_command(vi)) {
> > +		if (old_rss_hdr)
> > +			memcpy(vi->rss_hdr, old_rss_hdr, virtnet_rss_hdr_size(vi));
> > +		if (rxfh->key)
> > +			memcpy(vi->rss_hash_key_data, old_rss_key, vi->rss_key_size);
> > +		kfree(old_rss_hdr);
> > +		return -EINVAL;
> > +	}
> > +
> > +	kfree(old_rss_hdr);
> >  
> >  	return 0;
> >  }
> > @@ -6171,13 +6195,17 @@ static int virtnet_set_features(struct net_device *dev,
> >  	}
> >  
> >  	if ((dev->features ^ features) & NETIF_F_RXHASH) {
> > +		__le32 hash_types = vi->rss_hdr->hash_types;
> > +
> >  		if (features & NETIF_F_RXHASH)
> >  			vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> >  		else
> >  			vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
> >  
> > -		if (!virtnet_commit_rss_command(vi))
> > +		if (!virtnet_commit_rss_command(vi)) {
> > +			vi->rss_hdr->hash_types = hash_types;
> >  			return -EINVAL;
> > +		}
> >  	}
> >  
> >  	return 0;

[PATCH] virtio_net: roll back RSS state on control failure

[Xiong Weimin]

The ethtool RSS and RXHASH paths update the driver's cached RSS state
before committing the change to the device.  If the control virtqueue
command fails, the cached hash types, key or indirection table can then
report a configuration that the device did not accept.

Preserve the previous local state around RSS/hash control commands and
restore it when the device update fails, while propagating the error to
the caller.

v1 -> v2:
  - Use kvmemdup() instead of kmemdup() in virtnet_set_rxfh() for the
    saved RSS header, since the indirection table can push the header
    size up to ~128K.

Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>
---
 drivers/net/virtio_net.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ccd96315a..995ca4640 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -5573,8 +5573,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
 	}
 
 	if (rxfh->indir) {
-		old_rss_hdr = kmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
-				      GFP_KERNEL);
+		old_rss_hdr = kvmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
+				       GFP_KERNEL);
 		if (!old_rss_hdr)
 			return -ENOMEM;
 
-- 
2.43.0

[PATCH] virtio_net: roll back RSS state on control failure

[Xiong Weimin]

The ethtool RSS and RXHASH paths update the driver's cached RSS state
before committing the change to the device.  If the control virtqueue
command fails, the cached hash types, key or indirection table can then
report a configuration that the device did not accept.

Preserve the previous local state around RSS/hash control commands and
restore it when the device update fails, while propagating the error to
the caller.

v1 -> v2:
  - Use kvmemdup() instead of kmemdup() in virtnet_set_rxfh() for the
    saved RSS header, since the indirection table can push the header
    size up to ~128K.

Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>
---
 drivers/net/virtio_net.c | 56 +++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3e2a5876c..995ca4640 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -4295,6 +4295,7 @@ static int virtnet_set_hashflow(struct net_device *dev,
 				struct netlink_ext_ack *extack)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
+	u32 old_hashtypes = vi->rss_hash_types_saved;
 	u32 new_hashtypes = vi->rss_hash_types_saved;
 	bool is_disable = info->data & RXH_DISCARD;
 	bool is_l4 = info->data == (RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3);
@@ -4350,9 +4351,13 @@ static int virtnet_set_hashflow(struct net_device *dev,
 	if (new_hashtypes != vi->rss_hash_types_saved) {
 		vi->rss_hash_types_saved = new_hashtypes;
 		vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
-		if (vi->dev->features & NETIF_F_RXHASH)
-			if (!virtnet_commit_rss_command(vi))
+		if (vi->dev->features & NETIF_F_RXHASH) {
+			if (!virtnet_commit_rss_command(vi)) {
+				vi->rss_hash_types_saved = old_hashtypes;
+				vi->rss_hdr->hash_types = cpu_to_le32(old_hashtypes);
 				return -EINVAL;
+			}
+		}
 	}
 
 	return 0;
@@ -5546,6 +5551,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
 			    struct netlink_ext_ack *extack)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
+	struct virtio_net_rss_config_hdr *old_rss_hdr = NULL;
+	u8 old_rss_key[NETDEV_RSS_KEY_LEN];
 	bool update = false;
 	int i;
 
@@ -5553,14 +5560,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
 	    rxfh->hfunc != ETH_RSS_HASH_TOP)
 		return -EOPNOTSUPP;
 
-	if (rxfh->indir) {
-		if (!vi->has_rss)
-			return -EOPNOTSUPP;
-
-		for (i = 0; i < vi->rss_indir_table_size; ++i)
-			vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
-		update = true;
-	}
+	if (rxfh->indir && !vi->has_rss)
+		return -EOPNOTSUPP;
 
 	if (rxfh->key) {
 		/* If either _F_HASH_REPORT or _F_RSS are negotiated, the
@@ -5569,13 +5570,36 @@ static int virtnet_set_rxfh(struct net_device *dev,
 		 */
 		if (!vi->has_rss && !vi->has_rss_hash_report)
 			return -EOPNOTSUPP;
+	}
+
+	if (rxfh->indir) {
+		old_rss_hdr = kvmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
+				       GFP_KERNEL);
+		if (!old_rss_hdr)
+			return -ENOMEM;
+
+		for (i = 0; i < vi->rss_indir_table_size; ++i)
+			vi->rss_hdr->indirection_table[i] =
+				cpu_to_le16(rxfh->indir[i]);
+		update = true;
+	}
 
+	if (rxfh->key) {
+		memcpy(old_rss_key, vi->rss_hash_key_data, vi->rss_key_size);
 		memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
 		update = true;
 	}
 
-	if (update)
-		virtnet_commit_rss_command(vi);
+	if (update && !virtnet_commit_rss_command(vi)) {
+		if (old_rss_hdr)
+			memcpy(vi->rss_hdr, old_rss_hdr, virtnet_rss_hdr_size(vi));
+		if (rxfh->key)
+			memcpy(vi->rss_hash_key_data, old_rss_key, vi->rss_key_size);
+		kfree(old_rss_hdr);
+		return -EINVAL;
+	}
+
+	kfree(old_rss_hdr);
 
 	return 0;
 }
@@ -6171,13 +6195,17 @@ static int virtnet_set_features(struct net_device *dev,
 	}
 
 	if ((dev->features ^ features) & NETIF_F_RXHASH) {
+		__le32 hash_types = vi->rss_hdr->hash_types;
+
 		if (features & NETIF_F_RXHASH)
 			vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
 		else
 			vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
 
-		if (!virtnet_commit_rss_command(vi))
+		if (!virtnet_commit_rss_command(vi)) {
+			vi->rss_hdr->hash_types = hash_types;
 			return -EINVAL;
+		}
 	}
 
 	return 0;
-- 
2.43.0

Re: [PATCH] virtio_net: roll back RSS state on control failure

[Xiong Weimin]

Hi,

Please ignore my previous email
<20260811021900.3464410-1-xiongweimin@kylinos.cn>. The correct
full patch has been resent in a separate email.

Sorry for the noise.

Re: [PATCH] virtio_net: roll back RSS state on control failure

[Markus Elfring]

…
> Preserve the previous local state around RSS/hash control commands and
> restore it when the device update fails, while propagating the error to
> the caller.

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n145
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v7.2-rc7#n34


> v1 -> v2:
…
> ---
>  drivers/net/virtio_net.c | 56 +++++++++++++++++++++++++++++----------
…

Please move patch version descriptions behind the marker line.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc7#n795

Regards,
Markus

Re: [PATCH] virtio_net: roll back RSS state on control failure

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
    virtio_net: roll back RSS state on control failure

This patch saves the driver's cached RSS state (hash types, hash key and
indirection table) before issuing an RSS/hash control virtqueue command
and restores it if the command fails.  It also makes virtnet_set_rxfh()
propagate the failure to the caller instead of returning success.

A couple of questions on the commit message and the new rollback paths
below.

>     Preserve the previous local state around RSS/hash control commands and
>     restore it when the device update fails, while propagating the error to
>     the caller.

[Severity: Low]
Should this carry a Fixes: tag?  The change alters user-visible behavior:
virtnet_set_rxfh() previously returned 0 even when the control command was
rejected, and all three paths left the cached RSS state describing a
configuration the device never accepted.

The rxfh and RXHASH paths come from c7114b1249fa ("drivers/net/virtio_net:
Added basic RSS support.") and the hash-flow callback from c11708209df2
("drivers/net/virtio_net: Added RSS hash report control."), so a Fixes:
line would help stable tooling and backporters scope the fix.

>     v1 -> v2:
>       - Use kvmemdup() instead of kmemdup() in virtnet_set_rxfh() for the
>         saved RSS header, since the indirection table can push the header
>         size up to ~128K.

[Severity: Low]
This isn't a bug, but the inter-version changelog is above the '---'
separator, so it becomes part of the permanent git history.  Could it move
below the '---' marker instead?

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 3e2a5876c6c8c..995ca46404088 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -4350,9 +4351,13 @@ static int virtnet_set_hashflow(struct net_device *dev,
>  	if (new_hashtypes != vi->rss_hash_types_saved) {
>  		vi->rss_hash_types_saved = new_hashtypes;
>  		vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> -		if (vi->dev->features & NETIF_F_RXHASH)
> -			if (!virtnet_commit_rss_command(vi))
> +		if (vi->dev->features & NETIF_F_RXHASH) {
> +			if (!virtnet_commit_rss_command(vi)) {
> +				vi->rss_hash_types_saved = old_hashtypes;
> +				vi->rss_hdr->hash_types = cpu_to_le32(old_hashtypes);
>  				return -EINVAL;
> +			}
> +		}
> +		}
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but while the block is being
touched: the write to the device-visible vi->rss_hdr->hash_types in
virtnet_set_hashflow() happens outside the NETIF_F_RXHASH check, while the
control command is only sent inside it.  Should the header write also be
conditional?

With rx-hashing off, virtnet_set_features() has already programmed
VIRTIO_NET_HASH_REPORT_NONE into the same header:

	if (features & NETIF_F_RXHASH)
		vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
	else
		vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);

A following rx-flow-hash request then overwrites hash_types with a non-zero
value and skips the command, so the value stays staged in the shared
header.  The next unrelated commit sends the whole header:

drivers/net/virtio_net.c:virtnet_commit_rss_command() {
	sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
	...
}

so an ethtool -X key or indirection update via virtnet_set_rxfh() would
push those hash types to the device while dev->features still reports
RXHASH off.  ethtool_set_rxfh_fields() in net/ethtool/ioctl.c only checks
ops->set_rxfh_fields, so the sequence

	ethtool -K eth0 rx-hashing off
	ethtool -N eth0 rx-flow-hash tcp4 sd
	ethtool -X eth0 hkey ...

is reachable from userspace.  The new rollback only restores hash_types
inside the RXHASH branch, so this case is unchanged.

> @@ -5546,6 +5551,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
>  			    struct netlink_ext_ack *extack)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
> +	struct virtio_net_rss_config_hdr *old_rss_hdr = NULL;
> +	u8 old_rss_key[NETDEV_RSS_KEY_LEN];
>  	bool update = false;
>  	int i;

[ ... ]

> +	if (rxfh->indir) {
> +		old_rss_hdr = kvmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
> +				       GFP_KERNEL);
> +		if (!old_rss_hdr)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < vi->rss_indir_table_size; ++i)
> +			vi->rss_hdr->indirection_table[i] =
> +				cpu_to_le16(rxfh->indir[i]);
> +		update = true;
> +	}
>  
> +	if (rxfh->key) {
> +		memcpy(old_rss_key, vi->rss_hash_key_data, vi->rss_key_size);
>  		memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
>  		update = true;
>  	}
>  
> -	if (update)
> -		virtnet_commit_rss_command(vi);
> +	if (update && !virtnet_commit_rss_command(vi)) {
> +		if (old_rss_hdr)
> +			memcpy(vi->rss_hdr, old_rss_hdr, virtnet_rss_hdr_size(vi));
> +		if (rxfh->key)
> +			memcpy(vi->rss_hash_key_data, old_rss_key, vi->rss_key_size);
> +		kfree(old_rss_hdr);
> +		return -EINVAL;
> +	}
> +
> +	kfree(old_rss_hdr);

[Severity: High]
Should both of these frees be kvfree()?  old_rss_hdr comes from
kvmemdup(), whose kernel-doc in mm/util.c is explicit about the pairing:

mm/util.c:
 * Return: newly allocated copy of @src or %NULL in case of error,
 * result may be not physically contiguous. Use kvfree() to free.
 */
void *kvmemdup(const void *src, size_t len, gfp_t gfp)
{
	void *p;

	p = kvmalloc(len, gfp);

The vmalloc fallback is real for the sizes this patch's changelog
mentions:

mm/slub.c:__kvmalloc_node_noprof() {
	ret = __do_kmalloc_node(PASS_BUCKET_PARAM(b),
				kmalloc_gfp_adjust(flags, size),
				node, PASS_TOKEN_PARAM(token), &ac);
	if (ret || size <= PAGE_SIZE)
		return ret;
	...
}

The kmalloc attempt for size > PAGE_SIZE runs without
__GFP_DIRECT_RECLAIM, so plain fragmentation is enough to fall through to
__vmalloc_node().  The allocation size here is virtnet_rss_hdr_size(vi),
which is derived from vi->rss_indir_table_size, read straight from device
config space in virtnet_probe():

		vi->rss_indir_table_size =
			virtio_cread16(vdev, offsetof(struct virtio_net_config,
				rss_max_indirection_table_length));

Once a vmalloc pointer is returned, kfree() derives a struct page from a
non-direct-map address:

mm/slub.c:kfree() {
	page = virt_to_page(object);
	slab = page_slab(page);
	if (!slab) {
		/* kmalloc_nolock() doesn't support large kmalloc */
		free_large_kmalloc(page, (void *)object);
		return;
	}
	...
}

Can this corrupt slab state or oops, and leak the vmalloc area?  Note the
kfree() on the success path is the common case, so it would be hit on every
successful indirection-table update on such a device, not only on control
virtqueue failure.

> @@ -6171,13 +6195,17 @@ static int virtnet_set_features(struct net_device *dev,
>  	}
>  
>  	if ((dev->features ^ features) & NETIF_F_RXHASH) {
> +		__le32 hash_types = vi->rss_hdr->hash_types;
> +
>  		if (features & NETIF_F_RXHASH)
>  			vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
>  		else
>  			vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
>  
> -		if (!virtnet_commit_rss_command(vi))
> +		if (!virtnet_commit_rss_command(vi)) {
> +			vi->rss_hdr->hash_types = hash_types;
>  			return -EINVAL;
> +		}
>  	}

[Severity: Medium]
This is a pre-existing issue, but since partial rollback is being added
here: the guest-offload change earlier in the same function has already
been committed to the device by the time this RSS failure returns
-EINVAL:

drivers/net/virtio_net.c:virtnet_set_features() {
	...
		err = virtnet_set_guest_offloads(vi, offloads);
		if (err)
			return err;
		vi->guest_offloads = offloads;
	...
}

The core does not apply the requested feature set when ndo_set_features()
fails:

net/core/dev.c:__netdev_update_features() {
	if (unlikely(err < 0)) {
		netdev_err(dev,
			"set_features() failed (%d); wanted %pNF, left %pNF\n",
			err, &features, &dev->features);
		return -1;
	}
	...
}

So a single request toggling both rx-gro-hw and rx-hashing, where the
offload command succeeds and virtnet_commit_rss_command() then fails,
leaves dev->features with the old NETIF_F_GRO_HW state while the device
and vi->guest_offloads hold the new one.  Since vi->guest_offloads was
updated, can a later dev->features ^ features evaluation decide no
reprogramming is needed and make the divergence persistent?  Would
re-issuing virtnet_set_guest_offloads() with the previous mask and
restoring vi->guest_offloads before returning the error be appropriate
here?
-- 
pw-bot: cr