net: atlantic: eliminate double free in error handling logic

Driver has a logic leak in ring data allocation/free,
where aq_ring_free could be called multiple times on same ring,
if system is under stress and got memory allocation error.

Ring pointer was used as an indicator of failure, but this is
not correct since only ring data is allocated/deallocated.
Ring itself is an array member.

Changing ring allocation functions to return error code directly.
This simplifies error handling and eliminates aq_ring_free
on higher layer.

Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
Link: https://lore.kernel.org/r/20231213095044.23146-1-irusskikh@marvell.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
index 28c9b6f1a54f148d056c8106de3427c0fd479cce..abd4832e4ed21f3c2a22aed047a0331675162907 100644 (file)
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
@@ -953,8 +953,6 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
 {
        struct aq_ptp_s *aq_ptp = aq_nic->aq_ptp;
        unsigned int tx_ring_idx, rx_ring_idx;
-       struct aq_ring_s *hwts;
-       struct aq_ring_s *ring;
        int err;
 
        if (!aq_ptp)
@@ -962,29 +960,23 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
 
        tx_ring_idx = aq_ptp_ring_idx(aq_nic->aq_nic_cfg.tc_mode);
 
-       ring = aq_ring_tx_alloc(&aq_ptp->ptp_tx, aq_nic,
-                               tx_ring_idx, &aq_nic->aq_nic_cfg);
-       if (!ring) {
-               err = -ENOMEM;
+       err = aq_ring_tx_alloc(&aq_ptp->ptp_tx, aq_nic,
+                              tx_ring_idx, &aq_nic->aq_nic_cfg);
+       if (err)
                goto err_exit;
-       }
 
        rx_ring_idx = aq_ptp_ring_idx(aq_nic->aq_nic_cfg.tc_mode);
 
-       ring = aq_ring_rx_alloc(&aq_ptp->ptp_rx, aq_nic,
-                               rx_ring_idx, &aq_nic->aq_nic_cfg);
-       if (!ring) {
-               err = -ENOMEM;
+       err = aq_ring_rx_alloc(&aq_ptp->ptp_rx, aq_nic,
+                              rx_ring_idx, &aq_nic->aq_nic_cfg);
+       if (err)
                goto err_exit_ptp_tx;
-       }
 
-       hwts = aq_ring_hwts_rx_alloc(&aq_ptp->hwts_rx, aq_nic, PTP_HWST_RING_IDX,
-                                    aq_nic->aq_nic_cfg.rxds,
-                                    aq_nic->aq_nic_cfg.aq_hw_caps->rxd_size);
-       if (!hwts) {
-               err = -ENOMEM;
+       err = aq_ring_hwts_rx_alloc(&aq_ptp->hwts_rx, aq_nic, PTP_HWST_RING_IDX,
+                                   aq_nic->aq_nic_cfg.rxds,
+                                   aq_nic->aq_nic_cfg.aq_hw_caps->rxd_size);
+       if (err)
                goto err_exit_ptp_rx;
-       }
 
        err = aq_ptp_skb_ring_init(&aq_ptp->skb_ring, aq_nic->aq_nic_cfg.rxds);
        if (err != 0) {

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index e1885c1eb100a1fa67c0588832a991718728562c..cda8597b4e1469d2895f895f982f84cb97ef4506 100644 (file)
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -132,8 +132,8 @@ static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
        return 0;
 }
 
-static struct aq_ring_s *aq_ring_alloc(struct aq_ring_s *self,
-                                      struct aq_nic_s *aq_nic)
+static int aq_ring_alloc(struct aq_ring_s *self,
+                        struct aq_nic_s *aq_nic)
 {
        int err = 0;
 
@@ -156,46 +156,29 @@ static struct aq_ring_s *aq_ring_alloc(struct aq_ring_s *self,
 err_exit:
        if (err < 0) {
                aq_ring_free(self);
-               self = NULL;
        }
 
-       return self;
+       return err;
 }
 
-struct aq_ring_s *aq_ring_tx_alloc(struct aq_ring_s *self,
-                                  struct aq_nic_s *aq_nic,
-                                  unsigned int idx,
-                                  struct aq_nic_cfg_s *aq_nic_cfg)
+int aq_ring_tx_alloc(struct aq_ring_s *self,
+                    struct aq_nic_s *aq_nic,
+                    unsigned int idx,
+                    struct aq_nic_cfg_s *aq_nic_cfg)
 {
-       int err = 0;
-
        self->aq_nic = aq_nic;
        self->idx = idx;
        self->size = aq_nic_cfg->txds;
        self->dx_size = aq_nic_cfg->aq_hw_caps->txd_size;
 
-       self = aq_ring_alloc(self, aq_nic);
-       if (!self) {
-               err = -ENOMEM;
-               goto err_exit;
-       }
-
-err_exit:
-       if (err < 0) {
-               aq_ring_free(self);
-               self = NULL;
-       }
-
-       return self;
+       return aq_ring_alloc(self, aq_nic);
 }
 
-struct aq_ring_s *aq_ring_rx_alloc(struct aq_ring_s *self,
-                                  struct aq_nic_s *aq_nic,
-                                  unsigned int idx,
-                                  struct aq_nic_cfg_s *aq_nic_cfg)
+int aq_ring_rx_alloc(struct aq_ring_s *self,
+                    struct aq_nic_s *aq_nic,
+                    unsigned int idx,
+                    struct aq_nic_cfg_s *aq_nic_cfg)
 {
-       int err = 0;
-
        self->aq_nic = aq_nic;
        self->idx = idx;
        self->size = aq_nic_cfg->rxds;
@@ -217,22 +200,10 @@ struct aq_ring_s *aq_ring_rx_alloc(struct aq_ring_s *self,
                self->tail_size = 0;
        }
 
-       self = aq_ring_alloc(self, aq_nic);
-       if (!self) {
-               err = -ENOMEM;
-               goto err_exit;
-       }
-
-err_exit:
-       if (err < 0) {
-               aq_ring_free(self);
-               self = NULL;
-       }
-
-       return self;
+       return aq_ring_alloc(self, aq_nic);
 }
 
-struct aq_ring_s *
+int
 aq_ring_hwts_rx_alloc(struct aq_ring_s *self, struct aq_nic_s *aq_nic,
                      unsigned int idx, unsigned int size, unsigned int dx_size)
 {
@@ -250,10 +221,10 @@ aq_ring_hwts_rx_alloc(struct aq_ring_s *self, struct aq_nic_s *aq_nic,
                                           GFP_KERNEL);
        if (!self->dx_ring) {
                aq_ring_free(self);
-               return NULL;
+               return -ENOMEM;
        }
 
-       return self;
+       return 0;
 }
 
 int aq_ring_init(struct aq_ring_s *self, const enum atl_ring_type ring_type)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
index 0a6c34438c1d0e7241b9c8c3e5ee102f81899703..52847310740a21097dfc35a395e96dfe5de46321 100644 (file)
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
@@ -183,14 +183,14 @@ static inline unsigned int aq_ring_avail_dx(struct aq_ring_s *self)
                self->sw_head - self->sw_tail - 1);
 }
 
