[PATCH net] eth: fbnic: Avoid rounding zero ring sizes

[PATCH net] eth: fbnic: Avoid rounding zero ring sizes

[Björn Töpel]

roundup_pow_of_two() is undefined for zero. ethtool permits a zero ring
size to reach the driver, where the minimum-size check should reject it.

Validate the requested sizes before rounding them so zero is rejected
deterministically.

Fixes: 6cbf18a05c06 ("eth: fbnic: support ring size configuration")
Suggested-by: Alexander Duyck <alexanderduyck@fb.com>
Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index 0e47088ec44b..8954fcd08cb3 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -334,11 +334,6 @@ fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
 	struct fbnic_net *clone;
 	int err;
 
-	ring->rx_pending	= roundup_pow_of_two(ring->rx_pending);
-	ring->rx_mini_pending	= roundup_pow_of_two(ring->rx_mini_pending);
-	ring->rx_jumbo_pending	= roundup_pow_of_two(ring->rx_jumbo_pending);
-	ring->tx_pending	= roundup_pow_of_two(ring->tx_pending);
-
 	/* These are absolute minimums allowing the device and driver to operate
 	 * but not necessarily guarantee reasonable performance. Settings below
 	 * Rx queue size of 128 and BDQs smaller than 64 are likely suboptimal
@@ -368,6 +363,11 @@ fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
 		return -EINVAL;
 	}
 
+	ring->rx_pending	= roundup_pow_of_two(ring->rx_pending);
+	ring->rx_mini_pending	= roundup_pow_of_two(ring->rx_mini_pending);
+	ring->rx_jumbo_pending	= roundup_pow_of_two(ring->rx_jumbo_pending);
+	ring->tx_pending	= roundup_pow_of_two(ring->tx_pending);
+
 	if (!netif_running(netdev)) {
 		fbnic_set_rings(fbn, ring, kernel_ring);
 		return 0;

base-commit: 18a6fe05fb6e18de29fa90d388bb34044114b3d8
-- 
2.55.0

Re: [PATCH net] eth: fbnic: Avoid rounding zero ring sizes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] In fbnic_set_ringparam()
  (drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c), moving the minimum-…

--

eth: fbnic: Avoid rounding zero ring sizes

The patch moves the roundup_pow_of_two() normalization of the requested
ring sizes in fbnic_set_ringparam() to after the minimum-size checks, so
that a zero ring size coming from userspace is rejected instead of being
fed to roundup_pow_of_two().

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> index 0e47088ec44ba..8954fcd08cb31 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
> @@ -334,11 +334,6 @@ fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
>  	struct fbnic_net *clone;
>  	int err;
>  
> -	ring->rx_pending	= roundup_pow_of_two(ring->rx_pending);
> -	ring->rx_mini_pending	= roundup_pow_of_two(ring->rx_mini_pending);
> -	ring->rx_jumbo_pending	= roundup_pow_of_two(ring->rx_jumbo_pending);
> -	ring->tx_pending	= roundup_pow_of_two(ring->tx_pending);
> -
>  	/* These are absolute minimums allowing the device and driver to operate
>  	 * but not necessarily guarantee reasonable performance. Settings below
>  	 * Rx queue size of 128 and BDQs smaller than 64 are likely suboptimal

[Severity: Medium]
With the roundup moved below the checks, the minimum-size test in
fbnic_set_ringparam() now compares the raw userspace request rather than
the rounded value.  Does this reject nonzero sizes that used to be
accepted?

The effective minimums all evaluate to 64:

drivers/net/ethernet/meta/fbnic/fbnic_txrx.h:
    #define FBNIC_TX_DESC_MIN	roundup_pow_of_two(FBNIC_TX_DESC_WAKEUP)
    #define FBNIC_RX_DESC_MIN	roundup_pow_of_two(FBNIC_MAX_RX_PKT_DESC * 2)
    #define FBNIC_QUEUE_SIZE_MIN		64u

FBNIC_RX_DESC_MIN is 16 and FBNIC_TX_DESC_MIN is 64 for the common
MAX_SKB_FRAGS == 17, so max(FBNIC_QUEUE_SIZE_MIN, ...) is 64 in every
case.

Previously a request like "ethtool -G ethX tx 40" was rounded to 64 first
and then passed the check, ending up with a 64-entry ring.  Now the check
sees 40 and returns -EINVAL with "requested ring size too small".  The
same applies to any rx, rx-mini or rx-jumbo value in 33..63.

The ethtool core does not enforce a minimum, it only validates the maxima:

net/ethtool/rings.c:ethnl_set_rings() {
	...
	/* ensure new ring parameters are within limits */
	if (ringparam.rx_pending > ringparam.rx_max_pending)
	...
}

and ethtool_set_ringparam() in net/ethtool/ioctl.c does the same, so those
values do reach the driver unchanged.

