[PATCH net-next 0/3] ethtool ringparam upper bound

[PATCH net-next 0/3] ethtool ringparam upper bound

[Tariq Toukan]

Hi Dave,

This patchset by Jenny adds sanity checks in ethtool ringparam
operation for input upper bounds, similarly to what's done in
ethtool_set_channels.

The checks are added in patch 1, using a call to get_ringparam
prior to calling set_ringparam NDO.

Patch 2 changes the function's behavior in mlx4_en, so that
it returns an error for out-of-range input, instead of rounding
it to closest valid, similar to mlx5e.

Patch 3 removes the upper bound checks in mlx5e_ethtool_set_ringparam
as it becomes redundant.

Series generated against net-next commit:
f66faae2f80a Merge branch 'ipv6-ipv4-nexthop-align'

Thanks,
Tariq.


Eugenia Emantayev (3):
  ethtool: Ensure new ring parameters are within bounds during
    SRINGPARAM
  net/mlx4_en: Align behavior of set ring size flow via ethtool
  net/mlx5e: Remove redundant checks in set_ringparam

 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c      | 17 +++++++++++++----
 drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 15 ---------------
 net/core/ethtool.c                                   | 13 +++++++++++--
 3 files changed, 24 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/3] ethtool: Ensure new ring parameters are within bounds during SRINGPARAM

[Tariq Toukan]

From: Eugenia Emantayev <eugenia@mellanox.com>

Add a sanity check to ensure that all requested ring parameters
are within bounds, which should reduce errors in driver implementation.

Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 net/core/ethtool.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 50a79203043b..9ea7cd52fde0 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1704,14 +1704,23 @@ static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr)
 
 static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
 {
-	struct ethtool_ringparam ringparam;
+	struct ethtool_ringparam ringparam, max = { .cmd = ETHTOOL_GRINGPARAM };
 
-	if (!dev->ethtool_ops->set_ringparam)
+	if (!dev->ethtool_ops->set_ringparam || !dev->ethtool_ops->get_ringparam)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&ringparam, useraddr, sizeof(ringparam)))
 		return -EFAULT;
 
+	dev->ethtool_ops->get_ringparam(dev, &max);
+
+	/* ensure new ring parameters are within the maximums */
+	if (ringparam.rx_pending > max.rx_max_pending ||
+	    ringparam.rx_mini_pending > max.rx_mini_max_pending ||
+	    ringparam.rx_jumbo_pending > max.rx_jumbo_max_pending ||
+	    ringparam.tx_pending > max.tx_max_pending)
+		return -EINVAL;
+
 	return dev->ethtool_ops->set_ringparam(dev, &ringparam);
 }
 
-- 
1.8.3.1

Re: [PATCH net-next 1/3] ethtool: Ensure new ring parameters are within bounds during SRINGPARAM

[Jakub Kicinski]

On Mon,  8 Jan 2018 16:00:24 +0200, Tariq Toukan wrote:
> From: Eugenia Emantayev <eugenia@mellanox.com>
> 
> Add a sanity check to ensure that all requested ring parameters
> are within bounds, which should reduce errors in driver implementation.

(y)

> Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> ---
>  net/core/ethtool.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 50a79203043b..9ea7cd52fde0 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1704,14 +1704,23 @@ static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr)
>  
>  static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
>  {
> -	struct ethtool_ringparam ringparam;
> +	struct ethtool_ringparam ringparam, max = { .cmd = ETHTOOL_GRINGPARAM };
>  
> -	if (!dev->ethtool_ops->set_ringparam)
> +	if (!dev->ethtool_ops->set_ringparam || !dev->ethtool_ops->get_ringparam)
>  		return -EOPNOTSUPP;
>  
>  	if (copy_from_user(&ringparam, useraddr, sizeof(ringparam)))
>  		return -EFAULT;
>  
> +	dev->ethtool_ops->get_ringparam(dev, &max);

Perhaps check the return value here?  It's pretty unlikely but
get_ringparam may fail.

> +	/* ensure new ring parameters are within the maximums */
> +	if (ringparam.rx_pending > max.rx_max_pending ||
> +	    ringparam.rx_mini_pending > max.rx_mini_max_pending ||
> +	    ringparam.rx_jumbo_pending > max.rx_jumbo_max_pending ||
> +	    ringparam.tx_pending > max.tx_max_pending)
> +		return -EINVAL;
> +
>  	return dev->ethtool_ops->set_ringparam(dev, &ringparam);
>  }
>  

Re: [PATCH net-next 1/3] ethtool: Ensure new ring parameters are within bounds during SRINGPARAM

[Tariq Toukan]

