Thread (2 messages) 2 messages, 2 authors, 2021-12-19

Re: [PATCH v2] sfc: falcon: potential dereference null pointer of rx_queue->page_ring

From: Martin Habets <hidden>
Date: 2021-12-19 09:29:56
Also in: lkml

On Fri, Dec 17, 2021 at 10:33:08PM +0800, Jiasheng Jiang wrote:
The return value of kcalloc() needs to be checked.
Your predicate is wrong. The code that uses rx_queue->page_ring
can deal with it being NULL.
The only thing you might want to do is set rx_queue->page_ptr_mask
to 0.

Martin
quoted hunk ↗ jump to hunk
To avoid dereference of null pointer in case of the failure of alloc.
Therefore, it might be better to change the return type of
ef4_init_rx_recycle_ring(), ef4_init_rx_queue(), ef4_start_datapath(),
ef4_start_all(), and return -ENOMEM when alloc fails and return 0 the
others.
Also, ef4_realloc_channels(), ef4_net_open(), ef4_change_mtu(),
ef4_reset_up() and ef4_pm_thaw() should deal with the return value of
ef4_start_all().

Fixes: 5a6681e22c14 ("sfc: separate out SFC4000 ("Falcon") support into new sfc-falcon driver")
Signed-off-by: Jiasheng Jiang <redacted>
---
Changelog:

v1 -> v2

*Change 1. Alter the "ret" to 'rc' and cleanup the rx_queue and tx_queue
when alloc fails.
---
 drivers/net/ethernet/sfc/falcon/efx.c | 56 +++++++++++++++++++++------
 drivers/net/ethernet/sfc/falcon/efx.h |  2 +-
 drivers/net/ethernet/sfc/falcon/rx.c  | 18 +++++++--
 3 files changed, 60 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/sfc/falcon/efx.c b/drivers/net/ethernet/sfc/falcon/efx.c
index 5e7a57b680ca..89b93ed08251 100644
--- a/drivers/net/ethernet/sfc/falcon/efx.c
+++ b/drivers/net/ethernet/sfc/falcon/efx.c
@@ -201,7 +201,7 @@ static void ef4_init_napi_channel(struct ef4_channel *channel);
 static void ef4_fini_napi(struct ef4_nic *efx);
 static void ef4_fini_napi_channel(struct ef4_channel *channel);
 static void ef4_fini_struct(struct ef4_nic *efx);
-static void ef4_start_all(struct ef4_nic *efx);
+static int ef4_start_all(struct ef4_nic *efx);
 static void ef4_stop_all(struct ef4_nic *efx);
 
 #define EF4_ASSERT_RESET_SERIALISED(efx)		\
@@ -590,7 +590,7 @@ static int ef4_probe_channels(struct ef4_nic *efx)
  * to propagate configuration changes (mtu, checksum offload), or
  * to clear hardware error conditions
  */
