idpf: fix UAFs when destroying the queues

[ Upstream commit 290f1c033281c1a502a3cd1c53c3a549259c491f ]

The second tagged commit started sometimes (very rarely, but possible)
throwing WARNs from
net/core/page_pool.c:page_pool_disable_direct_recycling().
Turned out idpf frees interrupt vectors with embedded NAPIs *before*
freeing the queues making page_pools' NAPI pointers lead to freed
memory before these pools are destroyed by libeth.
It's not clear whether there are other accesses to the freed vectors
when destroying the queues, but anyway, we usually free queue/interrupt
vectors only when the queues are destroyed and the NAPIs are guaranteed
to not be referenced anywhere.

Invert the allocation and freeing logic making queue/interrupt vectors
be allocated first and freed last. Vectors don't require queues to be
present, so this is safe. Additionally, this change allows to remove
that useless queue->q_vector pointer cleanup, as vectors are still
valid when freeing the queues (+ both are freed within one function,
so it's not clear why nullify the pointers at all).

Fixes: 1c325aac10a8 ("idpf: configure resources for TX queues")
Fixes: 90912f9f4f2d ("idpf: convert header split mode to libeth + napi_build_skb()")
Reported-by: Michal Kubiak <michal.kubiak@intel.com>
Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Krishneil Singh <krishneil.k.singh@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
Link: https://patch.msgid.link/20240806220923.3359860-4-anthony.l.nguyen@intel.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 32b6f0d52e3c5a482d9d68f9550dc6b717af7e6e..3ac9d7ab83f20f280975c74da831e7615a7596be 100644 (file)
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -905,8 +905,8 @@ static void idpf_vport_stop(struct idpf_vport *vport)
 
        vport->link_up = false;
        idpf_vport_intr_deinit(vport);
-       idpf_vport_intr_rel(vport);
        idpf_vport_queues_rel(vport);
+       idpf_vport_intr_rel(vport);
        np->state = __IDPF_VPORT_DOWN;
 }
 
@@ -1351,43 +1351,43 @@ static int idpf_vport_open(struct idpf_vport *vport)
        /* we do not allow interface up just yet */
        netif_carrier_off(vport->netdev);
 
-       err = idpf_vport_queues_alloc(vport);
-       if (err)
-               return err;
-
        err = idpf_vport_intr_alloc(vport);
        if (err) {
                dev_err(&adapter->pdev->dev, "Failed to allocate interrupts for vport %u: %d\n",
                        vport->vport_id, err);
-               goto queues_rel;
+               return err;
        }
 
+       err = idpf_vport_queues_alloc(vport);
+       if (err)
+               goto intr_rel;
+
        err = idpf_vport_queue_ids_init(vport);
        if (err) {
                dev_err(&adapter->pdev->dev, "Failed to initialize queue ids for vport %u: %d\n",
                        vport->vport_id, err);
-               goto intr_rel;
+               goto queues_rel;
        }
 
        err = idpf_vport_intr_init(vport);
        if (err) {
                dev_err(&adapter->pdev->dev, "Failed to initialize interrupts for vport %u: %d\n",
                        vport->vport_id, err);
-               goto intr_rel;
+               goto queues_rel;
        }
 
        err = idpf_rx_bufs_init_all(vport);
        if (err) {
                dev_err(&adapter->pdev->dev, "Failed to initialize RX buffers for vport %u: %d\n",
                        vport->vport_id, err);
-               goto intr_rel;
+               goto queues_rel;
        }
 
        err = idpf_queue_reg_init(vport);
        if (err) {
                dev_err(&adapter->pdev->dev, "Failed to initialize queue registers for vport %u: %d\n",
                        vport->vport_id, err);
-               goto intr_rel;
+               goto queues_rel;
        }
 
        idpf_rx_init_buf_tail(vport);
@@ -1454,10 +1454,10 @@ unmap_queue_vectors:
        idpf_send_map_unmap_queue_vector_msg(vport, false);
 intr_deinit:
        idpf_vport_intr_deinit(vport);
-intr_rel:
-       idpf_vport_intr_rel(vport);
 queues_rel:
        idpf_vport_queues_rel(vport);
+intr_rel:
+       idpf_vport_intr_rel(vport);
 
        return err;
 }

diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index b023704bbbdab8018bfcbed2a0619da63b1b608c..0c22e524e56dbcfb29937f677a02aa3c7eb32df3 100644 (file)
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -3436,9 +3436,7 @@ static void idpf_vport_intr_napi_dis_all(struct idpf_vport *vport)
  */
 void idpf_vport_intr_rel(struct idpf_vport *vport)
 {
-       int i, j, v_idx;
-
-       for (v_idx = 0; v_idx < vport->num_q_vectors; v_idx++) {
+       for (u32 v_idx = 0; v_idx < vport->num_q_vectors; v_idx++) {
                struct idpf_q_vector *q_vector = &vport->q_vectors[v_idx];
 
                kfree(q_vector->bufq);
@@ -3449,26 +3447,6 @@ void idpf_vport_intr_rel(struct idpf_vport *vport)
                q_vector->rx = NULL;
        }
 
-       /* Clean up the mapping of queues to vectors */
-       for (i = 0; i < vport->num_rxq_grp; i++) {
-               struct idpf_rxq_group *rx_qgrp = &vport->rxq_grps[i];
-
-               if (idpf_is_queue_model_split(vport->rxq_model))
-                       for (j = 0; j < rx_qgrp->splitq.num_rxq_sets; j++)
-                               rx_qgrp->splitq.rxq_sets[j]->rxq.q_vector = NULL;
-               else
-                       for (j = 0; j < rx_qgrp->singleq.num_rxq; j++)
-                               rx_qgrp->singleq.rxqs[j]->q_vector = NULL;
-       }
-
-       if (idpf_is_queue_model_split(vport->txq_model))
-               for (i = 0; i < vport->num_txq_grp; i++)
-                       vport->txq_grps[i].complq->q_vector = NULL;
-       else
-               for (i = 0; i < vport->num_txq_grp; i++)
-                       for (j = 0; j < vport->txq_grps[i].num_txq; j++)
-                               vport->txq_grps[i].txqs[j]->q_vector = NULL;
-
        kfree(vport->q_vectors);
        vport->q_vectors = NULL;
 }