| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CY4PR11MB1576DF9D75817184274CAFC0AB1B9@CY4PR11MB1576.namprd11.prod.outlook.com>
Date: Tue, 6 Jul 2021 08:07:03 +0000
From: "Jankowski, Konrad0" <konrad0.jankowski@...el.com>
To: Stefan Assmann <sassmann@...nic.de>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "Laba, SlawomirX" <slawomirx.laba@...el.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [Intel-wired-lan] [PATCH] iavf: fix locking of critical sections
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of
> Stefan Assmann
> Sent: wtorek, 16 marca 2021 11:02
> To: intel-wired-lan@...ts.osuosl.org
> Cc: Laba, SlawomirX <slawomirx.laba@...el.com>; netdev@...r.kernel.org;
> sassmann@...nic.de
> Subject: [Intel-wired-lan] [PATCH] iavf: fix locking of critical sections
>
> To avoid races between iavf_init_task(), iavf_reset_task(),
> iavf_watchdog_task(), iavf_adminq_task() as well as the shutdown and
> remove functions more locking is required.
> The current protection by __IAVF_IN_CRITICAL_TASK is needed in additional
> places.
>
> - The reset task performs state transitions, therefore needs locking.
> - The adminq task acts on replies from the PF in
> iavf_virtchnl_completion() which may alter the states.
> - The init task is not only run during probe but also if a VF gets stuck
> to reinitialize it.
> - The shutdown function performs a state transition.
> - The remove function perorms a state transition and also free's
> resources.
>
> iavf_lock_timeout() is introduced to avoid waiting infinitely and cause a
> deadlock. Rather unlock and print a warning.
>
> Signed-off-by: Stefan Assmann <sassmann@...nic.de>
> ---
> drivers/net/ethernet/intel/iavf/iavf_main.c | 57 ++++++++++++++++++---
> 1 file changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index dc5b3c06d1e0..538b7aa43fa5 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -131,6 +131,30 @@ enum iavf_status iavf_free_virt_mem_d(struct
> iavf_hw *hw,
> return 0;
> }
>
> +/**
> + * iavf_timeout - try to set bit but give up after timeout
> + * @adapter: board private structure
> + * @bit: bit to set
> + * @msecs: timeout in msecs
> + *
> + * Returns 0 on success, negative on failure **/ static inline int
> +iavf_lock_timeout(struct iavf_adapter *adapter,
> + enum iavf_critical_section_t bit,
> + unsigned int msecs)
> +{
> + unsigned int wait, delay = 10;
> +
> + for (wait = 0; wait < msecs; wait += delay) {
> + if (!test_and_set_bit(bit, &adapter->crit_section))
> + return 0;
> +
> + msleep(delay);
> + }
> +
> + return -1;
> +}
> +
> /**
> * iavf_schedule_reset - Set the flags and schedule a reset event
> * @adapter: board private structure
> @@ -2069,6 +2093,10 @@ static void iavf_reset_task(struct work_struct
> *work)
> if (test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
> return;
>
> + if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 200)) {
> + schedule_work(&adapter->reset_task);
> + return;
> + }
> while (test_and_set_bit(__IAVF_IN_CLIENT_TASK,
> &adapter->crit_section))
> usleep_range(500, 1000);
> @@ -2275,6 +2303,8 @@ static void iavf_adminq_task(struct work_struct
> *work)
> if (!event.msg_buf)
> goto out;
>
> + if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 200))
> + goto freedom;
> do {
> ret = iavf_clean_arq_element(hw, &event, &pending);
> v_op = (enum
> virtchnl_ops)le32_to_cpu(event.desc.cookie_high);
> @@ -2288,6 +2318,7 @@ static void iavf_adminq_task(struct work_struct
> *work)
> if (pending != 0)
> memset(event.msg_buf, 0,
> IAVF_MAX_AQ_BUF_SIZE);
> } while (pending);
> + clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
>
> if ((adapter->flags &
> (IAVF_FLAG_RESET_PENDING | IAVF_FLAG_RESET_NEEDED)) ||
> @@ -3590,6 +3621,10 @@ static void iavf_init_task(struct work_struct
> *work)
> init_task.work);
> struct iavf_hw *hw = &adapter->hw;
>
> + if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 5000)) {
> + dev_warn(&adapter->pdev->dev, "failed to set
> __IAVF_IN_CRITICAL_TASK in %s\n", __FUNCTION__);
> + return;
> + }
> switch (adapter->state) {
> case __IAVF_STARTUP:
> if (iavf_startup(adapter) < 0)
> @@ -3602,14 +3637,14 @@ static void iavf_init_task(struct work_struct
> *work)
> case __IAVF_INIT_GET_RESOURCES:
> if (iavf_init_get_resources(adapter) < 0)
> goto init_failed;
> - return;
> + goto out;
> default:
> goto init_failed;
> }
>
> queue_delayed_work(iavf_wq, &adapter->init_task,
> msecs_to_jiffies(30));
> - return;
> + goto out;
> init_failed:
> if (++adapter->aq_wait_count > IAVF_AQ_MAX_ERR) {
> dev_err(&adapter->pdev->dev,
> @@ -3618,9 +3653,11 @@ static void iavf_init_task(struct work_struct
> *work)
> iavf_shutdown_adminq(hw);
> adapter->state = __IAVF_STARTUP;
> queue_delayed_work(iavf_wq, &adapter->init_task, HZ * 5);
> - return;
> + goto out;
> }
> queue_delayed_work(iavf_wq, &adapter->init_task, HZ);
> +out:
> + clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
> }
>
> /**
> @@ -3637,9 +3674,12 @@ static void iavf_shutdown(struct pci_dev *pdev)
> if (netif_running(netdev))
> iavf_close(netdev);
>
> + if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 5000))
> + dev_warn(&adapter->pdev->dev, "failed to set
> __IAVF_IN_CRITICAL_TASK
> +in %s\n", __FUNCTION__);
> /* Prevent the watchdog from running. */
> adapter->state = __IAVF_REMOVE;
> adapter->aq_required = 0;
> + clear_bit(__IAVF_IN_CRITICAL_TASK, &adapter->crit_section);
>
> #ifdef CONFIG_PM
> pci_save_state(pdev);
> @@ -3866,10 +3906,6 @@ static void iavf_remove(struct pci_dev *pdev)
> err);
> }
>
> - /* Shut down all the garbage mashers on the detention level */
> - adapter->state = __IAVF_REMOVE;
> - adapter->aq_required = 0;
> - adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
> iavf_request_reset(adapter);
> msleep(50);
> /* If the FW isn't responding, kick it once, but only once. */ @@ -
> 3877,6 +3913,13 @@ static void iavf_remove(struct pci_dev *pdev)
> iavf_request_reset(adapter);
> msleep(50);
> }
> + if (iavf_lock_timeout(adapter, __IAVF_IN_CRITICAL_TASK, 5000))
> + dev_warn(&adapter->pdev->dev, "failed to set
> __IAVF_IN_CRITICAL_TASK
> +in %s\n", __FUNCTION__);
> +
> + /* Shut down all the garbage mashers on the detention level */
> + adapter->state = __IAVF_REMOVE;
> + adapter->aq_required = 0;
> + adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
> iavf_free_all_tx_resources(adapter);
> iavf_free_all_rx_resources(adapter);
> iavf_misc_irq_disable(adapter);
> --
> 2.29.2
Tested-by: Konrad Jankowski <konrad0.jankowski@...el.com>
Powered by blists - more mailing lists