diff mbox

iwlwifi: pcie: compare with number of IRQs requested for, not number of CPUs

Message ID 20180529072517.30135-1-luca@coelho.fi (mailing list archive)
State Accepted
Commit ab1068d6866e28bf6427ceaea681a381e5870a4a
Delegated to: Kalle Valo
Headers show

Commit Message

Luca Coelho May 29, 2018, 7:25 a.m. UTC 
From: Hao Wei Tee <angelsl@in04.sg>

When there are 16 or more logical CPUs, we request for
`IWL_MAX_RX_HW_QUEUES` (16) IRQs only as we limit to that number of
IRQs, but later on we compare the number of IRQs returned to
nr_online_cpus+2 instead of max_irqs, the latter being what we
actually asked for. This ends up setting num_rx_queues to 17 which
causes lots of out-of-bounds array accesses later on.

Compare to max_irqs instead, and also add an assertion in case
num_rx_queues > IWM_MAX_RX_HW_QUEUES.

This fixes https://bugzilla.kernel.org/show_bug.cgi?id=199551

Signed-off-by: Hao Wei Tee <angelsl@in04.sg>
Tested-by: Sara Sharon <sara.sharon@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Kalle Valo May 29, 2018, 7:31 a.m. UTC | #1 
Luca Coelho <luca@coelho.fi> writes:

> From: Hao Wei Tee <angelsl@in04.sg>
>
> When there are 16 or more logical CPUs, we request for
> `IWL_MAX_RX_HW_QUEUES` (16) IRQs only as we limit to that number of
> IRQs, but later on we compare the number of IRQs returned to
> nr_online_cpus+2 instead of max_irqs, the latter being what we
> actually asked for. This ends up setting num_rx_queues to 17 which
> causes lots of out-of-bounds array accesses later on.
>
> Compare to max_irqs instead, and also add an assertion in case
> num_rx_queues > IWM_MAX_RX_HW_QUEUES.
>
> This fixes https://bugzilla.kernel.org/show_bug.cgi?id=199551
>
> Signed-off-by: Hao Wei Tee <angelsl@in04.sg>
> Tested-by: Sara Sharon <sara.sharon@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>

Like we discussed I'll take this directly for 4.17 and add:

Fixes: 2e5d4a8f61dc ("iwlwifi: pcie: Add new configuration to enable MSIX")
Kalle Valo May 29, 2018, 7:40 a.m. UTC | #2 
Luciano Coelho <luca@coelho.fi> wrote:

> From: Hao Wei Tee <angelsl@in04.sg>
> 
> When there are 16 or more logical CPUs, we request for
> `IWL_MAX_RX_HW_QUEUES` (16) IRQs only as we limit to that number of
> IRQs, but later on we compare the number of IRQs returned to
> nr_online_cpus+2 instead of max_irqs, the latter being what we
> actually asked for. This ends up setting num_rx_queues to 17 which
> causes lots of out-of-bounds array accesses later on.
> 
> Compare to max_irqs instead, and also add an assertion in case
> num_rx_queues > IWM_MAX_RX_HW_QUEUES.
> 
> This fixes https://bugzilla.kernel.org/show_bug.cgi?id=199551
> 
> Fixes: 2e5d4a8f61dc ("iwlwifi: pcie: Add new configuration to enable MSIX")
> Signed-off-by: Hao Wei Tee <angelsl@in04.sg>
> Tested-by: Sara Sharon <sara.sharon@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>

Patch applied to wireless-drivers.git, thanks.

ab1068d6866e iwlwifi: pcie: compare with number of IRQs requested for, not number of CPUs
Luca Coelho May 29, 2018, 2:50 p.m. UTC | #3 
(note: your previous email didn't reach the linux-wireless mailing
list, since it drops all html emails without notice).


On Tue, 2018-05-29 at 07:35 -0700, Jonathan wrote:
> Do you think this will land in a 4.17 RC version, or in 4.17
> official, or in a 4.17.1 subsequent release?

We will try to get it into 4.17.  We are currently in 4.17-rc7, so
there probably won't be another rc release for 4.17.

If it doesn't go in, then we will most likely get it in 4.17.1 (or
maybe 4.17.2, depends on Greg's schedule).

Again, we can't really promise anything, because the patch needs to be
accepts at our assumed times in the whole chain, and is out of our
control.

--
Cheers,
Luca.
diff mbox

Patch

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index f8a0234d332c..5517ea4c2aa0 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -1590,14 +1590,13 @@  static void iwl_pcie_set_interrupt_capa(struct pci_dev *pdev,
 					struct iwl_trans *trans)
 {
 	struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans);
-	int max_irqs, num_irqs, i, ret, nr_online_cpus;
+	int max_irqs, num_irqs, i, ret;
 	u16 pci_cmd;
 
 	if (!trans->cfg->mq_rx_supported)
 		goto enable_msi;
 
-	nr_online_cpus = num_online_cpus();
-	max_irqs = min_t(u32, nr_online_cpus + 2, IWL_MAX_RX_HW_QUEUES);
+	max_irqs = min_t(u32, num_online_cpus() + 2, IWL_MAX_RX_HW_QUEUES);
 	for (i = 0; i < max_irqs; i++)
 		trans_pcie->msix_entries[i].entry = i;
 
@@ -1623,16 +1622,17 @@  static void iwl_pcie_set_interrupt_capa(struct pci_dev *pdev,
 	 * Two interrupts less: non rx causes shared with FBQ and RSS.
 	 * More than two interrupts: we will use fewer RSS queues.
 	 */
-	if (num_irqs <= nr_online_cpus) {
+	if (num_irqs <= max_irqs - 2) {
 		trans_pcie->trans->num_rx_queues = num_irqs + 1;
 		trans_pcie->shared_vec_mask = IWL_SHARED_IRQ_NON_RX |
 			IWL_SHARED_IRQ_FIRST_RSS;
-	} else if (num_irqs == nr_online_cpus + 1) {
+	} else if (num_irqs == max_irqs - 1) {
 		trans_pcie->trans->num_rx_queues = num_irqs;
 		trans_pcie->shared_vec_mask = IWL_SHARED_IRQ_NON_RX;
 	} else {
 		trans_pcie->trans->num_rx_queues = num_irqs - 1;
 	}
+	WARN_ON(trans_pcie->trans->num_rx_queues > IWL_MAX_RX_HW_QUEUES);
 
 	trans_pcie->alloc_vecs = num_irqs;
 	trans_pcie->msix_enabled = true;