On 09/01/2018 4:23 AM, Jakub Kicinski wrote:
> On Mon,  8 Jan 2018 16:00:24 +0200, Tariq Toukan wrote:
>> From: Eugenia Emantayev <eugenia@mellanox.com>
>>
>> Add a sanity check to ensure that all requested ring parameters
>> are within bounds, which should reduce errors in driver implementation.
> 
> (y)
> 
>> Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
>> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
>> ---
>>   net/core/ethtool.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index 50a79203043b..9ea7cd52fde0 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -1704,14 +1704,23 @@ static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr)
>>   
>>   static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
>>   {
>> -	struct ethtool_ringparam ringparam;
>> +	struct ethtool_ringparam ringparam, max = { .cmd = ETHTOOL_GRINGPARAM };
>>   
>> -	if (!dev->ethtool_ops->set_ringparam)
>> +	if (!dev->ethtool_ops->set_ringparam || !dev->ethtool_ops->get_ringparam)
>>   		return -EOPNOTSUPP;
>>   
>>   	if (copy_from_user(&ringparam, useraddr, sizeof(ringparam)))
>>   		return -EFAULT;
>>   
>> +	dev->ethtool_ops->get_ringparam(dev, &max);
> 
> Perhaps check the return value here?  It's pretty unlikely but
> get_ringparam may fail.
> 

get_ringparam NDO returns void.

>> +	/* ensure new ring parameters are within the maximums */
>> +	if (ringparam.rx_pending > max.rx_max_pending ||
>> +	    ringparam.rx_mini_pending > max.rx_mini_max_pending ||
>> +	    ringparam.rx_jumbo_pending > max.rx_jumbo_max_pending ||
>> +	    ringparam.tx_pending > max.tx_max_pending)
>> +		return -EINVAL;
>> +
>>   	return dev->ethtool_ops->set_ringparam(dev, &ringparam);
>>   }
>>   

Re: [PATCH net-next 1/3] ethtool: Ensure new ring parameters are within bounds during SRINGPARAM

[Jakub Kicinski]

