[net-next,4/8] pds_core: Prevent race issues involving the adminq
Commit Message
There are multiple paths that can result in using the pdsc's
adminq.
[1] pdsc_adminq_isr and the resulting work from queue_work(),
i.e. pdsc_work_thread()->pdsc_process_adminq()
[2] pdsc_adminq_post()
When the device goes through reset via PCIe reset and/or
a fw_down/fw_up cycle due to bad PCIe state or bad device
state the adminq is destroyed and recreated.
A NULL pointer dereference can happen if [1] or [2] happens
after the adminq is already destroyed.
In order to fix this, add some further state checks and
implement reference counting for adminq uses. Reference
counting was used because multiple threads can attempt to
access the adminq at the same time via [1] or [2]. Additionally,
multiple clients (i.e. pds-vfio-pci) can be using [2]
at the same time.
The adminq_refcnt is initialized to 1 when the adminq has been
allocated and is ready to use. Users/clients of the adminq
(i.e. [1] and [2]) will increment the refcnt when they are using
the adminq. When the driver goes into a fw_down cycle it will
set the PDSC_S_FW_DEAD bit and then wait for the adminq_refcnt
to hit 1. Setting the PDSC_S_FW_DEAD before waiting will prevent
any further adminq_refcnt increments. Waiting for the
adminq_refcnt to hit 1 allows for any current users of the adminq
to finish before the driver frees the adminq. Once the
adminq_refcnt hits 1 the driver clears the refcnt to signify that
the adminq is deleted and cannot be used. On the fw_up cycle the
driver will once again initialize the adminq_refcnt to 1 allowing
the adminq to be used again.
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>
---
drivers/net/ethernet/amd/pds_core/adminq.c | 31 +++++++++++++++++-----
drivers/net/ethernet/amd/pds_core/core.c | 21 +++++++++++++++
drivers/net/ethernet/amd/pds_core/core.h | 1 +
3 files changed, 47 insertions(+), 6 deletions(-)
Comments
On Thu, Jan 04, 2024 at 09:12:17AM -0800, Brett Creeley wrote:
> There are multiple paths that can result in using the pdsc's
> adminq.
>
> [1] pdsc_adminq_isr and the resulting work from queue_work(),
> i.e. pdsc_work_thread()->pdsc_process_adminq()
>
> [2] pdsc_adminq_post()
>
> When the device goes through reset via PCIe reset and/or
> a fw_down/fw_up cycle due to bad PCIe state or bad device
> state the adminq is destroyed and recreated.
>
> A NULL pointer dereference can happen if [1] or [2] happens
> after the adminq is already destroyed.
>
> In order to fix this, add some further state checks and
> implement reference counting for adminq uses. Reference
> counting was used because multiple threads can attempt to
> access the adminq at the same time via [1] or [2]. Additionally,
> multiple clients (i.e. pds-vfio-pci) can be using [2]
> at the same time.
>
> The adminq_refcnt is initialized to 1 when the adminq has been
> allocated and is ready to use. Users/clients of the adminq
> (i.e. [1] and [2]) will increment the refcnt when they are using
> the adminq. When the driver goes into a fw_down cycle it will
> set the PDSC_S_FW_DEAD bit and then wait for the adminq_refcnt
> to hit 1. Setting the PDSC_S_FW_DEAD before waiting will prevent
> any further adminq_refcnt increments. Waiting for the
> adminq_refcnt to hit 1 allows for any current users of the adminq
> to finish before the driver frees the adminq. Once the
> adminq_refcnt hits 1 the driver clears the refcnt to signify that
> the adminq is deleted and cannot be used. On the fw_up cycle the
> driver will once again initialize the adminq_refcnt to 1 allowing
> the adminq to be used again.
>
> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
> Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>
...
> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
> index 0356e56a6e99..3b3e1541dd1c 100644
> --- a/drivers/net/ethernet/amd/pds_core/core.c
> +++ b/drivers/net/ethernet/amd/pds_core/core.c
> @@ -450,6 +450,7 @@ int pdsc_setup(struct pdsc *pdsc, bool init)
> pdsc_debugfs_add_viftype(pdsc);
> }
>
> + refcount_set(&pdsc->adminq_refcnt, 1);
> clear_bit(PDSC_S_FW_DEAD, &pdsc->state);
> return 0;
>
> @@ -514,6 +515,24 @@ void pdsc_stop(struct pdsc *pdsc)
> PDS_CORE_INTR_MASK_SET);
> }
>
> +void pdsc_adminq_wait_and_dec_once_unused(struct pdsc *pdsc)
Hi Brett,
a minor nit from my side: pdsc_adminq_wait_and_dec_once_unused is only used
in this file so perhaps it should be static?
> +{
> + /* The driver initializes the adminq_refcnt to 1 when the adminq is
> + * allocated and ready for use. Other users/requesters will increment
> + * the refcnt while in use. If the refcnt is down to 1 then the adminq
> + * is not in use and the refcnt can be cleared and adminq freed. Before
> + * calling this function the driver will set PDSC_S_FW_DEAD, which
> + * prevent subsequent attempts to use the adminq and increment the
> + * refcnt to fail. This guarantees that this function will eventually
> + * exit.
> + */
> + while (!refcount_dec_if_one(&pdsc->adminq_refcnt)) {
> + dev_dbg_ratelimited(pdsc->dev, "%s: adminq in use\n",
> + __func__);
> + cpu_relax();
> + }
> +}
> +
> void pdsc_fw_down(struct pdsc *pdsc)
> {
> union pds_core_notifyq_comp reset_event = {
> @@ -529,6 +548,8 @@ void pdsc_fw_down(struct pdsc *pdsc)
> if (pdsc->pdev->is_virtfn)
> return;
>
> + pdsc_adminq_wait_and_dec_once_unused(pdsc);
> +
> /* Notify clients of fw_down */
> if (pdsc->fw_reporter)
> devlink_health_report(pdsc->fw_reporter, "FW down reported", pdsc);
...
On 1/4/2024 11:16 AM, Simon Horman wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Jan 04, 2024 at 09:12:17AM -0800, Brett Creeley wrote:
>> There are multiple paths that can result in using the pdsc's
>> adminq.
>>
>> [1] pdsc_adminq_isr and the resulting work from queue_work(),
>> i.e. pdsc_work_thread()->pdsc_process_adminq()
>>
>> [2] pdsc_adminq_post()
>>
>> When the device goes through reset via PCIe reset and/or
>> a fw_down/fw_up cycle due to bad PCIe state or bad device
>> state the adminq is destroyed and recreated.
>>
>> A NULL pointer dereference can happen if [1] or [2] happens
>> after the adminq is already destroyed.
>>
>> In order to fix this, add some further state checks and
>> implement reference counting for adminq uses. Reference
>> counting was used because multiple threads can attempt to
>> access the adminq at the same time via [1] or [2]. Additionally,
>> multiple clients (i.e. pds-vfio-pci) can be using [2]
>> at the same time.
>>
>> The adminq_refcnt is initialized to 1 when the adminq has been
>> allocated and is ready to use. Users/clients of the adminq
>> (i.e. [1] and [2]) will increment the refcnt when they are using
>> the adminq. When the driver goes into a fw_down cycle it will
>> set the PDSC_S_FW_DEAD bit and then wait for the adminq_refcnt
>> to hit 1. Setting the PDSC_S_FW_DEAD before waiting will prevent
>> any further adminq_refcnt increments. Waiting for the
>> adminq_refcnt to hit 1 allows for any current users of the adminq
>> to finish before the driver frees the adminq. Once the
>> adminq_refcnt hits 1 the driver clears the refcnt to signify that
>> the adminq is deleted and cannot be used. On the fw_up cycle the
>> driver will once again initialize the adminq_refcnt to 1 allowing
>> the adminq to be used again.
>>
>> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
>> Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>
>
> ...
>
>> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
>> index 0356e56a6e99..3b3e1541dd1c 100644
>> --- a/drivers/net/ethernet/amd/pds_core/core.c
>> +++ b/drivers/net/ethernet/amd/pds_core/core.c
>> @@ -450,6 +450,7 @@ int pdsc_setup(struct pdsc *pdsc, bool init)
>> pdsc_debugfs_add_viftype(pdsc);
>> }
>>
>> + refcount_set(&pdsc->adminq_refcnt, 1);
>> clear_bit(PDSC_S_FW_DEAD, &pdsc->state);
>> return 0;
>>
>> @@ -514,6 +515,24 @@ void pdsc_stop(struct pdsc *pdsc)
>> PDS_CORE_INTR_MASK_SET);
>> }
>>
>> +void pdsc_adminq_wait_and_dec_once_unused(struct pdsc *pdsc)
>
> Hi Brett,
>
> a minor nit from my side: pdsc_adminq_wait_and_dec_once_unused is only used
> in this file so perhaps it should be static?
Simon,
Yep, looks like I missed that. Good catch.
Thanks,
Brett
>
>> +{
>> + /* The driver initializes the adminq_refcnt to 1 when the adminq is
>> + * allocated and ready for use. Other users/requesters will increment
>> + * the refcnt while in use. If the refcnt is down to 1 then the adminq
>> + * is not in use and the refcnt can be cleared and adminq freed. Before
>> + * calling this function the driver will set PDSC_S_FW_DEAD, which
>> + * prevent subsequent attempts to use the adminq and increment the
>> + * refcnt to fail. This guarantees that this function will eventually
>> + * exit.
>> + */
>> + while (!refcount_dec_if_one(&pdsc->adminq_refcnt)) {
>> + dev_dbg_ratelimited(pdsc->dev, "%s: adminq in use\n",
>> + __func__);
>> + cpu_relax();
>> + }
>> +}
>> +
>> void pdsc_fw_down(struct pdsc *pdsc)
>> {
>> union pds_core_notifyq_comp reset_event = {
>> @@ -529,6 +548,8 @@ void pdsc_fw_down(struct pdsc *pdsc)
>> if (pdsc->pdev->is_virtfn)
>> return;
>>
>> + pdsc_adminq_wait_and_dec_once_unused(pdsc);
>> +
>> /* Notify clients of fw_down */
>> if (pdsc->fw_reporter)
>> devlink_health_report(pdsc->fw_reporter, "FW down reported", pdsc);
>
> ...
Hi Brett,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Brett-Creeley/pds_core-Prevent-health-thread-from-running-during-reset-remove/20240105-011706
base: net-next/main
patch link: https://lore.kernel.org/r/20240104171221.31399-5-brett.creeley%40amd.com
patch subject: [PATCH net-next 4/8] pds_core: Prevent race issues involving the adminq
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240106/202401060952.4J8S3LbP-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240106/202401060952.4J8S3LbP-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401060952.4J8S3LbP-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/net/ethernet/amd/pds_core/core.c:518:6: warning: no previous prototype for 'pdsc_adminq_wait_and_dec_once_unused' [-Wmissing-prototypes]
518 | void pdsc_adminq_wait_and_dec_once_unused(struct pdsc *pdsc)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/pdsc_adminq_wait_and_dec_once_unused +518 drivers/net/ethernet/amd/pds_core/core.c
517
> 518 void pdsc_adminq_wait_and_dec_once_unused(struct pdsc *pdsc)
519 {
520 /* The driver initializes the adminq_refcnt to 1 when the adminq is
521 * allocated and ready for use. Other users/requesters will increment
522 * the refcnt while in use. If the refcnt is down to 1 then the adminq
523 * is not in use and the refcnt can be cleared and adminq freed. Before
524 * calling this function the driver will set PDSC_S_FW_DEAD, which
525 * prevent subsequent attempts to use the adminq and increment the
526 * refcnt to fail. This guarantees that this function will eventually
527 * exit.
528 */
529 while (!refcount_dec_if_one(&pdsc->adminq_refcnt)) {
530 dev_dbg_ratelimited(pdsc->dev, "%s: adminq in use\n",
531 __func__);
532 cpu_relax();
533 }
534 }
535
Hi Brett,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Brett-Creeley/pds_core-Prevent-health-thread-from-running-during-reset-remove/20240105-011706
base: net-next/main
patch link: https://lore.kernel.org/r/20240104171221.31399-5-brett.creeley%40amd.com
patch subject: [PATCH net-next 4/8] pds_core: Prevent race issues involving the adminq
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20240107/202401070931.tvTUY2US-lkp@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240107/202401070931.tvTUY2US-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401070931.tvTUY2US-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/net/ethernet/amd/pds_core/core.c:518:6: warning: no previous prototype for function 'pdsc_adminq_wait_and_dec_once_unused' [-Wmissing-prototypes]
518 | void pdsc_adminq_wait_and_dec_once_unused(struct pdsc *pdsc)
| ^
drivers/net/ethernet/amd/pds_core/core.c:518:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
518 | void pdsc_adminq_wait_and_dec_once_unused(struct pdsc *pdsc)
| ^
| static
1 warning generated.
vim +/pdsc_adminq_wait_and_dec_once_unused +518 drivers/net/ethernet/amd/pds_core/core.c
517
> 518 void pdsc_adminq_wait_and_dec_once_unused(struct pdsc *pdsc)
519 {
520 /* The driver initializes the adminq_refcnt to 1 when the adminq is
521 * allocated and ready for use. Other users/requesters will increment
522 * the refcnt while in use. If the refcnt is down to 1 then the adminq
523 * is not in use and the refcnt can be cleared and adminq freed. Before
524 * calling this function the driver will set PDSC_S_FW_DEAD, which
525 * prevent subsequent attempts to use the adminq and increment the
526 * refcnt to fail. This guarantees that this function will eventually
527 * exit.
528 */
529 while (!refcount_dec_if_one(&pdsc->adminq_refcnt)) {
530 dev_dbg_ratelimited(pdsc->dev, "%s: adminq in use\n",
531 __func__);
532 cpu_relax();
533 }
534 }
535
@@ -63,6 +63,15 @@ static int pdsc_process_notifyq(struct pdsc_qcq *qcq)
return nq_work;
}
+static bool pdsc_adminq_inc_if_up(struct pdsc *pdsc)
+{
+ if (pdsc->state & BIT_ULL(PDSC_S_STOPPING_DRIVER) ||
+ pdsc->state & BIT_ULL(PDSC_S_FW_DEAD))
+ return false;
+
+ return refcount_inc_not_zero(&pdsc->adminq_refcnt);
+}
+
void pdsc_process_adminq(struct pdsc_qcq *qcq)
{
union pds_core_adminq_comp *comp;
@@ -75,9 +84,9 @@ void pdsc_process_adminq(struct pdsc_qcq *qcq)
int aq_work = 0;
int credits;
- /* Don't process AdminQ when shutting down */
- if (pdsc->state & BIT_ULL(PDSC_S_STOPPING_DRIVER)) {
- dev_err(pdsc->dev, "%s: called while PDSC_S_STOPPING_DRIVER\n",
+ /* Don't process AdminQ when it's not up */
+ if (!pdsc_adminq_inc_if_up(pdsc)) {
+ dev_err(pdsc->dev, "%s: called while adminq is unavailable\n",
__func__);
return;
}
@@ -124,6 +133,7 @@ void pdsc_process_adminq(struct pdsc_qcq *qcq)
pds_core_intr_credits(&pdsc->intr_ctrl[qcq->intx],
credits,
PDS_CORE_INTR_CRED_REARM);
+ refcount_dec(&pdsc->adminq_refcnt);
}
void pdsc_work_thread(struct work_struct *work)
@@ -138,9 +148,9 @@ irqreturn_t pdsc_adminq_isr(int irq, void *data)
struct pdsc *pdsc = data;
struct pdsc_qcq *qcq;
- /* Don't process AdminQ when shutting down */
- if (pdsc->state & BIT_ULL(PDSC_S_STOPPING_DRIVER)) {
- dev_err(pdsc->dev, "%s: called while PDSC_S_STOPPING_DRIVER\n",
+ /* Don't process AdminQ when it's not up */
+ if (!pdsc_adminq_inc_if_up(pdsc)) {
+ dev_err(pdsc->dev, "%s: called while adminq is unavailable\n",
__func__);
return IRQ_HANDLED;
}
@@ -148,6 +158,7 @@ irqreturn_t pdsc_adminq_isr(int irq, void *data)
qcq = &pdsc->adminqcq;
queue_work(pdsc->wq, &qcq->work);
pds_core_intr_mask(&pdsc->intr_ctrl[qcq->intx], PDS_CORE_INTR_MASK_CLEAR);
+ refcount_dec(&pdsc->adminq_refcnt);
return IRQ_HANDLED;
}
@@ -231,6 +242,12 @@ int pdsc_adminq_post(struct pdsc *pdsc,
int err = 0;
int index;
+ if (!pdsc_adminq_inc_if_up(pdsc)) {
+ dev_dbg(pdsc->dev, "%s: preventing adminq cmd %u\n",
+ __func__, cmd->opcode);
+ return -ENXIO;
+ }
+
wc.qcq = &pdsc->adminqcq;
index = __pdsc_adminq_post(pdsc, &pdsc->adminqcq, cmd, comp, &wc);
if (index < 0) {
@@ -286,6 +303,8 @@ int pdsc_adminq_post(struct pdsc *pdsc,
queue_work(pdsc->wq, &pdsc->health_work);
}
+ refcount_dec(&pdsc->adminq_refcnt);
+
return err;
}
EXPORT_SYMBOL_GPL(pdsc_adminq_post);
@@ -450,6 +450,7 @@ int pdsc_setup(struct pdsc *pdsc, bool init)
pdsc_debugfs_add_viftype(pdsc);
}
+ refcount_set(&pdsc->adminq_refcnt, 1);
clear_bit(PDSC_S_FW_DEAD, &pdsc->state);
return 0;
@@ -514,6 +515,24 @@ void pdsc_stop(struct pdsc *pdsc)
PDS_CORE_INTR_MASK_SET);
}
+void pdsc_adminq_wait_and_dec_once_unused(struct pdsc *pdsc)
+{
+ /* The driver initializes the adminq_refcnt to 1 when the adminq is
+ * allocated and ready for use. Other users/requesters will increment
+ * the refcnt while in use. If the refcnt is down to 1 then the adminq
+ * is not in use and the refcnt can be cleared and adminq freed. Before
+ * calling this function the driver will set PDSC_S_FW_DEAD, which
+ * prevent subsequent attempts to use the adminq and increment the
+ * refcnt to fail. This guarantees that this function will eventually
+ * exit.
+ */
+ while (!refcount_dec_if_one(&pdsc->adminq_refcnt)) {
+ dev_dbg_ratelimited(pdsc->dev, "%s: adminq in use\n",
+ __func__);
+ cpu_relax();
+ }
+}
+
void pdsc_fw_down(struct pdsc *pdsc)
{
union pds_core_notifyq_comp reset_event = {
@@ -529,6 +548,8 @@ void pdsc_fw_down(struct pdsc *pdsc)
if (pdsc->pdev->is_virtfn)
return;
+ pdsc_adminq_wait_and_dec_once_unused(pdsc);
+
/* Notify clients of fw_down */
if (pdsc->fw_reporter)
devlink_health_report(pdsc->fw_reporter, "FW down reported", pdsc);
@@ -184,6 +184,7 @@ struct pdsc {
struct mutex devcmd_lock; /* lock for dev_cmd operations */
struct mutex config_lock; /* lock for configuration operations */
spinlock_t adminq_lock; /* lock for adminq operations */
+ refcount_t adminq_refcnt;
struct pds_core_dev_info_regs __iomem *info_regs;
struct pds_core_dev_cmd_regs __iomem *cmd_regs;
struct pds_core_intr __iomem *intr_ctrl;