| Message ID | 1726079379-19272-5-git-send-email-selvin.xavier@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/bnxt_re: Bug fixes for 6.12 kernel | expand |
在 2024/9/12 2:29, Selvin Xavier 写道: > There is a race between the CREQ tasklet and destroy > qp when accessing the qp-handle table. There is > a chance of reading a valid qp-handle in the > CREQ tasklet handler while the QP is already moving > ahead with the destruction. > > Fixing this race by implementing a table-lock to > synchronize the access. > > Fixes: f218d67ef004 ("RDMA/bnxt_re: Allow posting when QPs are in error") > Fixes: 84cf229f4001 ("RDMA/bnxt_re: Fix the qp table indexing") > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > --- > drivers/infiniband/hw/bnxt_re/qplib_fp.c | 5 +++++ > drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 12 ++++++++---- > drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 2 ++ > 3 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_fp.c b/drivers/infiniband/hw/bnxt_re/qplib_fp.c > index 42e98e5..5d36216 100644 > --- a/drivers/infiniband/hw/bnxt_re/qplib_fp.c > +++ b/drivers/infiniband/hw/bnxt_re/qplib_fp.c > @@ -1524,12 +1524,15 @@ int bnxt_qplib_destroy_qp(struct bnxt_qplib_res *res, > struct creq_destroy_qp_resp resp = {}; > struct bnxt_qplib_cmdqmsg msg = {}; > struct cmdq_destroy_qp req = {}; > + unsigned long flags; > u32 tbl_indx; > int rc; > > + spin_lock_irqsave(&rcfw->tbl_lock, flags); > tbl_indx = map_qp_id_to_tbl_indx(qp->id, rcfw); > rcfw->qp_tbl[tbl_indx].qp_id = BNXT_QPLIB_QP_ID_INVALID; > rcfw->qp_tbl[tbl_indx].qp_handle = NULL; > + spin_unlock_irqrestore(&rcfw->tbl_lock, flags); > > bnxt_qplib_rcfw_cmd_prep((struct cmdq_base *)&req, > CMDQ_BASE_OPCODE_DESTROY_QP, > @@ -1540,8 +1543,10 @@ int bnxt_qplib_destroy_qp(struct bnxt_qplib_res *res, > sizeof(resp), 0); > rc = bnxt_qplib_rcfw_send_message(rcfw, &msg); > if (rc) { > + spin_lock_irqsave(&rcfw->tbl_lock, flags); > rcfw->qp_tbl[tbl_indx].qp_id = qp->id; > rcfw->qp_tbl[tbl_indx].qp_handle = qp; > + spin_unlock_irqrestore(&rcfw->tbl_lock, flags); > return rc; > } > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c > index 3ffaef0c..993c356 100644 > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c > @@ -637,17 +637,21 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw, > case CREQ_QP_EVENT_EVENT_QP_ERROR_NOTIFICATION: > err_event = (struct creq_qp_error_notification *)qp_event; > qp_id = le32_to_cpu(err_event->xid); > + spin_lock(&rcfw->tbl_lock); > tbl_indx = map_qp_id_to_tbl_indx(qp_id, rcfw); > qp = rcfw->qp_tbl[tbl_indx].qp_handle; > + if (!qp) { > + spin_unlock(&rcfw->tbl_lock); > + break; > + } > + bnxt_qplib_mark_qp_error(qp); > + rc = rcfw->creq.aeq_handler(rcfw, qp_event, qp); > + spin_unlock(&rcfw->tbl_lock); > dev_dbg(&pdev->dev, "Received QP error notification\n"); > dev_dbg(&pdev->dev, > "qpid 0x%x, req_err=0x%x, resp_err=0x%x\n", > qp_id, err_event->req_err_state_reason, > err_event->res_err_state_reason); > - if (!qp) > - break; > - bnxt_qplib_mark_qp_error(qp); > - rc = rcfw->creq.aeq_handler(rcfw, qp_event, qp); > break; > default: > /* > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h > index 45996e6..07779ae 100644 > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h > @@ -224,6 +224,8 @@ struct bnxt_qplib_rcfw { > struct bnxt_qplib_crsqe *crsqe_tbl; > int qp_tbl_size; > struct bnxt_qplib_qp_node *qp_tbl; > + /* To synchronize the qp-handle hash table */ > + spinlock_t tbl_lock; missing spin_lock_init? In this commit, spin_lock_init is not found, maybe this function is called in other commit? Or now spin_lock_init is not used again? Zhu Yanjun > u64 oos_prev; > u32 init_oos_stats; > u32 cmdq_depth;
在 2024/9/12 2:29, Selvin Xavier 写道: > There is a race between the CREQ tasklet and destroy > qp when accessing the qp-handle table. There is > a chance of reading a valid qp-handle in the > CREQ tasklet handler while the QP is already moving > ahead with the destruction. > > Fixing this race by implementing a table-lock to > synchronize the access. > > Fixes: f218d67ef004 ("RDMA/bnxt_re: Allow posting when QPs are in error") > Fixes: 84cf229f4001 ("RDMA/bnxt_re: Fix the qp table indexing") > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > --- > drivers/infiniband/hw/bnxt_re/qplib_fp.c | 5 +++++ > drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 12 ++++++++---- > drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 2 ++ > 3 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_fp.c b/drivers/infiniband/hw/bnxt_re/qplib_fp.c > index 42e98e5..5d36216 100644 > --- a/drivers/infiniband/hw/bnxt_re/qplib_fp.c > +++ b/drivers/infiniband/hw/bnxt_re/qplib_fp.c > @@ -1524,12 +1524,15 @@ int bnxt_qplib_destroy_qp(struct bnxt_qplib_res *res, > struct creq_destroy_qp_resp resp = {}; > struct bnxt_qplib_cmdqmsg msg = {}; > struct cmdq_destroy_qp req = {}; > + unsigned long flags; > u32 tbl_indx; > int rc; > > + spin_lock_irqsave(&rcfw->tbl_lock, flags); Because the race occurs between tasklets, spin_lock_bh is enough to this? Zhu Yanjun > tbl_indx = map_qp_id_to_tbl_indx(qp->id, rcfw); > rcfw->qp_tbl[tbl_indx].qp_id = BNXT_QPLIB_QP_ID_INVALID; > rcfw->qp_tbl[tbl_indx].qp_handle = NULL; > + spin_unlock_irqrestore(&rcfw->tbl_lock, flags); > > bnxt_qplib_rcfw_cmd_prep((struct cmdq_base *)&req, > CMDQ_BASE_OPCODE_DESTROY_QP, > @@ -1540,8 +1543,10 @@ int bnxt_qplib_destroy_qp(struct bnxt_qplib_res *res, > sizeof(resp), 0); > rc = bnxt_qplib_rcfw_send_message(rcfw, &msg); > if (rc) { > + spin_lock_irqsave(&rcfw->tbl_lock, flags); > rcfw->qp_tbl[tbl_indx].qp_id = qp->id; > rcfw->qp_tbl[tbl_indx].qp_handle = qp; > + spin_unlock_irqrestore(&rcfw->tbl_lock, flags); > return rc; > } > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c > index 3ffaef0c..993c356 100644 > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c > @@ -637,17 +637,21 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw, > case CREQ_QP_EVENT_EVENT_QP_ERROR_NOTIFICATION: > err_event = (struct creq_qp_error_notification *)qp_event; > qp_id = le32_to_cpu(err_event->xid); > + spin_lock(&rcfw->tbl_lock); > tbl_indx = map_qp_id_to_tbl_indx(qp_id, rcfw); > qp = rcfw->qp_tbl[tbl_indx].qp_handle; > + if (!qp) { > + spin_unlock(&rcfw->tbl_lock); > + break; > + } > + bnxt_qplib_mark_qp_error(qp); > + rc = rcfw->creq.aeq_handler(rcfw, qp_event, qp); > + spin_unlock(&rcfw->tbl_lock); > dev_dbg(&pdev->dev, "Received QP error notification\n"); > dev_dbg(&pdev->dev, > "qpid 0x%x, req_err=0x%x, resp_err=0x%x\n", > qp_id, err_event->req_err_state_reason, > err_event->res_err_state_reason); > - if (!qp) > - break; > - bnxt_qplib_mark_qp_error(qp); > - rc = rcfw->creq.aeq_handler(rcfw, qp_event, qp); > break; > default: > /* > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h > index 45996e6..07779ae 100644 > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h > @@ -224,6 +224,8 @@ struct bnxt_qplib_rcfw { > struct bnxt_qplib_crsqe *crsqe_tbl; > int qp_tbl_size; > struct bnxt_qplib_qp_node *qp_tbl; > + /* To synchronize the qp-handle hash table */ > + spinlock_t tbl_lock; > u64 oos_prev; > u32 init_oos_stats; > u32 cmdq_depth;
On Fri, Sep 13, 2024 at 10:00 AM Zhu Yanjun <yanjun.zhu@linux.dev> wrote: > > 在 2024/9/12 2:29, Selvin Xavier 写道: > > There is a race between the CREQ tasklet and destroy > > qp when accessing the qp-handle table. There is > > a chance of reading a valid qp-handle in the > > CREQ tasklet handler while the QP is already moving > > ahead with the destruction. > > > > Fixing this race by implementing a table-lock to > > synchronize the access. > > > > Fixes: f218d67ef004 ("RDMA/bnxt_re: Allow posting when QPs are in error") > > Fixes: 84cf229f4001 ("RDMA/bnxt_re: Fix the qp table indexing") > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > --- > > drivers/infiniband/hw/bnxt_re/qplib_fp.c | 5 +++++ > > drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 12 ++++++++---- > > drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 2 ++ > > 3 files changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_fp.c b/drivers/infiniband/hw/bnxt_re/qplib_fp.c > > index 42e98e5..5d36216 100644 > > --- a/drivers/infiniband/hw/bnxt_re/qplib_fp.c > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_fp.c > > @@ -1524,12 +1524,15 @@ int bnxt_qplib_destroy_qp(struct bnxt_qplib_res *res, > > struct creq_destroy_qp_resp resp = {}; > > struct bnxt_qplib_cmdqmsg msg = {}; > > struct cmdq_destroy_qp req = {}; > > + unsigned long flags; > > u32 tbl_indx; > > int rc; > > > > + spin_lock_irqsave(&rcfw->tbl_lock, flags); > > Because the race occurs between tasklets, spin_lock_bh is enough to this? You are right. The race occurs in the BH handler. bnxt_qplib_service_creq , which is scheduled in the tasklet context, is already using the spin_lock_irqsave for synchronizing this function with other contexts. The current code change is with-in this context. I added this change on top of the current code and used irqsave. But I will fix both the spin lock usage and post a v2. Thanks for the review. > > Zhu Yanjun > > > tbl_indx = map_qp_id_to_tbl_indx(qp->id, rcfw); > > rcfw->qp_tbl[tbl_indx].qp_id = BNXT_QPLIB_QP_ID_INVALID; > > rcfw->qp_tbl[tbl_indx].qp_handle = NULL; > > + spin_unlock_irqrestore(&rcfw->tbl_lock, flags); > > > > bnxt_qplib_rcfw_cmd_prep((struct cmdq_base *)&req, > > CMDQ_BASE_OPCODE_DESTROY_QP, > > @@ -1540,8 +1543,10 @@ int bnxt_qplib_destroy_qp(struct bnxt_qplib_res *res, > > sizeof(resp), 0); > > rc = bnxt_qplib_rcfw_send_message(rcfw, &msg); > > if (rc) { > > + spin_lock_irqsave(&rcfw->tbl_lock, flags); > > rcfw->qp_tbl[tbl_indx].qp_id = qp->id; > > rcfw->qp_tbl[tbl_indx].qp_handle = qp; > > + spin_unlock_irqrestore(&rcfw->tbl_lock, flags); > > return rc; > > } > > > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c > > index 3ffaef0c..993c356 100644 > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c > > @@ -637,17 +637,21 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw, > > case CREQ_QP_EVENT_EVENT_QP_ERROR_NOTIFICATION: > > err_event = (struct creq_qp_error_notification *)qp_event; > > qp_id = le32_to_cpu(err_event->xid); > > + spin_lock(&rcfw->tbl_lock); > > tbl_indx = map_qp_id_to_tbl_indx(qp_id, rcfw); > > qp = rcfw->qp_tbl[tbl_indx].qp_handle; > > + if (!qp) { > > + spin_unlock(&rcfw->tbl_lock); > > + break; > > + } > > + bnxt_qplib_mark_qp_error(qp); > > + rc = rcfw->creq.aeq_handler(rcfw, qp_event, qp); > > + spin_unlock(&rcfw->tbl_lock); > > dev_dbg(&pdev->dev, "Received QP error notification\n"); > > dev_dbg(&pdev->dev, > > "qpid 0x%x, req_err=0x%x, resp_err=0x%x\n", > > qp_id, err_event->req_err_state_reason, > > err_event->res_err_state_reason); > > - if (!qp) > > - break; > > - bnxt_qplib_mark_qp_error(qp); > > - rc = rcfw->creq.aeq_handler(rcfw, qp_event, qp); > > break; > > default: > > /* > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h > > index 45996e6..07779ae 100644 > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h > > @@ -224,6 +224,8 @@ struct bnxt_qplib_rcfw { > > struct bnxt_qplib_crsqe *crsqe_tbl; > > int qp_tbl_size; > > struct bnxt_qplib_qp_node *qp_tbl; > > + /* To synchronize the qp-handle hash table */ > > + spinlock_t tbl_lock; > > u64 oos_prev; > > u32 init_oos_stats; > > u32 cmdq_depth; >
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_fp.c b/drivers/infiniband/hw/bnxt_re/qplib_fp.c index 42e98e5..5d36216 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_fp.c +++ b/drivers/infiniband/hw/bnxt_re/qplib_fp.c @@ -1524,12 +1524,15 @@ int bnxt_qplib_destroy_qp(struct bnxt_qplib_res *res, struct creq_destroy_qp_resp resp = {}; struct bnxt_qplib_cmdqmsg msg = {}; struct cmdq_destroy_qp req = {}; + unsigned long flags; u32 tbl_indx; int rc; + spin_lock_irqsave(&rcfw->tbl_lock, flags); tbl_indx = map_qp_id_to_tbl_indx(qp->id, rcfw); rcfw->qp_tbl[tbl_indx].qp_id = BNXT_QPLIB_QP_ID_INVALID; rcfw->qp_tbl[tbl_indx].qp_handle = NULL; + spin_unlock_irqrestore(&rcfw->tbl_lock, flags); bnxt_qplib_rcfw_cmd_prep((struct cmdq_base *)&req, CMDQ_BASE_OPCODE_DESTROY_QP, @@ -1540,8 +1543,10 @@ int bnxt_qplib_destroy_qp(struct bnxt_qplib_res *res, sizeof(resp), 0); rc = bnxt_qplib_rcfw_send_message(rcfw, &msg); if (rc) { + spin_lock_irqsave(&rcfw->tbl_lock, flags); rcfw->qp_tbl[tbl_indx].qp_id = qp->id; rcfw->qp_tbl[tbl_indx].qp_handle = qp; + spin_unlock_irqrestore(&rcfw->tbl_lock, flags); return rc; } diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c index 3ffaef0c..993c356 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c @@ -637,17 +637,21 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw, case CREQ_QP_EVENT_EVENT_QP_ERROR_NOTIFICATION: err_event = (struct creq_qp_error_notification *)qp_event; qp_id = le32_to_cpu(err_event->xid); + spin_lock(&rcfw->tbl_lock); tbl_indx = map_qp_id_to_tbl_indx(qp_id, rcfw); qp = rcfw->qp_tbl[tbl_indx].qp_handle; + if (!qp) { + spin_unlock(&rcfw->tbl_lock); + break; + } + bnxt_qplib_mark_qp_error(qp); + rc = rcfw->creq.aeq_handler(rcfw, qp_event, qp); + spin_unlock(&rcfw->tbl_lock); dev_dbg(&pdev->dev, "Received QP error notification\n"); dev_dbg(&pdev->dev, "qpid 0x%x, req_err=0x%x, resp_err=0x%x\n", qp_id, err_event->req_err_state_reason, err_event->res_err_state_reason); - if (!qp) - break; - bnxt_qplib_mark_qp_error(qp); - rc = rcfw->creq.aeq_handler(rcfw, qp_event, qp); break; default: /* diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h index 45996e6..07779ae 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h @@ -224,6 +224,8 @@ struct bnxt_qplib_rcfw { struct bnxt_qplib_crsqe *crsqe_tbl; int qp_tbl_size; struct bnxt_qplib_qp_node *qp_tbl; + /* To synchronize the qp-handle hash table */ + spinlock_t tbl_lock; u64 oos_prev; u32 init_oos_stats; u32 cmdq_depth;
There is a race between the CREQ tasklet and destroy qp when accessing the qp-handle table. There is a chance of reading a valid qp-handle in the CREQ tasklet handler while the QP is already moving ahead with the destruction. Fixing this race by implementing a table-lock to synchronize the access. Fixes: f218d67ef004 ("RDMA/bnxt_re: Allow posting when QPs are in error") Fixes: 84cf229f4001 ("RDMA/bnxt_re: Fix the qp table indexing") Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> --- drivers/infiniband/hw/bnxt_re/qplib_fp.c | 5 +++++ drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 12 ++++++++---- drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 2 ++ 3 files changed, 15 insertions(+), 4 deletions(-)