The return value of kcalloc() needs to be checked.
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 <[email protected]>
---
drivers/net/ethernet/sfc/falcon/efx.c | 46 ++++++++++++++++++++-------
drivers/net/ethernet/sfc/falcon/efx.h | 2 +-
drivers/net/ethernet/sfc/falcon/rx.c | 18 ++++++++---
3 files changed, 50 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/sfc/falcon/efx.c b/drivers/net/ethernet/sfc/falcon/efx.c
index 5e7a57b680ca..fbd55029988e 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 ret;
/* 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);
+ ret = ef4_init_rx_queue(rx_queue);
+ if (ret)
+ return ret;
+
atomic_inc(&efx->active_queues);
ef4_stop_eventq(channel);
ef4_fast_push_rx_descriptors(rx_queue, false);
@@ -680,6 +684,7 @@ 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;
}
static void ef4_stop_datapath(struct ef4_nic *efx)
@@ -853,7 +858,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 +1822,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 ret;
+
EF4_ASSERT_RESET_SERIALISED(efx);
BUG_ON(efx->state == STATE_DISABLED);
@@ -1823,10 +1833,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);
+ ret = ef4_start_datapath(efx);
+ if (ret)
+ return ret;
/* Start the hardware monitor if there is one */
if (efx->type->monitor != NULL)
@@ -1838,6 +1850,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 +2088,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 +2157,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 +2429,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 +3055,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)
+ return rc;
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..cf9a4c387dd5 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 ret;
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);
+ ret = ef4_init_rx_recycle_ring(efx, rx_queue);
+ if (ret)
+ return ret;
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
On 17.12.2021 10:14, Jiasheng Jiang wrote:
> The return value of kcalloc() needs to be checked.
> 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 <[email protected]>
> ---
> drivers/net/ethernet/sfc/falcon/efx.c | 46 ++++++++++++++++++++-------
> drivers/net/ethernet/sfc/falcon/efx.h | 2 +-
> drivers/net/ethernet/sfc/falcon/rx.c | 18 ++++++++---
> 3 files changed, 50 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/sfc/falcon/efx.c b/drivers/net/ethernet/sfc/falcon/efx.c
> index 5e7a57b680ca..fbd55029988e 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 ret;
>
> /* 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);
> + ret = ef4_init_rx_queue(rx_queue);
> + if (ret)
I think you can't simply return here, you have to clean up properly.
Check ef4_stop_datapath() for cleanup activities.
> + return ret;
> +
> atomic_inc(&efx->active_queues);
> ef4_stop_eventq(channel);
> ef4_fast_push_rx_descriptors(rx_queue, false);
> @@ -680,6 +684,7 @@ 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;
> }
>
> static void ef4_stop_datapath(struct ef4_nic *efx)
> @@ -853,7 +858,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 +1822,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 ret;
> +
> EF4_ASSERT_RESET_SERIALISED(efx);
> BUG_ON(efx->state == STATE_DISABLED);
>
> @@ -1823,10 +1833,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);
> + ret = ef4_start_datapath(efx);
> + if (ret)
> + return ret;
>
> /* Start the hardware monitor if there is one */
> if (efx->type->monitor != NULL)
> @@ -1838,6 +1850,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 +2088,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 +2157,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 +2429,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 +3055,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)
> + return rc;
>
> 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..cf9a4c387dd5 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 ret;
>
> 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);
> + ret = ef4_init_rx_recycle_ring(efx, rx_queue);
> + if (ret)
> + return ret;
>
> 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)