[PATCH net 0/2] intel: Interpret .set_channels() input differently

[PATCH net 0/2] intel: Interpret .set_channels() input differently

[Jacob Keller]

The ice and idpf drivers can trigger a crash with AF_XDP due to incorrect
interpretation of the asymmetric Tx and Rx parameters in their
.set_channels() implementations:

1. ethtool -l <IFNAME> -> combined: 40
2. Attach AF_XDP to queue 30
3. ethtool -L <IFNAME> rx 15 tx 15
   combined number is not specified, so command becomes {rx_count = 15,
   tx_count = 15, combined_count = 40}.
4. ethnl_set_channels checks, if there are any AF_XDP of queues from the
   new (combined_count + rx_count) to the old one, so from 55 to 40, check
   does not trigger.
5. the driver interprets `rx 15 tx 15` as 15 combined channels and deletes
   the queue that AF_XDP is attached to.

This is fundamentally a problem with interpreting a request for asymmetric
queues as symmetric combined queues.

Fix the ice and idpf drivers to stop interpreting such requests as a
request for combined queues. Due to current driver design for both ice and
idpf, it is not possible to support requests of the same count of Tx and Rx
queues with independent interrupts, (i.e. ethtool -L <IFNAME> rx 15 tx 15)
so such requests are now rejected.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Larysa Zaremba (2):
      ice: Interpret .set_channels() input differently
      idpf: Interpret .set_channels() input differently

 drivers/net/ethernet/intel/ice/ice_ethtool.c   | 22 ++++++----------------
 drivers/net/ethernet/intel/idpf/idpf_ethtool.c | 21 ++++++---------------
 2 files changed, 12 insertions(+), 31 deletions(-)
---
base-commit: aea27a92a41dae14843f92c79e9e42d8f570105c
change-id: 20240514-iwl-net-2024-05-14-set-channels-fixes-25be6f04a86d

Best regards,
-- 
Jacob Keller <jacob.e.keller@intel.com>

[PATCH net 1/2] ice: Interpret .set_channels() input differently

[Jacob Keller]

From: Larysa Zaremba <larysa.zaremba@intel.com>

A bug occurs because a safety check guarding AF_XDP-related queues in
ethnl_set_channels(), does not trigger. This happens, because kernel and
ice driver interpret the ethtool command differently.

How the bug occurs:
1. ethtool -l <IFNAME> -> combined: 40
2. Attach AF_XDP to queue 30
3. ethtool -L <IFNAME> rx 15 tx 15
   combined number is not specified, so command becomes {rx_count = 15,
   tx_count = 15, combined_count = 40}.
4. ethnl_set_channels checks, if there are any AF_XDP of queues from the
   new (combined_count + rx_count) to the old one, so from 55 to 40, check
   does not trigger.
5. ice interprets `rx 15 tx 15` as 15 combined channels and deletes the
   queue that AF_XDP is attached to.

Interpret the command in a way that is more consistent with ethtool
manual [0] (--show-channels and --set-channels).

Considering that in the ice driver only the difference between RX and TX
queues forms dedicated channels, change the correct way to set number of
channels to:

ethtool -L <IFNAME> combined 10 /* For symmetric queues */
ethtool -L <IFNAME> combined 8 tx 2 rx 0 /* For asymmetric queues */

[0] https://man7.org/linux/man-pages/man8/ethtool.8.html

Fixes: 87324e747fde ("ice: Implement ethtool ops for channels")
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com>
Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 78b833b3e1d7..d91f41f61bce 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3593,7 +3593,6 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 	struct ice_pf *pf = vsi->back;
 	int new_rx = 0, new_tx = 0;
 	bool locked = false;
-	u32 curr_combined;
 	int ret = 0;
 
 	/* do not support changing channels in Safe Mode */
@@ -3615,22 +3614,13 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 		return -EOPNOTSUPP;
 	}
 
-	curr_combined = ice_get_combined_cnt(vsi);
+	if (!ch->combined_count) {
+		netdev_err(dev, "Please specify at least 1 combined channel\n");
+		return -EINVAL;
+	}
 
