Re: [PATCH net-next v3 1/3] net: libwx: add support for set_ringparam in wx_ethtool_ops_vf

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
Subject: net: libwx: add support for set_ringparam in wx_ethtool_ops_vf

This commit adds support for the set_ringparam ethtool operation in 
wx_ethtool_ops_vf, enabling ring size configuration for ngbevf and txgbevf.
It also modifies wx_set_ring() to return an error code instead of void to
propagate allocation failures.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index d042567b81284..2e5dba88d04a1 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
[ ... ]
> @@ -3248,8 +3248,8 @@ netdev_features_t wx_features_check(struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL(wx_features_check);
>  
> -void wx_set_ring(struct wx *wx, u32 new_tx_count,
> -		 u32 new_rx_count, struct wx_ring *temp_ring)
> +int wx_set_ring(struct wx *wx, u32 new_tx_count,
> +		u32 new_rx_count, struct wx_ring *temp_ring)

[Severity: High]
Since the return type of wx_set_ring() was changed to propagate errors, do 
existing callers in txgbe and ngbe need to be updated to check this return 
value? 

If they still ignore the return value, an allocation failure will silently 
leave userspace thinking the operation succeeded.

>  {
>  	int i, err = 0;
>  
[ ... ]
> @@ -3299,7 +3299,7 @@ void wx_set_ring(struct wx *wx, u32 new_tx_count,
>  					i--;
>  					wx_free_rx_resources(&temp_ring[i]);
>  				}
> -				return;
> +				return err;

[Severity: Medium]
This isn't a bug introduced by this patch, but does this break configuration 
atomicity?

If wx_setup_rx_resources() fails here, the TX ring changes were already 
committed and old TX resources freed earlier in this function. This leaves 
the device in a partially updated state with the new TX ring size but the 
old RX ring size, rather than cleanly rolling back the entire ethtool 
operation.

[Severity: High]
This is also a pre-existing issue, but does this error path free 
uninitialized pointers? 

The temp_ring array is created via a shallow memcpy from the old ring, 
inheriting the old next_to_alloc index. Since wx_setup_rx_resources() 
allocates rx_buffer_info via kvmalloc (which doesn't zero memory) and 
doesn't reset next_to_alloc, wx_free_rx_resources() will call 
wx_clean_rx_ring() which iterates up to the inherited next_to_alloc index.

Will this pass uninitialized garbage pointers to dev_kfree_skb() and 
page_pool_put_full_page(), potentially causing a kernel panic?

>  			}
>  		}
>  
[ ... ]