| Message ID | 1550837779-16793-1-git-send-email-devesh.sharma@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next,V2] bnxt_re: fix the regression due to changes in alloc_pbl | expand |
On Fri, Feb 22, 2019 at 07:16:19AM -0500, Devesh Sharma wrote: > While adding the use of for_each_sg_dma_page iterator for > Brodcom's rdma driver, there was a regression added in the > __alloc_pbl path. The change left bnxt_re in DOA state in > for-next branch. > > Fixing the regression to avoid the host crash when a user > space object is created. Restricting the unconditional > access to hwq.pg_arr when hwq is initialized for user space > objects. > > Fixes: 161ebe2498d4 ("RDMA/bnxt_re: Use for_each_sg_dma_page iterator on umem SGL") > Reported-by: Gal Pressman <galpress@amazon.com> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 11 +++++++---- > drivers/infiniband/hw/bnxt_re/qplib_fp.c | 20 ++++++-------------- > drivers/infiniband/hw/bnxt_re/qplib_res.c | 5 +---- > 3 files changed, 14 insertions(+), 22 deletions(-) > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > index 83bf6f5d..fc65751 100644 > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > @@ -793,8 +793,8 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp) > { > struct bnxt_re_qp *qp = container_of(ib_qp, struct bnxt_re_qp, ib_qp); > struct bnxt_re_dev *rdev = qp->rdev; > - int rc; > unsigned int flags; > + int rc; > > bnxt_qplib_flush_cqn_wq(&qp->qplib_qp); > rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp); > @@ -803,9 +803,12 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp) > return rc; > } > > - flags = bnxt_re_lock_cqs(qp); > - bnxt_qplib_clean_qp(&qp->qplib_qp); > - bnxt_re_unlock_cqs(qp, flags); > + if (!rdma_is_kernel_res(&qp->res)) { So this doesn't compile: drivers/infiniband/hw/bnxt_re/ib_verbs.c:806:31: error: ‘struct bnxt_re_qp’ has no member named ‘res’; did you mean ‘rdev’? I fixed it and applied it to for-next, but you should probably test things. Jason
On Fri, Feb 22, 2019 at 11:48 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Fri, Feb 22, 2019 at 07:16:19AM -0500, Devesh Sharma wrote: > > While adding the use of for_each_sg_dma_page iterator for > > Brodcom's rdma driver, there was a regression added in the > > __alloc_pbl path. The change left bnxt_re in DOA state in > > for-next branch. > > > > Fixing the regression to avoid the host crash when a user > > space object is created. Restricting the unconditional > > access to hwq.pg_arr when hwq is initialized for user space > > objects. > > > > Fixes: 161ebe2498d4 ("RDMA/bnxt_re: Use for_each_sg_dma_page iterator on umem SGL") > > Reported-by: Gal Pressman <galpress@amazon.com> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 11 +++++++---- > > drivers/infiniband/hw/bnxt_re/qplib_fp.c | 20 ++++++-------------- > > drivers/infiniband/hw/bnxt_re/qplib_res.c | 5 +---- > > 3 files changed, 14 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > index 83bf6f5d..fc65751 100644 > > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > @@ -793,8 +793,8 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp) > > { > > struct bnxt_re_qp *qp = container_of(ib_qp, struct bnxt_re_qp, ib_qp); > > struct bnxt_re_dev *rdev = qp->rdev; > > - int rc; > > unsigned int flags; > > + int rc; > > > > bnxt_qplib_flush_cqn_wq(&qp->qplib_qp); > > rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp); > > @@ -803,9 +803,12 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp) > > return rc; > > } > > > > - flags = bnxt_re_lock_cqs(qp); > > - bnxt_qplib_clean_qp(&qp->qplib_qp); > > - bnxt_re_unlock_cqs(qp, flags); > > + if (!rdma_is_kernel_res(&qp->res)) { > > So this doesn't compile: Oh Heck! I had fixed the build failure in my test environment and missed to pull back changes to master :-( > > drivers/infiniband/hw/bnxt_re/ib_verbs.c:806:31: error: ‘struct bnxt_re_qp’ has no member named ‘res’; did you mean ‘rdev’? > > I fixed it and applied it to for-next, but you should probably test In my test env rping passed. Silly miss! > things. > > Jason
Hi Devesh,
I love your patch! Yet something to improve:
[auto build test ERROR on rdma/for-next]
[also build test ERROR on next-20190222]
[cannot apply to v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Devesh-Sharma/bnxt_re-fix-the-regression-due-to-changes-in-alloc_pbl/20190223-033010
base: https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
drivers/infiniband/hw/bnxt_re/ib_verbs.c: In function 'bnxt_re_destroy_qp':
>> drivers/infiniband/hw/bnxt_re/ib_verbs.c:806:31: error: 'struct bnxt_re_qp' has no member named 'res'; did you mean 'rdev'?
if (!rdma_is_kernel_res(&qp->res)) {
^~~
rdev
vim +806 drivers/infiniband/hw/bnxt_re/ib_verbs.c
790
791 /* Queue Pairs */
792 int bnxt_re_destroy_qp(struct ib_qp *ib_qp)
793 {
794 struct bnxt_re_qp *qp = container_of(ib_qp, struct bnxt_re_qp, ib_qp);
795 struct bnxt_re_dev *rdev = qp->rdev;
796 unsigned int flags;
797 int rc;
798
799 bnxt_qplib_flush_cqn_wq(&qp->qplib_qp);
800 rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp);
801 if (rc) {
802 dev_err(rdev_to_dev(rdev), "Failed to destroy HW QP");
803 return rc;
804 }
805
> 806 if (!rdma_is_kernel_res(&qp->res)) {
807 flags = bnxt_re_lock_cqs(qp);
808 bnxt_qplib_clean_qp(&qp->qplib_qp);
809 bnxt_re_unlock_cqs(qp, flags);
810 }
811
812 bnxt_qplib_free_qp_res(&rdev->qplib_res, &qp->qplib_qp);
813
814 if (ib_qp->qp_type == IB_QPT_GSI && rdev->qp1_sqp) {
815 rc = bnxt_qplib_destroy_ah(&rdev->qplib_res,
816 &rdev->sqp_ah->qplib_ah, false);
817 if (rc) {
818 dev_err(rdev_to_dev(rdev),
819 "Failed to destroy HW AH for shadow QP");
820 return rc;
821 }
822
823 bnxt_qplib_clean_qp(&qp->qplib_qp);
824 rc = bnxt_qplib_destroy_qp(&rdev->qplib_res,
825 &rdev->qp1_sqp->qplib_qp);
826 if (rc) {
827 dev_err(rdev_to_dev(rdev),
828 "Failed to destroy Shadow QP");
829 return rc;
830 }
831 bnxt_qplib_free_qp_res(&rdev->qplib_res,
832 &rdev->qp1_sqp->qplib_qp);
833 mutex_lock(&rdev->qp_lock);
834 list_del(&rdev->qp1_sqp->list);
835 atomic_dec(&rdev->qp_count);
836 mutex_unlock(&rdev->qp_lock);
837
838 kfree(rdev->sqp_ah);
839 kfree(rdev->qp1_sqp);
840 rdev->qp1_sqp = NULL;
841 rdev->sqp_ah = NULL;
842 }
843
844 if (!IS_ERR_OR_NULL(qp->rumem))
845 ib_umem_release(qp->rumem);
846 if (!IS_ERR_OR_NULL(qp->sumem))
847 ib_umem_release(qp->sumem);
848
849 mutex_lock(&rdev->qp_lock);
850 list_del(&qp->list);
851 atomic_dec(&rdev->qp_count);
852 mutex_unlock(&rdev->qp_lock);
853 kfree(qp);
854 return 0;
855 }
856
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Devesh,
I love your patch! Yet something to improve:
[auto build test ERROR on rdma/for-next]
[also build test ERROR on next-20190222]
[cannot apply to v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Devesh-Sharma/bnxt_re-fix-the-regression-due-to-changes-in-alloc_pbl/20190223-033010
base: https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
All errors (new ones prefixed by >>):
>> drivers/infiniband/hw/bnxt_re/ib_verbs.c:806:36: sparse: error: no member 'res' in struct bnxt_re_qp
vim +/res +806 drivers/infiniband/hw/bnxt_re/ib_verbs.c
790
791 /* Queue Pairs */
792 int bnxt_re_destroy_qp(struct ib_qp *ib_qp)
793 {
794 struct bnxt_re_qp *qp = container_of(ib_qp, struct bnxt_re_qp, ib_qp);
795 struct bnxt_re_dev *rdev = qp->rdev;
796 unsigned int flags;
797 int rc;
798
799 bnxt_qplib_flush_cqn_wq(&qp->qplib_qp);
800 rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp);
801 if (rc) {
802 dev_err(rdev_to_dev(rdev), "Failed to destroy HW QP");
803 return rc;
804 }
805
> 806 if (!rdma_is_kernel_res(&qp->res)) {
807 flags = bnxt_re_lock_cqs(qp);
808 bnxt_qplib_clean_qp(&qp->qplib_qp);
809 bnxt_re_unlock_cqs(qp, flags);
810 }
811
812 bnxt_qplib_free_qp_res(&rdev->qplib_res, &qp->qplib_qp);
813
814 if (ib_qp->qp_type == IB_QPT_GSI && rdev->qp1_sqp) {
815 rc = bnxt_qplib_destroy_ah(&rdev->qplib_res,
816 &rdev->sqp_ah->qplib_ah, false);
817 if (rc) {
818 dev_err(rdev_to_dev(rdev),
819 "Failed to destroy HW AH for shadow QP");
820 return rc;
821 }
822
823 bnxt_qplib_clean_qp(&qp->qplib_qp);
824 rc = bnxt_qplib_destroy_qp(&rdev->qplib_res,
825 &rdev->qp1_sqp->qplib_qp);
826 if (rc) {
827 dev_err(rdev_to_dev(rdev),
828 "Failed to destroy Shadow QP");
829 return rc;
830 }
831 bnxt_qplib_free_qp_res(&rdev->qplib_res,
832 &rdev->qp1_sqp->qplib_qp);
833 mutex_lock(&rdev->qp_lock);
834 list_del(&rdev->qp1_sqp->list);
835 atomic_dec(&rdev->qp_count);
836 mutex_unlock(&rdev->qp_lock);
837
838 kfree(rdev->sqp_ah);
839 kfree(rdev->qp1_sqp);
840 rdev->qp1_sqp = NULL;
841 rdev->sqp_ah = NULL;
842 }
843
844 if (!IS_ERR_OR_NULL(qp->rumem))
845 ib_umem_release(qp->rumem);
846 if (!IS_ERR_OR_NULL(qp->sumem))
847 ib_umem_release(qp->sumem);
848
849 mutex_lock(&rdev->qp_lock);
850 list_del(&qp->list);
851 atomic_dec(&rdev->qp_count);
852 mutex_unlock(&rdev->qp_lock);
853 kfree(qp);
854 return 0;
855 }
856
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, Feb 23, 2019 at 3:15 AM kbuild test robot <lkp@intel.com> wrote: > > Hi Devesh, > > I love your patch! Yet something to improve: > > [auto build test ERROR on rdma/for-next] > [also build test ERROR on next-20190222] > [cannot apply to v5.0-rc4] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Devesh-Sharma/bnxt_re-fix-the-regression-due-to-changes-in-alloc_pbl/20190223-033010 > base: https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next > reproduce: > # apt-get install sparse > make ARCH=x86_64 allmodconfig > make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' > > All errors (new ones prefixed by >>): > > >> drivers/infiniband/hw/bnxt_re/ib_verbs.c:806:36: sparse: error: no member 'res' in struct bnxt_re_qp > > vim +/res +806 drivers/infiniband/hw/bnxt_re/ib_verbs.c > > 790 > 791 /* Queue Pairs */ > 792 int bnxt_re_destroy_qp(struct ib_qp *ib_qp) > 793 { > 794 struct bnxt_re_qp *qp = container_of(ib_qp, struct bnxt_re_qp, ib_qp); > 795 struct bnxt_re_dev *rdev = qp->rdev; > 796 unsigned int flags; > 797 int rc; > 798 > 799 bnxt_qplib_flush_cqn_wq(&qp->qplib_qp); > 800 rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp); > 801 if (rc) { > 802 dev_err(rdev_to_dev(rdev), "Failed to destroy HW QP"); > 803 return rc; > 804 } > 805 > > 806 if (!rdma_is_kernel_res(&qp->res)) { > 807 flags = bnxt_re_lock_cqs(qp); > 808 bnxt_qplib_clean_qp(&qp->qplib_qp); > 809 bnxt_re_unlock_cqs(qp, flags); > 810 } > 811 > 812 bnxt_qplib_free_qp_res(&rdev->qplib_res, &qp->qplib_qp); > 813 > 814 if (ib_qp->qp_type == IB_QPT_GSI && rdev->qp1_sqp) { > 815 rc = bnxt_qplib_destroy_ah(&rdev->qplib_res, > 816 &rdev->sqp_ah->qplib_ah, false); > 817 if (rc) { > 818 dev_err(rdev_to_dev(rdev), > 819 "Failed to destroy HW AH for shadow QP"); > 820 return rc; > 821 } > 822 > 823 bnxt_qplib_clean_qp(&qp->qplib_qp); > 824 rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, > 825 &rdev->qp1_sqp->qplib_qp); > 826 if (rc) { > 827 dev_err(rdev_to_dev(rdev), > 828 "Failed to destroy Shadow QP"); > 829 return rc; > 830 } > 831 bnxt_qplib_free_qp_res(&rdev->qplib_res, > 832 &rdev->qp1_sqp->qplib_qp); > 833 mutex_lock(&rdev->qp_lock); > 834 list_del(&rdev->qp1_sqp->list); > 835 atomic_dec(&rdev->qp_count); > 836 mutex_unlock(&rdev->qp_lock); > 837 > 838 kfree(rdev->sqp_ah); > 839 kfree(rdev->qp1_sqp); > 840 rdev->qp1_sqp = NULL; > 841 rdev->sqp_ah = NULL; > 842 } > 843 > 844 if (!IS_ERR_OR_NULL(qp->rumem)) > 845 ib_umem_release(qp->rumem); > 846 if (!IS_ERR_OR_NULL(qp->sumem)) > 847 ib_umem_release(qp->sumem); > 848 > 849 mutex_lock(&rdev->qp_lock); > 850 list_del(&qp->list); > 851 atomic_dec(&rdev->qp_count); > 852 mutex_unlock(&rdev->qp_lock); > 853 kfree(qp); > 854 return 0; > 855 } > 856 > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation Jason, Should I push out V3 to even out kbuild-robot? -Regards Devesh
On Sat, Feb 23, 2019 at 10:00:41AM +0530, Devesh Sharma wrote:
> Should I push out V3 to even out kbuild-robot?
No
Jason
diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c index 83bf6f5d..fc65751 100644 --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c @@ -793,8 +793,8 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp) { struct bnxt_re_qp *qp = container_of(ib_qp, struct bnxt_re_qp, ib_qp); struct bnxt_re_dev *rdev = qp->rdev; - int rc; unsigned int flags; + int rc; bnxt_qplib_flush_cqn_wq(&qp->qplib_qp); rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp); @@ -803,9 +803,12 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp) return rc; } - flags = bnxt_re_lock_cqs(qp); - bnxt_qplib_clean_qp(&qp->qplib_qp); - bnxt_re_unlock_cqs(qp, flags); + if (!rdma_is_kernel_res(&qp->res)) { + flags = bnxt_re_lock_cqs(qp); + bnxt_qplib_clean_qp(&qp->qplib_qp); + bnxt_re_unlock_cqs(qp, flags); + } + bnxt_qplib_free_qp_res(&rdev->qplib_res, &qp->qplib_qp); if (ib_qp->qp_type == IB_QPT_GSI && rdev->qp1_sqp) { diff --git a/drivers/infiniband/hw/bnxt_re/qplib_fp.c b/drivers/infiniband/hw/bnxt_re/qplib_fp.c index 77eb3d5..71c34d5 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_fp.c +++ b/drivers/infiniband/hw/bnxt_re/qplib_fp.c @@ -862,18 +862,18 @@ int bnxt_qplib_create_qp1(struct bnxt_qplib_res *res, struct bnxt_qplib_qp *qp) int bnxt_qplib_create_qp(struct bnxt_qplib_res *res, struct bnxt_qplib_qp *qp) { struct bnxt_qplib_rcfw *rcfw = res->rcfw; - struct sq_send *hw_sq_send_hdr, **hw_sq_send_ptr; - struct cmdq_create_qp req; - struct creq_create_qp_resp resp; - struct bnxt_qplib_pbl *pbl; - struct sq_psn_search **psn_search_ptr; unsigned long int psn_search, poff = 0; + struct sq_psn_search **psn_search_ptr; struct bnxt_qplib_q *sq = &qp->sq; struct bnxt_qplib_q *rq = &qp->rq; int i, rc, req_size, psn_sz = 0; + struct sq_send **hw_sq_send_ptr; + struct creq_create_qp_resp resp; struct bnxt_qplib_hwq *xrrq; u16 cmd_flags = 0, max_ssge; - u32 sw_prod, qp_flags = 0; + struct cmdq_create_qp req; + struct bnxt_qplib_pbl *pbl; + u32 qp_flags = 0; u16 max_rsge; RCFW_CMD_PREP(req, CREATE_QP, cmd_flags); @@ -948,14 +948,6 @@ int bnxt_qplib_create_qp(struct bnxt_qplib_res *res, struct bnxt_qplib_qp *qp) CMDQ_CREATE_QP_SQ_PG_SIZE_PG_1G : CMDQ_CREATE_QP_SQ_PG_SIZE_PG_4K); - /* initialize all SQ WQEs to LOCAL_INVALID (sq prep for hw fetch) */ - hw_sq_send_ptr = (struct sq_send **)sq->hwq.pbl_ptr; - for (sw_prod = 0; sw_prod < sq->hwq.max_elements; sw_prod++) { - hw_sq_send_hdr = &hw_sq_send_ptr[get_sqe_pg(sw_prod)] - [get_sqe_idx(sw_prod)]; - hw_sq_send_hdr->wqe_type = SQ_BASE_WQE_TYPE_LOCAL_INVALID; - } - if (qp->scq) req.scq_cid = cpu_to_le32(qp->scq->id); diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c b/drivers/infiniband/hw/bnxt_re/qplib_res.c index d08b9d9..0bc24f9 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_res.c +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c @@ -119,11 +119,8 @@ static int __alloc_pbl(struct pci_dev *pdev, struct bnxt_qplib_pbl *pbl, for_each_sg_dma_page (sghead, &sg_iter, pages, 0) { pbl->pg_map_arr[i] = sg_page_iter_dma_address(&sg_iter); pbl->pg_arr[i] = NULL; - if (!pbl->pg_arr[i]) - goto fail; - - i++; pbl->pg_count++; + i++; } }
While adding the use of for_each_sg_dma_page iterator for Brodcom's rdma driver, there was a regression added in the __alloc_pbl path. The change left bnxt_re in DOA state in for-next branch. Fixing the regression to avoid the host crash when a user space object is created. Restricting the unconditional access to hwq.pg_arr when hwq is initialized for user space objects. Fixes: 161ebe2498d4 ("RDMA/bnxt_re: Use for_each_sg_dma_page iterator on umem SGL") Reported-by: Gal Pressman <galpress@amazon.com> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> --- drivers/infiniband/hw/bnxt_re/ib_verbs.c | 11 +++++++---- drivers/infiniband/hw/bnxt_re/qplib_fp.c | 20 ++++++-------------- drivers/infiniband/hw/bnxt_re/qplib_res.c | 5 +---- 3 files changed, 14 insertions(+), 22 deletions(-)