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/[email protected]/
[1] https://man7.org/linux/man-pages/man8/ethtool.8.html
Fixes: 02cbfba1add5 ("idpf: add ethtool callbacks")
Reviewed-by: Przemek Kitszel <[email protected]>
Reviewed-by: Igor Bagnucki <[email protected]>
Signed-off-by: Larysa Zaremba <[email protected]>
---
.../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.43.0
> -----Original Message-----
> From: Larysa Zaremba <[email protected]>
> Sent: Friday, April 26, 2024 8:41 AM
> To: Nguyen, Anthony L <[email protected]>; intel-wired-
> [email protected]; [email protected]; [email protected]
> Cc: Zaremba, Larysa <[email protected]>; Brady, Alan
> <[email protected]>; Fijalkowski, Maciej <[email protected]>;
> Brandeburg, Jesse <[email protected]>; Tantilov, Emil S
> <[email protected]>; Linga, Pavan Kumar
> <[email protected]>; Hay, Joshua A <[email protected]>;
> Kitszel, Przemyslaw <[email protected]>; Bagnucki, Igor
> <[email protected]>
> Subject: [PATCH iwl-net] idpf: Interpret .set_channels() input differently
>
> 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-
> [email protected]/
> [1] https://man7.org/linux/man-pages/man8/ethtool.8.html
>
> Fixes: 02cbfba1add5 ("idpf: add ethtool callbacks")
> Reviewed-by: Przemek Kitszel <[email protected]>
> Reviewed-by: Igor Bagnucki <[email protected]>
> Signed-off-by: Larysa Zaremba <[email protected]>
> ---
> .../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
Tested-by: Krishneil Singh <[email protected]>
On Fri, Apr 26, 2024 at 05:41:22PM +0200, Larysa Zaremba wrote:
> 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/[email protected]/
> [1] https://man7.org/linux/man-pages/man8/ethtool.8.html
>
> Fixes: 02cbfba1add5 ("idpf: add ethtool callbacks")
> Reviewed-by: Przemek Kitszel <[email protected]>
> Reviewed-by: Igor Bagnucki <[email protected]>
> Signed-off-by: Larysa Zaremba <[email protected]>
Reviewed-by: Simon Horman <[email protected]>