On Tue, 9 Jan 2018 09:30:05 +0200
Tariq Toukan <tariqt@mellanox.com> wrote:
> >> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> >> index 50a79203043b..9ea7cd52fde0 100644
> >> --- a/net/core/ethtool.c
> >> +++ b/net/core/ethtool.c
> >> @@ -1704,14 +1704,23 @@ static int ethtool_get_ringparam(struct
> >> net_device *dev, void __user *useraddr) 
> >>   static int ethtool_set_ringparam(struct net_device *dev, void
> >> __user *useraddr) {
> >> -	struct ethtool_ringparam ringparam;
> >> +	struct ethtool_ringparam ringparam, max = { .cmd =
> >> ETHTOOL_GRINGPARAM }; 
> >> -	if (!dev->ethtool_ops->set_ringparam)
> >> +	if (!dev->ethtool_ops->set_ringparam
> >> || !dev->ethtool_ops->get_ringparam) return -EOPNOTSUPP;
> >>   
> >>   	if (copy_from_user(&ringparam, useraddr,
> >> sizeof(ringparam))) return -EFAULT;
> >>   
> >> +	dev->ethtool_ops->get_ringparam(dev, &max);  
> > 
> > Perhaps check the return value here?  It's pretty unlikely but
> > get_ringparam may fail.
> >   
> 
> get_ringparam NDO returns void.

Ah, you're right, I looked at the return of ethtool_get_ringparam().

[PATCH net-next 2/3] net/mlx4_en: Align behavior of set ring size flow via ethtool

[Tariq Toukan]

From: Eugenia Emantayev <eugenia@mellanox.com>

In current implementation, any requested RX/TX ring size value
that is less than minimum is silently casted to nearest valid value.
Update this behavior to align with mlx5 behavior by printing warning
in dmesg and remaining the size unchanged.
Kernel is responsible for verifying against the maximum.

Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index bf1f04164885..ebc1f566a4d9 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1094,12 +1094,21 @@ static int mlx4_en_set_ringparam(struct net_device *dev,
 	if (param->rx_jumbo_pending || param->rx_mini_pending)
 		return -EINVAL;
 
+	if (param->rx_pending < MLX4_EN_MIN_RX_SIZE) {
+		en_warn(priv, "%s: rx_pending (%d) < min (%d)\n",
+			__func__, param->rx_pending,
+			MLX4_EN_MIN_RX_SIZE);
+		return -EINVAL;
+	}
+	if (param->tx_pending < MLX4_EN_MIN_TX_SIZE) {
+		en_warn(priv, "%s: tx_pending (%d) < min (%lu)\n",
+			__func__, param->tx_pending,
+			MLX4_EN_MIN_TX_SIZE);
+		return -EINVAL;
+	}
+
 	rx_size = roundup_pow_of_two(param->rx_pending);
-	rx_size = max_t(u32, rx_size, MLX4_EN_MIN_RX_SIZE);
-	rx_size = min_t(u32, rx_size, MLX4_EN_MAX_RX_SIZE);
 	tx_size = roundup_pow_of_two(param->tx_pending);
-	tx_size = max_t(u32, tx_size, MLX4_EN_MIN_TX_SIZE);
-	tx_size = min_t(u32, tx_size, MLX4_EN_MAX_TX_SIZE);
 
 	if (rx_size == (priv->port_up ? priv->rx_ring[0]->actual_size :
 					priv->rx_ring[0]->size) &&
-- 
1.8.3.1

[PATCH net-next 3/3] net/mlx5e: Remove redundant checks in set_ringparam

[Tariq Toukan]

From: Eugenia Emantayev <eugenia@mellanox.com>

Since the checks are done in upper layer ethtool code,
checks in driver are not needed any more.

Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 8f05efa5c829..1554780d1810 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -296,7 +296,6 @@ int mlx5e_ethtool_set_ringparam(struct mlx5e_priv *priv,
 	struct mlx5e_channels new_channels = {};
 	u32 rx_pending_wqes;
 	u32 min_rq_size;
-	u32 max_rq_size;
 	u8 log_rq_size;
 	u8 log_sq_size;
 	u32 num_mtts;
@@ -315,8 +314,6 @@ int mlx5e_ethtool_set_ringparam(struct mlx5e_priv *priv,
 
 	min_rq_size = mlx5e_rx_wqes_to_packets(priv, rq_wq_type,
 					       1 << mlx5_min_log_rq_size(rq_wq_type));
-	max_rq_size = mlx5e_rx_wqes_to_packets(priv, rq_wq_type,
-					       1 << mlx5_max_log_rq_size(rq_wq_type));
 	rx_pending_wqes = mlx5e_packets_to_rx_wqes(priv, rq_wq_type,
 						   param->rx_pending);
 
@@ -326,12 +323,6 @@ int mlx5e_ethtool_set_ringparam(struct mlx5e_priv *priv,
 			    min_rq_size);
 		return -EINVAL;
 	}
-	if (param->rx_pending > max_rq_size) {
-		netdev_info(priv->netdev, "%s: rx_pending (%d) > max (%d)\n",
-			    __func__, param->rx_pending,
-			    max_rq_size);
-		return -EINVAL;
-	}
 
 	num_mtts = MLX5E_REQUIRED_MTTS(rx_pending_wqes);
 	if (priv->channels.params.rq_wq_type == MLX5_WQ_TYPE_LINKED_LIST_STRIDING_RQ &&
@@ -347,12 +338,6 @@ int mlx5e_ethtool_set_ringparam(struct mlx5e_priv *priv,
 			    1 << MLX5E_PARAMS_MINIMUM_LOG_SQ_SIZE);
 		return -EINVAL;
 	}
-	if (param->tx_pending > (1 << MLX5E_PARAMS_MAXIMUM_LOG_SQ_SIZE)) {
-		netdev_info(priv->netdev, "%s: tx_pending (%d) > max (%d)\n",
-			    __func__, param->tx_pending,
-			    1 << MLX5E_PARAMS_MAXIMUM_LOG_SQ_SIZE);
-		return -EINVAL;
-	}
 
 	log_rq_size = order_base_2(rx_pending_wqes);
 	log_sq_size = order_base_2(param->tx_pending);
-- 
1.8.3.1

Re: [PATCH net-next 0/3] ethtool ringparam upper bound

[David Miller]

From: Tariq Toukan <tariqt@mellanox.com>
Date: Mon,  8 Jan 2018 16:00:23 +0200

> This patchset by Jenny adds sanity checks in ethtool ringparam
> operation for input upper bounds, similarly to what's done in
> ethtool_set_channels.
> 
> The checks are added in patch 1, using a call to get_ringparam
> prior to calling set_ringparam NDO.
> 
> Patch 2 changes the function's behavior in mlx4_en, so that
> it returns an error for out-of-range input, instead of rounding
> it to closest valid, similar to mlx5e.
> 
> Patch 3 removes the upper bound checks in mlx5e_ethtool_set_ringparam
> as it becomes redundant.
> 
> Series generated against net-next commit:
> f66faae2f80a Merge branch 'ipv6-ipv4-nexthop-align'

Series applied, thank you.