Re: [PATCH v2] sfc: potential dereference null pointer of rx_queue->page_ring
From: Martin Habets <hidden>
Date: 2021-12-19 09:27:21
Also in:
bpf, lkml
On Fri, Dec 17, 2021 at 04:33:58PM +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 efx_init_rx_recycle_ring(), efx_init_rx_queue(), efx_start_channels(), efx_start_datapath() and efx_start_all(), and return -ENOMEM when alloc fails and return 0 the others. Also, efx_net_open(), efx_pm_thaw(), efx_realloc_channels(), efx_reset_up() and efx_change_mtu() should deal with the return value of efx_start_all(). By the way, I have no idea where the code come from, which does not mention in the commit message. But I find the same bug in another two places in the ethernet and I will then commit the patches to repair them. The Fixes tag below is the one I guess. Fixes: 5a6681e22c14 ("sfc: separate out SFC4000 ("Falcon") support into new sfc-falcon driver") Signed-off-by: Jiasheng Jiang <redacted> --- Changelog: v1 -> v2 *Change 1. Casade return -ENOMEM when alloc fails and deal with the error. --- drivers/net/ethernet/sfc/ef100_netdev.c | 4 +++- drivers/net/ethernet/sfc/efx.c | 9 ++++++-- drivers/net/ethernet/sfc/efx_channels.c | 14 ++++++++++--- drivers/net/ethernet/sfc/efx_channels.h | 2 +- drivers/net/ethernet/sfc/efx_common.c | 28 ++++++++++++++++++------- drivers/net/ethernet/sfc/efx_common.h | 2 +- drivers/net/ethernet/sfc/rx_common.c | 14 ++++++++++--- drivers/net/ethernet/sfc/rx_common.h | 2 +- 8 files changed, 56 insertions(+), 19 deletions(-)diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c index 67fe44db6b61..aaba36e40446 100644 --- a/drivers/net/ethernet/sfc/ef100_netdev.c +++ b/drivers/net/ethernet/sfc/ef100_netdev.c@@ -162,7 +162,9 @@ static int ef100_net_open(struct net_device *net_dev) if (rc) goto fail; - efx_start_all(efx); + rc = efx_start_all(efx); + if (rc) + goto fail; /* Link state detection is normally event-driven; we have * to poll now because we could have missed a changediff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c index c746ca7235f1..874301cda686 100644 --- a/drivers/net/ethernet/sfc/efx.c +++ b/drivers/net/ethernet/sfc/efx.c@@ -540,7 +540,10 @@ int efx_net_open(struct net_device *net_dev) * before the monitor starts running */ efx_link_status_changed(efx); - efx_start_all(efx); + rc = efx_start_all(efx); + if (rc) + return rc; + if (efx->state == STATE_DISABLED || efx->reset_pending) netif_device_detach(efx->net_dev); efx_selftest_async_start(efx);@@ -1224,7 +1227,9 @@ static int efx_pm_thaw(struct device *dev) efx_mcdi_port_reconfigure(efx); mutex_unlock(&efx->mac_lock); - efx_start_all(efx); + rc = efx_start_all(efx); + if (rc) + goto fail; efx_device_attach_if_not_resetting(efx);diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c index bb48a139dd15..451f7c6a7680 100644 --- a/drivers/net/ethernet/sfc/efx_channels.c +++ b/drivers/net/ethernet/sfc/efx_channels.c@@ -837,7 +837,10 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries) "unable to restart interrupts on channel reallocation\n"); efx_schedule_reset(efx, RESET_TYPE_DISABLE); } else { - efx_start_all(efx); + rc = efx_start_all(efx); + if (rc) + goto rollback; + efx_device_attach_if_not_resetting(efx); } return rc;@@ -1055,11 +1058,12 @@ void efx_disable_interrupts(struct efx_nic *efx) efx->type->irq_disable_non_ev(efx); } -void efx_start_channels(struct efx_nic *efx) +int efx_start_channels(struct efx_nic *efx) { struct efx_tx_queue *tx_queue; struct efx_rx_queue *rx_queue; struct efx_channel *channel; + int ret; efx_for_each_channel(channel, efx) { efx_for_each_channel_tx_queue(tx_queue, channel) {@@ -1068,7 +1072,10 @@ void efx_start_channels(struct efx_nic *efx) } efx_for_each_channel_rx_queue(rx_queue, channel) { - efx_init_rx_queue(rx_queue); + ret = efx_init_rx_queue(rx_queue); + if (ret) + return ret; + atomic_inc(&efx->active_queues); efx_stop_eventq(channel); efx_fast_push_rx_descriptors(rx_queue, false);@@ -1077,6 +1084,7 @@ void efx_start_channels(struct efx_nic *efx) WARN_ON(channel->rx_pkt_n_frags); } + return 0; } void efx_stop_channels(struct efx_nic *efx)diff --git a/drivers/net/ethernet/sfc/efx_channels.h b/drivers/net/ethernet/sfc/efx_channels.h index d77ec1f77fb1..da38b6be5a45 100644 --- a/drivers/net/ethernet/sfc/efx_channels.h +++ b/drivers/net/ethernet/sfc/efx_channels.h@@ -42,7 +42,7 @@ void efx_remove_channel(struct efx_channel *channel); void efx_remove_channels(struct efx_nic *efx); void efx_fini_channels(struct efx_nic *efx); struct efx_channel *efx_copy_channel(const struct efx_channel *old_channel); -void efx_start_channels(struct efx_nic *efx); +int efx_start_channels(struct efx_nic *efx); void efx_stop_channels(struct efx_nic *efx); void efx_init_napi_channel(struct efx_channel *channel);diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c index de797e1ac5a9..0b5fec7769ea 100644 --- a/drivers/net/ethernet/sfc/efx_common.c +++ b/drivers/net/ethernet/sfc/efx_common.c@@ -309,7 +309,10 @@ int efx_change_mtu(struct net_device *net_dev, int new_mtu) efx_mac_reconfigure(efx, true); mutex_unlock(&efx->mac_lock); - efx_start_all(efx); + rc = efx_start_all(efx); + if (rc) + return rc; + efx_device_attach_if_not_resetting(efx); return 0; }@@ -361,11 +364,12 @@ void efx_start_monitor(struct efx_nic *efx) * to propagate configuration changes (mtu, checksum offload), or * to clear hardware error conditions */ -static void efx_start_datapath(struct efx_nic *efx) +static int efx_start_datapath(struct efx_nic *efx) { netdev_features_t old_features = efx->net_dev->features; bool old_rx_scatter = efx->rx_scatter; size_t rx_buf_len; + int ret; /* Calculate the rx buffer allocation parameters required to * support the current MTU, including padding for header@@ -431,12 +435,15 @@ static void efx_start_datapath(struct efx_nic *efx) efx->txq_wake_thresh = efx->txq_stop_thresh / 2; /* Initialise the channels */ - efx_start_channels(efx); + ret = efx_start_channels(efx); + if (ret) + return ret; efx_ptp_start_datapath(efx); if (netif_device_present(efx->net_dev)) netif_tx_wake_all_queues(efx->net_dev); + return 0; } static void efx_stop_datapath(struct efx_nic *efx)@@ -524,8 +531,9 @@ static void efx_stop_port(struct efx_nic *efx) * is safe to call multiple times, so long as the NIC is not disabled. * Requires the RTNL lock. */ -void efx_start_all(struct efx_nic *efx) +int efx_start_all(struct efx_nic *efx) { + int ret; EFX_ASSERT_RESET_SERIALISED(efx); BUG_ON(efx->state == STATE_DISABLED);@@ -534,10 +542,12 @@ void efx_start_all(struct efx_nic *efx) */ if (efx->port_enabled || !netif_running(efx->net_dev) || efx->reset_pending) - return; + return 0; efx_start_port(efx); - efx_start_datapath(efx); + ret = efx_start_datapath(efx); + if (ret) + return ret; /* Start the hardware monitor if there is one */ efx_start_monitor(efx);@@ -557,6 +567,8 @@ void efx_start_all(struct efx_nic *efx) 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@@ -786,7 +798,9 @@ int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok) mutex_unlock(&efx->mac_lock); - efx_start_all(efx); + rc = efx_start_all(efx); + if (rc) + goto fail; if (efx->type->udp_tnl_push_ports) efx->type->udp_tnl_push_ports(efx);diff --git a/drivers/net/ethernet/sfc/efx_common.h b/drivers/net/ethernet/sfc/efx_common.h index 65513fd0cf6c..adf84a64760b 100644 --- a/drivers/net/ethernet/sfc/efx_common.h +++ b/drivers/net/ethernet/sfc/efx_common.h@@ -28,7 +28,7 @@ void efx_fini_struct(struct efx_nic *efx); void efx_link_clear_advertising(struct efx_nic *efx); void efx_link_set_wanted_fc(struct efx_nic *efx, u8); -void efx_start_all(struct efx_nic *efx); +int efx_start_all(struct efx_nic *efx); void efx_stop_all(struct efx_nic *efx); void efx_net_stats(struct net_device *net_dev, struct rtnl_link_stats64 *stats);diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c index 68fc7d317693..b7d974e25a43 100644 --- a/drivers/net/ethernet/sfc/rx_common.c +++ b/drivers/net/ethernet/sfc/rx_common.c@@ -131,7 +131,7 @@ void efx_discard_rx_packet(struct efx_channel *channel, efx_free_rx_buffers(rx_queue, rx_buf, n_frags); } -static void efx_init_rx_recycle_ring(struct efx_rx_queue *rx_queue) +static int efx_init_rx_recycle_ring(struct efx_rx_queue *rx_queue) { unsigned int bufs_in_recycle_ring, page_ring_size; struct efx_nic *efx = rx_queue->efx;@@ -150,7 +150,11 @@ static void efx_init_rx_recycle_ring(struct efx_rx_queue *rx_queue) 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; } static void efx_fini_rx_recycle_ring(struct efx_rx_queue *rx_queue)@@ -222,7 +226,7 @@ int efx_probe_rx_queue(struct efx_rx_queue *rx_queue) return rc; } -void efx_init_rx_queue(struct efx_rx_queue *rx_queue) +int efx_init_rx_queue(struct efx_rx_queue *rx_queue) { unsigned int max_fill, trigger, max_trigger; struct efx_nic *efx = rx_queue->efx;@@ -236,7 +240,9 @@ void efx_init_rx_queue(struct efx_rx_queue *rx_queue) rx_queue->notified_count = 0; rx_queue->removed_count = 0; rx_queue->min_fill = -1U; - efx_init_rx_recycle_ring(rx_queue); + rc = efx_init_rx_recycle_ring(rx_queue); + if (rc) + return rc; rx_queue->page_remove = 0; rx_queue->page_add = rx_queue->page_ptr_mask + 1;@@ -275,6 +281,8 @@ void efx_init_rx_queue(struct efx_rx_queue *rx_queue) /* Set up RX descriptor ring */ efx_nic_init_rx(rx_queue); + + return 0; } void efx_fini_rx_queue(struct efx_rx_queue *rx_queue)diff --git a/drivers/net/ethernet/sfc/rx_common.h b/drivers/net/ethernet/sfc/rx_common.h index 207ccd8ba062..16133d3f46b2 100644 --- a/drivers/net/ethernet/sfc/rx_common.h +++ b/drivers/net/ethernet/sfc/rx_common.h@@ -47,7 +47,7 @@ void efx_discard_rx_packet(struct efx_channel *channel, unsigned int n_frags); int efx_probe_rx_queue(struct efx_rx_queue *rx_queue); -void efx_init_rx_queue(struct efx_rx_queue *rx_queue); +int efx_init_rx_queue(struct efx_rx_queue *rx_queue); void efx_fini_rx_queue(struct efx_rx_queue *rx_queue); void efx_remove_rx_queue(struct efx_rx_queue *rx_queue); void efx_destroy_rx_queue(struct efx_rx_queue *rx_queue);-- 2.25.1