-static void ef4_start_datapath(struct ef4_nic *efx)
+static int ef4_start_datapath(struct ef4_nic *efx)
 {
 	netdev_features_t old_features = efx->net_dev->features;
 	bool old_rx_scatter = efx->rx_scatter;
@@ -598,6 +598,7 @@ static void ef4_start_datapath(struct ef4_nic *efx)
 	struct ef4_rx_queue *rx_queue;
 	struct ef4_channel *channel;
 	size_t rx_buf_len;
+	int rc;
 
 	/* Calculate the rx buffer allocation parameters required to
 	 * support the current MTU, including padding for header
@@ -668,7 +669,10 @@ static void ef4_start_datapath(struct ef4_nic *efx)
 		}
 
 		ef4_for_each_channel_rx_queue(rx_queue, channel) {
-			ef4_init_rx_queue(rx_queue);
+			rc = ef4_init_rx_queue(rx_queue);
+			if (rc)
+				goto fail;
+
 			atomic_inc(&efx->active_queues);
 			ef4_stop_eventq(channel);
 			ef4_fast_push_rx_descriptors(rx_queue, false);
@@ -680,6 +684,17 @@ static void ef4_start_datapath(struct ef4_nic *efx)
 
 	if (netif_device_present(efx->net_dev))
 		netif_tx_wake_all_queues(efx->net_dev);
+
+	return 0;
+
+fail:
+	ef4_for_each_channel(channel, efx) {
+		ef4_for_each_channel_rx_queue(rx_queue, channel)
+			ef4_fini_rx_queue(rx_queue);
+		ef4_for_each_possible_channel_tx_queue(tx_queue, channel)
+			ef4_fini_tx_queue(tx_queue);
+	}
+	return rc;
 }
 
 static void ef4_stop_datapath(struct ef4_nic *efx)
@@ -853,7 +868,10 @@ ef4_realloc_channels(struct ef4_nic *efx, u32 rxq_entries, u32 txq_entries)
 			  "unable to restart interrupts on channel reallocation\n");
 		ef4_schedule_reset(efx, RESET_TYPE_DISABLE);
 	} else {
-		ef4_start_all(efx);
+		rc = ef4_start_all(efx);
+		if (rc)
+			return rc;
+
 		netif_device_attach(efx->net_dev);
 	}
 	return rc;
@@ -1814,8 +1832,10 @@ static int ef4_probe_all(struct ef4_nic *efx)
  * is safe to call multiple times, so long as the NIC is not disabled.
  * Requires the RTNL lock.
  */
-static void ef4_start_all(struct ef4_nic *efx)
+static int ef4_start_all(struct ef4_nic *efx)
 {
+	int rc;
+
 	EF4_ASSERT_RESET_SERIALISED(efx);
 	BUG_ON(efx->state == STATE_DISABLED);
 
@@ -1823,10 +1843,12 @@ static void ef4_start_all(struct ef4_nic *efx)
 	 * of these flags are safe to read under just the rtnl lock */
 	if (efx->port_enabled || !netif_running(efx->net_dev) ||
 	    efx->reset_pending)
-		return;
+		return 0;
 
 	ef4_start_port(efx);
-	ef4_start_datapath(efx);
+	rc = ef4_start_datapath(efx);
+	if (rc)
+		return rc;
 
 	/* Start the hardware monitor if there is one */
 	if (efx->type->monitor != NULL)
@@ -1838,6 +1860,8 @@ static void ef4_start_all(struct ef4_nic *efx)
 	spin_lock_bh(&efx->stats_lock);
 	efx->type->update_stats(efx, NULL, NULL);
 	spin_unlock_bh(&efx->stats_lock);
+
+	return 0;
 }
 
 /* Quiesce the hardware and software data path, and regular activity
@@ -2074,7 +2098,10 @@ int ef4_net_open(struct net_device *net_dev)
 	 * before the monitor starts running */
 	ef4_link_status_changed(efx);
 
-	ef4_start_all(efx);
+	rc = ef4_start_all(efx);
+	if (rc)
+		return rc;
+
 	ef4_selftest_async_start(efx);
 	return 0;
 }
@@ -2140,7 +2167,10 @@ static int ef4_change_mtu(struct net_device *net_dev, int new_mtu)
 	ef4_mac_reconfigure(efx);
 	mutex_unlock(&efx->mac_lock);
 
-	ef4_start_all(efx);
+	rc = ef4_start_all(efx);
+	if (rc)
+		return rc;
+
 	netif_device_attach(efx->net_dev);
 	return 0;
 }
@@ -2409,7 +2439,9 @@ int ef4_reset_up(struct ef4_nic *efx, enum reset_type method, bool ok)
 
 	mutex_unlock(&efx->mac_lock);
 
-	ef4_start_all(efx);
+	rc = ef4_start_all(efx);
+	if (rc)
+		return rc;
 
 	return 0;
 
@@ -3033,7 +3065,9 @@ static int ef4_pm_thaw(struct device *dev)
 		efx->phy_op->reconfigure(efx);
 		mutex_unlock(&efx->mac_lock);
 
