On 2023/3/17 18:17, Selvin Xavier wrote: > On Fri, Mar 17, 2023 at 2:40 PM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote: >> >> On 2023/3/15 16:16, Selvin Xavier wrote: >>> Add resize_cq verb support for user space CQs. Resize operation for >>> kernel CQs are not supported now. >>> >>> Driver should free the current CQ only after user library polls >>> for all the completions and switch to new CQ. So after the resize_cq >>> is returned from the driver, user libray polls for existing completions >> >> libray -> library >> >>> and store it as temporary data. Once library reaps all completions in the >>> current CQ, it invokes the ibv_cmd_poll_cq to inform the driver about >>> the resize_cq completion. Adding a check for user CQs in driver's >>> poll_cq and complete the resize operation for user CQs. >>> Updating uverbs_cmd_mask with poll_cq to support this. >>> >>> User library changes are available in this pull request. >>> https://github.com/linux-rdma/rdma-core/pull/1315 >>> >>> Signed-off-by: Selvin Xavier <selvin.xavier@xxxxxxxxxxxx> >>> --- >>> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 109 +++++++++++++++++++++++++++++++ >>> drivers/infiniband/hw/bnxt_re/ib_verbs.h | 3 + >>> drivers/infiniband/hw/bnxt_re/main.c | 2 + >>> drivers/infiniband/hw/bnxt_re/qplib_fp.c | 44 +++++++++++++ >>> drivers/infiniband/hw/bnxt_re/qplib_fp.h | 5 ++ >>> include/uapi/rdma/bnxt_re-abi.h | 4 ++ >>> 6 files changed, 167 insertions(+) >>> >>> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c >>> index 989edc7..e86afec 100644 >>> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c >>> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c >>> @@ -2912,6 +2912,106 @@ int bnxt_re_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr, >>> return rc; >>> } >>> >>> +static void bnxt_re_resize_cq_complete(struct bnxt_re_cq *cq) >>> +{ >>> + struct bnxt_re_dev *rdev = cq->rdev; >>> + >>> + bnxt_qplib_resize_cq_complete(&rdev->qplib_res, &cq->qplib_cq); >>> + >>> + cq->qplib_cq.max_wqe = cq->resize_cqe; >>> + if (cq->resize_umem) { >>> + ib_umem_release(cq->umem); >>> + cq->umem = cq->resize_umem; >>> + cq->resize_umem = NULL; >>> + cq->resize_cqe = 0; >>> + } >>> +} >>> + >>> +int bnxt_re_resize_cq(struct ib_cq *ibcq, int cqe, struct ib_udata *udata) >>> +{ >>> + struct bnxt_qplib_sg_info sg_info = {}; >>> + struct bnxt_qplib_dpi *orig_dpi = NULL; >>> + struct bnxt_qplib_dev_attr *dev_attr; >>> + struct bnxt_re_ucontext *uctx = NULL; >>> + struct bnxt_re_resize_cq_req req; >>> + struct bnxt_re_dev *rdev; >>> + struct bnxt_re_cq *cq; >>> + int rc, entries; >>> + >>> + cq = container_of(ibcq, struct bnxt_re_cq, ib_cq); >>> + rdev = cq->rdev; >>> + dev_attr = &rdev->dev_attr; >>> + if (!ibcq->uobject) { >>> + ibdev_err(&rdev->ibdev, "Kernel CQ Resize not supported"); >>> + return -EOPNOTSUPP; >>> + } >>> + >>> + if (cq->resize_umem) { >>> + ibdev_err(&rdev->ibdev, "Resize CQ %#x failed - Busy", >>> + cq->qplib_cq.id); >>> + return -EBUSY; >>> + } >> >> Does above cq->resize_umem checking has any conconcurrent protection >> again the bnxt_re_resize_cq_complete() called by bnxt_re_poll_cq()? >> >> bnxt_re_resize_cq() seems like a control path operation, while >> bnxt_re_poll_cq() seems like a data path operation, I am not sure >> there is any conconcurrent protection between them. >> > The previous check is to prevent simultaneous resize_cq context from > the user application. > > if you see the library implementation (PR > https://github.com/linux-rdma/rdma-core/pull/1315), entire operation > is done in the single resize_cq context from application > i.e. > bnxt_re_resize_cq > -> ibv_cmd_resize_cq > call driver bnxt_re_resize_cq and return > -> poll out the current completions and store it in a user lib list > -> issue an ibv_cmd_poll_cq. > This will invoke bnxt_re_poll_cq in the kernel driver. We free > the previous cq resources. > > So the synchronization between resize_cq and poll_cq is happening in > the user lib. We can free the old CQ only after user land sees the > final Completion on the previous CQ. So we return from the driver's > bnxt_re_resize_cq and then poll for all completions and then use > ibv_cmd_poll_cq as a hook to reach the kernel driver and free the old > CQ's resource. > > Summarize, driver's bnxt_re_poll_cq for user space CQs will be called > only from resize_cq context and no concurrent protection required in > the driver. Is it acceptable to depend on the user space code to ensure the correct order in the kernel space? Isn't that may utilized by some malicious user to crash the kernel?