>  	/* These are absolute minimums allowing the device and driver to operate
>  	 * but not necessarily guarantee reasonable performance. Settings below
>  	 * Rx queue size of 128 and BDQs smaller than 64 are likely suboptimal
>  	 * at best.
>  	 */
>  	if (ring->rx_pending < max(FBNIC_QUEUE_SIZE_MIN, FBNIC_RX_DESC_MIN) ||
>  	    ring->rx_mini_pending < FBNIC_QUEUE_SIZE_MIN ||
>  	    ring->rx_jumbo_pending < FBNIC_QUEUE_SIZE_MIN ||
>  	    ring->tx_pending < max(FBNIC_QUEUE_SIZE_MIN, FBNIC_TX_DESC_MIN)) {
>  		NL_SET_ERR_MSG_MOD(extack, "requested ring size too small");
>  		return -EINVAL;
>  	}

> @@ -368,6 +363,11 @@ fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
>  		return -EINVAL;
>  	}
>  
> +	ring->rx_pending	= roundup_pow_of_two(ring->rx_pending);
> +	ring->rx_mini_pending	= roundup_pow_of_two(ring->rx_mini_pending);
> +	ring->rx_jumbo_pending	= roundup_pow_of_two(ring->rx_jumbo_pending);
> +	ring->tx_pending	= roundup_pow_of_two(ring->tx_pending);
> +

[Severity: Medium]
Was the reorder needed to reach the stated goal?  The commit message says
only:

    Validate the requested sizes before rounding them so zero is rejected
    deterministically.

It does not mention that previously accepted non-power-of-two sizes below
the 64-descriptor minimum now start failing with -EINVAL.  Since the patch
carries a Fixes: tag and is therefore a stable-backport candidate, could
the changelog spell out that user-visible change?

Would an explicit rejection of zero, or clamping in place with
max(min, roundup_pow_of_two(...)), address the undefined
roundup_pow_of_two(0) without altering which nonzero requests are
accepted?

>  	if (!netif_running(netdev)) {
>  		fbnic_set_rings(fbn, ring, kernel_ring);
>  		return 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915181229.4159158-1-bjorn%40kernel.org

Re: [PATCH net] eth: fbnic: Avoid rounding zero ring sizes

[Björn Töpel]

netdev-bot+sashiko@kernel.org writes:

>> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>> index 0e47088ec44ba..8954fcd08cb31 100644
>> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>> @@ -334,11 +334,6 @@ fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
>>  	struct fbnic_net *clone;
>>  	int err;
>>  
>> -	ring->rx_pending	= roundup_pow_of_two(ring->rx_pending);
>> -	ring->rx_mini_pending	= roundup_pow_of_two(ring->rx_mini_pending);
>> -	ring->rx_jumbo_pending	= roundup_pow_of_two(ring->rx_jumbo_pending);
>> -	ring->tx_pending	= roundup_pow_of_two(ring->tx_pending);
>> -
>>  	/* These are absolute minimums allowing the device and driver to operate
>>  	 * but not necessarily guarantee reasonable performance. Settings below
>>  	 * Rx queue size of 128 and BDQs smaller than 64 are likely suboptimal
>
> [Severity: Medium]
> With the roundup moved below the checks, the minimum-size test in
> fbnic_set_ringparam() now compares the raw userspace request rather than
> the rounded value.  Does this reject nonzero sizes that used to be
> accepted?
>
> The effective minimums all evaluate to 64:
>
> drivers/net/ethernet/meta/fbnic/fbnic_txrx.h:
>     #define FBNIC_TX_DESC_MIN	roundup_pow_of_two(FBNIC_TX_DESC_WAKEUP)
>     #define FBNIC_RX_DESC_MIN	roundup_pow_of_two(FBNIC_MAX_RX_PKT_DESC * 2)
>     #define FBNIC_QUEUE_SIZE_MIN		64u
>
> FBNIC_RX_DESC_MIN is 16 and FBNIC_TX_DESC_MIN is 64 for the common
> MAX_SKB_FRAGS == 17, so max(FBNIC_QUEUE_SIZE_MIN, ...) is 64 in every
> case.
>
> Previously a request like "ethtool -G ethX tx 40" was rounded to 64 first
> and then passed the check, ending up with a 64-entry ring.  Now the check
> sees 40 and returns -EINVAL with "requested ring size too small".  The
> same applies to any rx, rx-mini or rx-jumbo value in 33..63.
>
> The ethtool core does not enforce a minimum, it only validates the maxima:
>
> net/ethtool/rings.c:ethnl_set_rings() {
> 	...
> 	/* ensure new ring parameters are within limits */
> 	if (ringparam.rx_pending > ringparam.rx_max_pending)
> 	...
> }
>
> and ethtool_set_ringparam() in net/ethtool/ioctl.c does the same, so those
> values do reach the driver unchanged.

Indeed. V2!

Björn