-struct aq_ring_s *aq_ring_tx_alloc(struct aq_ring_s *self,
-                                  struct aq_nic_s *aq_nic,
-                                  unsigned int idx,
-                                  struct aq_nic_cfg_s *aq_nic_cfg);
-struct aq_ring_s *aq_ring_rx_alloc(struct aq_ring_s *self,
-                                  struct aq_nic_s *aq_nic,
-                                  unsigned int idx,
-                                  struct aq_nic_cfg_s *aq_nic_cfg);
+int aq_ring_tx_alloc(struct aq_ring_s *self,
+                    struct aq_nic_s *aq_nic,
+                    unsigned int idx,
+                    struct aq_nic_cfg_s *aq_nic_cfg);
+int aq_ring_rx_alloc(struct aq_ring_s *self,
+                    struct aq_nic_s *aq_nic,
+                    unsigned int idx,
+                    struct aq_nic_cfg_s *aq_nic_cfg);
 
 int aq_ring_init(struct aq_ring_s *self, const enum atl_ring_type ring_type);
 void aq_ring_rx_deinit(struct aq_ring_s *self);
@@ -207,9 +207,9 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
                     int budget);
 int aq_ring_rx_fill(struct aq_ring_s *self);
 
-struct aq_ring_s *aq_ring_hwts_rx_alloc(struct aq_ring_s *self,
-               struct aq_nic_s *aq_nic, unsigned int idx,
-               unsigned int size, unsigned int dx_size);
+int aq_ring_hwts_rx_alloc(struct aq_ring_s *self,
+                         struct aq_nic_s *aq_nic, unsigned int idx,
+                         unsigned int size, unsigned int dx_size);
 void aq_ring_hwts_rx_clean(struct aq_ring_s *self, struct aq_nic_s *aq_nic);
 
 unsigned int aq_ring_fill_stats_data(struct aq_ring_s *self, u64 *data);

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
index f5db1c44e9b9171f0ab53d10f99b482a47d2fb0e..9769ab4f9bef01fe5e0015497ffb9c8a53655be3 100644 (file)
--- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
@@ -136,35 +136,32 @@ int aq_vec_ring_alloc(struct aq_vec_s *self, struct aq_nic_s *aq_nic,
                const unsigned int idx_ring = AQ_NIC_CFG_TCVEC2RING(aq_nic_cfg,
                                                                    i, idx);
 
-               ring = aq_ring_tx_alloc(&self->ring[i][AQ_VEC_TX_ID], aq_nic,
-                                       idx_ring, aq_nic_cfg);
-               if (!ring) {
-                       err = -ENOMEM;
+               ring = &self->ring[i][AQ_VEC_TX_ID];
+               err = aq_ring_tx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
+               if (err)
                        goto err_exit;
-               }
 
                ++self->tx_rings;
 
                aq_nic_set_tx_ring(aq_nic, idx_ring, ring);
 
-               if (xdp_rxq_info_reg(&self->ring[i][AQ_VEC_RX_ID].xdp_rxq,
+               ring = &self->ring[i][AQ_VEC_RX_ID];
+               if (xdp_rxq_info_reg(&ring->xdp_rxq,
                                     aq_nic->ndev, idx,
                                     self->napi.napi_id) < 0) {
                        err = -ENOMEM;
                        goto err_exit;
                }
-               if (xdp_rxq_info_reg_mem_model(&self->ring[i][AQ_VEC_RX_ID].xdp_rxq,
+               if (xdp_rxq_info_reg_mem_model(&ring->xdp_rxq,
                                               MEM_TYPE_PAGE_SHARED, NULL) < 0) {
-                       xdp_rxq_info_unreg(&self->ring[i][AQ_VEC_RX_ID].xdp_rxq);
+                       xdp_rxq_info_unreg(&ring->xdp_rxq);
                        err = -ENOMEM;
                        goto err_exit;
                }
 
-               ring = aq_ring_rx_alloc(&self->ring[i][AQ_VEC_RX_ID], aq_nic,
-                                       idx_ring, aq_nic_cfg);
-               if (!ring) {
-                       xdp_rxq_info_unreg(&self->ring[i][AQ_VEC_RX_ID].xdp_rxq);
-                       err = -ENOMEM;
+               err = aq_ring_rx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
+               if (err) {
+                       xdp_rxq_info_unreg(&ring->xdp_rxq);
                        goto err_exit;
                }