-	/* these checks are for cases where user didn't specify a particular
-	 * value on cmd line but we get non-zero value anyway via
-	 * get_channels(); look at ethtool.c in ethtool repository (the user
-	 * space part), particularly, do_schannels() routine
-	 */
-	if (ch->rx_count == vsi->num_rxq - curr_combined)
-		ch->rx_count = 0;
-	if (ch->tx_count == vsi->num_txq - curr_combined)
-		ch->tx_count = 0;
-	if (ch->combined_count == curr_combined)
-		ch->combined_count = 0;
-
-	if (!(ch->combined_count || (ch->rx_count && ch->tx_count))) {
-		netdev_err(dev, "Please specify at least 1 Rx and 1 Tx channel\n");
+	if (ch->rx_count && ch->tx_count) {
+		netdev_err(dev, "Dedicated RX or TX channels cannot be used simultaneously\n");
 		return -EINVAL;
 	}
 

-- 
2.44.0.53.g0f9d4d28b7e6

Re: [PATCH net 1/2] ice: Interpret .set_channels() input differently

[Simon Horman]

On Tue, May 14, 2024 at 11:51:12AM -0700, Jacob Keller wrote:
> From: Larysa Zaremba <larysa.zaremba@intel.com>
> 
> A bug occurs because a safety check guarding AF_XDP-related queues in
> ethnl_set_channels(), does not trigger. This happens, because kernel and
> ice driver interpret the ethtool command differently.
> 
> How the bug occurs:
> 1. ethtool -l <IFNAME> -> combined: 40
> 2. Attach AF_XDP to queue 30
> 3. ethtool -L <IFNAME> rx 15 tx 15
>    combined number is not specified, so command becomes {rx_count = 15,
>    tx_count = 15, combined_count = 40}.
> 4. ethnl_set_channels checks, if there are any AF_XDP of queues from the
>    new (combined_count + rx_count) to the old one, so from 55 to 40, check
>    does not trigger.
> 5. ice interprets `rx 15 tx 15` as 15 combined channels and deletes the
>    queue that AF_XDP is attached to.
> 
> Interpret the command in a way that is more consistent with ethtool
> manual [0] (--show-channels and --set-channels).
> 
> Considering that in the ice driver only the difference between RX and TX
> queues forms dedicated channels, change the correct way to set number of
> channels to:
> 
> ethtool -L <IFNAME> combined 10 /* For symmetric queues */
> ethtool -L <IFNAME> combined 8 tx 2 rx 0 /* For asymmetric queues */
> 
> [0] https://man7.org/linux/man-pages/man8/ethtool.8.html
> 
> Fixes: 87324e747fde ("ice: Implement ethtool ops for channels")
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com>
> Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com>
> Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net 2/2] idpf: Interpret .set_channels() input differently

[Jacob Keller]

From: Larysa Zaremba <larysa.zaremba@intel.com>

Unlike ice, idpf does not check, if user has requested at least 1 combined
channel. Instead, it relies on a check in the core code. Unfortunately, the
check does not trigger for us because of the hacky .set_channels()
interpretation logic that is not consistent with the core code.

This naturally leads to user being able to trigger a crash with an invalid
input. This is how:

1. ethtool -l <IFNAME> -> combined: 40
2. ethtool -L <IFNAME> rx 0 tx 0
   combined number is not specified, so command becomes {rx_count = 0,
   tx_count = 0, combined_count = 40}.
3. ethnl_set_channels checks, if there is at least 1 RX and 1 TX channel,
   comparing (combined_count + rx_count) and (combined_count + tx_count)
   to zero. Obviously, (40 + 0) is greater than zero, so the core code
   deems the input OK.
4. idpf interprets `rx 0 tx 0` as 0 channels and tries to proceed with such
   configuration.

The issue has to be solved fundamentally, as current logic is also known to
cause AF_XDP problems in ice [0].

Interpret the command in a way that is more consistent with ethtool
manual [1] (--show-channels and --set-channels) and new ice logic.

Considering that in the idpf driver only the difference between RX and TX
queues forms dedicated channels, change the correct way to set number of
channels to:

ethtool -L <IFNAME> combined 10 /* For symmetric queues */
ethtool -L <IFNAME> combined 8 tx 2 rx 0 /* For asymmetric queues */

[0] https://lore.kernel.org/netdev/20240418095857.2827-1-larysa.zaremba@intel.com/
[1] https://man7.org/linux/man-pages/man8/ethtool.8.html