-		ef4_start_all(efx);
+		rc = ef4_start_all(efx);
+		if (rc)
+			goto fail;
 
 		netif_device_attach(efx->net_dev);
 
diff --git a/drivers/net/ethernet/sfc/falcon/efx.h b/drivers/net/ethernet/sfc/falcon/efx.h
index d3b4646545fa..483501b42667 100644
--- a/drivers/net/ethernet/sfc/falcon/efx.h
+++ b/drivers/net/ethernet/sfc/falcon/efx.h
@@ -39,7 +39,7 @@ void ef4_set_default_rx_indir_table(struct ef4_nic *efx);
 void ef4_rx_config_page_split(struct ef4_nic *efx);
 int ef4_probe_rx_queue(struct ef4_rx_queue *rx_queue);
 void ef4_remove_rx_queue(struct ef4_rx_queue *rx_queue);
-void ef4_init_rx_queue(struct ef4_rx_queue *rx_queue);
+int ef4_init_rx_queue(struct ef4_rx_queue *rx_queue);
 void ef4_fini_rx_queue(struct ef4_rx_queue *rx_queue);
 void ef4_fast_push_rx_descriptors(struct ef4_rx_queue *rx_queue, bool atomic);
 void ef4_rx_slow_fill(struct timer_list *t);
diff --git a/drivers/net/ethernet/sfc/falcon/rx.c b/drivers/net/ethernet/sfc/falcon/rx.c
index 966f13e7475d..6042219ddb2f 100644
--- a/drivers/net/ethernet/sfc/falcon/rx.c
+++ b/drivers/net/ethernet/sfc/falcon/rx.c
@@ -709,8 +709,8 @@ int ef4_probe_rx_queue(struct ef4_rx_queue *rx_queue)
 	return rc;
 }
 
-static void ef4_init_rx_recycle_ring(struct ef4_nic *efx,
-				     struct ef4_rx_queue *rx_queue)
+static int ef4_init_rx_recycle_ring(struct ef4_nic *efx,
+				    struct ef4_rx_queue *rx_queue)
 {
 	unsigned int bufs_in_recycle_ring, page_ring_size;
 
@@ -728,13 +728,19 @@ static void ef4_init_rx_recycle_ring(struct ef4_nic *efx,
 					    efx->rx_bufs_per_page);
 	rx_queue->page_ring = kcalloc(page_ring_size,
 				      sizeof(*rx_queue->page_ring), GFP_KERNEL);
+	if (!rx_queue->page_ring)
+		return -ENOMEM;
+
 	rx_queue->page_ptr_mask = page_ring_size - 1;
+
+	return 0;
 }
 
-void ef4_init_rx_queue(struct ef4_rx_queue *rx_queue)
+int ef4_init_rx_queue(struct ef4_rx_queue *rx_queue)
 {
 	struct ef4_nic *efx = rx_queue->efx;
 	unsigned int max_fill, trigger, max_trigger;
+	int rc;
 
 	netif_dbg(rx_queue->efx, drv, rx_queue->efx->net_dev,
 		  "initialising RX queue %d\n", ef4_rx_queue_index(rx_queue));
@@ -744,7 +750,9 @@ void ef4_init_rx_queue(struct ef4_rx_queue *rx_queue)
 	rx_queue->notified_count = 0;
 	rx_queue->removed_count = 0;
 	rx_queue->min_fill = -1U;
-	ef4_init_rx_recycle_ring(efx, rx_queue);
+	rc = ef4_init_rx_recycle_ring(efx, rx_queue);
+	if (rc)
+		return rc;
 
 	rx_queue->page_remove = 0;
 	rx_queue->page_add = rx_queue->page_ptr_mask + 1;
@@ -770,6 +778,8 @@ void ef4_init_rx_queue(struct ef4_rx_queue *rx_queue)
 
 	/* Set up RX descriptor ring */
 	ef4_nic_init_rx(rx_queue);
+
+	return 0;
 }
 
 void ef4_fini_rx_queue(struct ef4_rx_queue *rx_queue)
-- 
2.25.1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help