| Message ID | 20240514-iwl-net-2024-05-14-set-channels-fixes-v1-0-eb18d88e30c3@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | intel: Interpret .set_channels() input differently | expand |
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> >
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
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,