Fixes: 02cbfba1add5 ("idpf: add ethtool callbacks")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Igor Bagnucki <igor.bagnucki@intel.com>
Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_ethtool.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
index 986d429d1175..1cf3067a9c31 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_ethtool.c
@@ -222,14 +222,19 @@ static int idpf_set_channels(struct net_device *netdev,
 			     struct ethtool_channels *ch)
 {
 	struct idpf_vport_config *vport_config;
-	u16 combined, num_txq, num_rxq;
 	unsigned int num_req_tx_q;
 	unsigned int num_req_rx_q;
 	struct idpf_vport *vport;
+	u16 num_txq, num_rxq;
 	struct device *dev;
 	int err = 0;
 	u16 idx;
 
+	if (ch->rx_count && ch->tx_count) {
+		netdev_err(netdev, "Dedicated RX or TX channels cannot be used simultaneously\n");
+		return -EINVAL;
+	}
+
 	idpf_vport_ctrl_lock(netdev);
 	vport = idpf_netdev_to_vport(netdev);
 
@@ -239,20 +244,6 @@ static int idpf_set_channels(struct net_device *netdev,
 	num_txq = vport_config->user_config.num_req_tx_qs;
 	num_rxq = vport_config->user_config.num_req_rx_qs;
 
-	combined = min(num_txq, num_rxq);
-
-	/* these checks are for cases where user didn't specify a particular
-	 * value on cmd line but we get non-zero value anyway via
-	 * get_channels(); look at ethtool.c in ethtool repository (the user
-	 * space part), particularly, do_schannels() routine
-	 */
-	if (ch->combined_count == combined)
-		ch->combined_count = 0;
-	if (ch->combined_count && ch->rx_count == num_rxq - combined)
-		ch->rx_count = 0;
-	if (ch->combined_count && ch->tx_count == num_txq - combined)
-		ch->tx_count = 0;
-
 	num_req_tx_q = ch->combined_count + ch->tx_count;
 	num_req_rx_q = ch->combined_count + ch->rx_count;
 

-- 
2.44.0.53.g0f9d4d28b7e6

Re: [PATCH net 0/2] intel: Interpret .set_channels() input differently

[Larysa Zaremba]

On Tue, May 14, 2024 at 11:51:11AM -0700, Jacob Keller wrote:
> The ice and idpf drivers can trigger a crash with AF_XDP due to incorrect
> interpretation of the asymmetric Tx and Rx parameters in their
> .set_channels() implementations:
> 
> 1. ethtool -l <IFNAME> -> combined: 40
> 2. Attach AF_XDP to queue 30
> 3. ethtool -L <IFNAME> rx 15 tx 15
>    combined number is not specified, so command becomes {rx_count = 15,
>    tx_count = 15, combined_count = 40}.
> 4. ethnl_set_channels checks, if there are any AF_XDP of queues from the
>    new (combined_count + rx_count) to the old one, so from 55 to 40, check
>    does not trigger.
> 5. the driver interprets `rx 15 tx 15` as 15 combined channels and deletes
>    the queue that AF_XDP is attached to.
> 
> This is fundamentally a problem with interpreting a request for asymmetric
> queues as symmetric combined queues.
> 
> Fix the ice and idpf drivers to stop interpreting such requests as a
> request for combined queues. Due to current driver design for both ice and
> idpf, it is not possible to support requests of the same count of Tx and Rx
> queues with independent interrupts, (i.e. ethtool -L <IFNAME> rx 15 tx 15)
> so such requests are now rejected.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---

Please, do not merge, first patch contains a redundant check

if (!ch->combined_count)

I will send another version.

> Larysa Zaremba (2):
>       ice: Interpret .set_channels() input differently
>       idpf: Interpret .set_channels() input differently
> 
>  drivers/net/ethernet/intel/ice/ice_ethtool.c   | 22 ++++++----------------
>  drivers/net/ethernet/intel/idpf/idpf_ethtool.c | 21 ++++++---------------
>  2 files changed, 12 insertions(+), 31 deletions(-)
> ---
> base-commit: aea27a92a41dae14843f92c79e9e42d8f570105c
> change-id: 20240514-iwl-net-2024-05-14-set-channels-fixes-25be6f04a86d
> 
> Best regards,
> -- 
> Jacob Keller <jacob.e.keller@intel.com>
> 

Re: [PATCH net 0/2] intel: Interpret .set_channels() input differently

[Jacob Keller]

On 5/16/2024 1:44 AM, Larysa Zaremba wrote:
> On Tue, May 14, 2024 at 11:51:11AM -0700, Jacob Keller wrote:
>> The ice and idpf drivers can trigger a crash with AF_XDP due to incorrect
>> interpretation of the asymmetric Tx and Rx parameters in their
>> .set_channels() implementations:
>>
>> 1. ethtool -l <IFNAME> -> combined: 40
>> 2. Attach AF_XDP to queue 30
>> 3. ethtool -L <IFNAME> rx 15 tx 15
>>    combined number is not specified, so command becomes {rx_count = 15,
>>    tx_count = 15, combined_count = 40}.
>> 4. ethnl_set_channels checks, if there are any AF_XDP of queues from the
>>    new (combined_count + rx_count) to the old one, so from 55 to 40, check
>>    does not trigger.
>> 5. the driver interprets `rx 15 tx 15` as 15 combined channels and deletes
>>    the queue that AF_XDP is attached to.
>>
>> This is fundamentally a problem with interpreting a request for asymmetric
>> queues as symmetric combined queues.
>>
>> Fix the ice and idpf drivers to stop interpreting such requests as a
>> request for combined queues. Due to current driver design for both ice and
>> idpf, it is not possible to support requests of the same count of Tx and Rx
>> queues with independent interrupts, (i.e. ethtool -L <IFNAME> rx 15 tx 15)
>> so such requests are now rejected.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
> 
> Please, do not merge, first patch contains a redundant check
> 
> if (!ch->combined_count)
> 
> I will send another version.

I looked at the ethnl_set_channels and was at first confused because
there is no explicit check for just !ch->combined_count. That makes
sense since it was previously possible to just set both tx and rx which
we were incorrectly interpreting as combined channels.

The core code *does* check that we have at least one Tx and one Rx
channel by checking the following conditions:

>         /* ensure there is at least one RX and one TX channel */
>         if (!channels.combined_count && !channels.rx_count)
>                 err_attr = ETHTOOL_A_CHANNELS_RX_COUNT;
>         else if (!channels.combined_count && !channels.tx_count)
>                 err_attr = ETHTOOL_A_CHANNELS_TX_COUNT;
>         else
>                 err_attr = 0;


This combined with our added check in the driver that we can't have both
ch->rx_count and ch->tx_count set, this effectively covers the same test
that ch->combined_count covers, so its unnecessary to waste time
checking it again.

Makes sense.

Thanks,
Jake