| 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: <20240208091538.GG56027@unreal>
Date: Thu, 8 Feb 2024 11:15:38 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Konstantin Taranov <kotaranov@...rosoft.com>
Cc: Konstantin Taranov <kotaranov@...ux.microsoft.com>,
"sharmaajay@...rosoft.com" <sharmaajay@...rosoft.com>,
Long Li <longli@...rosoft.com>, "jgg@...pe.ca" <jgg@...pe.ca>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH rdma-next v1 1/1] RDMA/mana_ib: Fix bug in creation of
dma regions
On Thu, Feb 08, 2024 at 08:49:43AM +0000, Konstantin Taranov wrote:
> > From: Leon Romanovsky <leon@...nel.org>
> > > From: Konstantin Taranov <kotaranov@...rosoft.com>
> > >
> > > Dma registration was ignoring virtual addresses by setting it to 0.
> > > As a result, mana_ib could only register page-aligned memory.
> > > As well as, it could fail to produce dma regions with zero offset for
> > > WQs and CQs (e.g., page size is 8192 but address is only 4096 bytes
> > > aligned), which is required by hardware.
> > >
> > > This patch takes into account the virtual address, allowing to create
> > > a dma region with any offset. For queues (e.g., WQs, CQs) that require
> > > dma regions with zero offset we add a flag to ensure zero offset.
> > >
> > > Signed-off-by: Konstantin Taranov <kotaranov@...rosoft.com>
> > > ---
> > > drivers/infiniband/hw/mana/cq.c | 3 ++-
> > > drivers/infiniband/hw/mana/main.c | 16 +++++++++++++---
> > > drivers/infiniband/hw/mana/mana_ib.h | 2 +-
> > > drivers/infiniband/hw/mana/mr.c | 2 +-
> > > drivers/infiniband/hw/mana/qp.c | 4 ++--
> > > drivers/infiniband/hw/mana/wq.c | 3 ++-
> > > 6 files changed, 21 insertions(+), 9 deletions(-)
> >
> > You definitely advised to look at the Documentation/process/submitting-
> > patches.rst guide.
> > 1. First revision doesn't need to be v1.
>
> Thanks. I did not know that.
>
> > 2. One logical fix/change == one patch.
>
> It is one fix. If I only replace 0 with virt, the code will stop working as the offset will not be
> zero quite often. That is why I need to make offset = 0 for queues.
>
> > 3. Fixes should have Fixes: tag in the commit message.
> As existing applications were made to go around this limitation, I wanted this patch arrive to rdma-next.
> Or do you say that I cannot opt for rdma-next and must make it a "fix"?
Once you write "fix" word in the patch, the expectation is to have Fixes line.
There is nothing wrong with applying patch with such tag to rdma-next
and we are doing it all the time. Our policy is fluid here and can be
summarized as follows:
1. Try to satisfy submitters request to put in specific target rdma-rc/rdma-next.
2. Very lax with taking patches to rdma-rc before -rc4.
3. In general, strict after -rc4, only patches with panics, build breakage and
UAPI visible bugs.
4. More pedantic review of -rc material.
So if you write rdma-next in title, add Fixes line which points to "old" code, we will apply
your patch to rdma-next.
>
> >
> > And I'm confident that the force_zero_offset change is not correct.
>
> It was tested with many page sizes and offsets. Could you elaborate why it is not correct?
I prefer that Jason will elaborate more on this, he will do it better
than me.
>
> Thanks!
>
> >
> > Thanks
> >
> > >
> > > diff --git a/drivers/infiniband/hw/mana/cq.c
> > > b/drivers/infiniband/hw/mana/cq.c index 83d20c3f0..e35de6b92 100644
> > > --- a/drivers/infiniband/hw/mana/cq.c
> > > +++ b/drivers/infiniband/hw/mana/cq.c
> > > @@ -48,7 +48,8 @@ int mana_ib_create_cq(struct ib_cq *ibcq, const struct
> > ib_cq_init_attr *attr,
> > > return err;
> > > }
> > >
> > > - err = mana_ib_gd_create_dma_region(mdev, cq->umem, &cq-
> > >gdma_region);
> > > + err = mana_ib_gd_create_dma_region(mdev, cq->umem, &cq-
> > >gdma_region,
> > > + ucmd.buf_addr, true);
> > > if (err) {
> > > ibdev_dbg(ibdev,
> > > "Failed to create dma region for create cq,
> > > %d\n", diff --git a/drivers/infiniband/hw/mana/main.c
> > > b/drivers/infiniband/hw/mana/main.c
> > > index 29dd2438d..13a4d5ab4 100644
> > > --- a/drivers/infiniband/hw/mana/main.c
> > > +++ b/drivers/infiniband/hw/mana/main.c
> > > @@ -302,7 +302,7 @@ mana_ib_gd_add_dma_region(struct mana_ib_dev
> > *dev,
> > > struct gdma_context *gc, }
> > >
> > > int mana_ib_gd_create_dma_region(struct mana_ib_dev *dev, struct
> > ib_umem *umem,
> > > - mana_handle_t *gdma_region)
> > > + mana_handle_t *gdma_region, u64 virt,
> > > + bool force_zero_offset)
> > > {
> > > struct gdma_dma_region_add_pages_req *add_req = NULL;
> > > size_t num_pages_processed = 0, num_pages_to_handle; @@ -324,11
> > > +324,21 @@ int mana_ib_gd_create_dma_region(struct mana_ib_dev *dev,
> > struct ib_umem *umem,
> > > hwc = gc->hwc.driver_data;
> > >
> > > /* Hardware requires dma region to align to chosen page size */
> > > - page_sz = ib_umem_find_best_pgsz(umem, PAGE_SZ_BM, 0);
> > > + page_sz = ib_umem_find_best_pgsz(umem, PAGE_SZ_BM, virt);
> > > if (!page_sz) {
> > > ibdev_dbg(&dev->ib_dev, "failed to find page size.\n");
> > > return -ENOMEM;
> > > }
> > > +
> > > + if (force_zero_offset) {
> > > + while (ib_umem_dma_offset(umem, page_sz) && page_sz >
> > PAGE_SIZE)
> > > + page_sz /= 2;
> > > + if (ib_umem_dma_offset(umem, page_sz) != 0) {
> > > + ibdev_dbg(&dev->ib_dev, "failed to find page size to force zero
> > offset.\n");
> > > + return -ENOMEM;
> > > + }
> > > + }
> > > +
> > > num_pages_total = ib_umem_num_dma_blocks(umem, page_sz);
> > >
> > > max_pgs_create_cmd =
> > > @@ -348,7 +358,7 @@ int mana_ib_gd_create_dma_region(struct
> > mana_ib_dev *dev, struct ib_umem *umem,
> > > sizeof(struct
> > > gdma_create_dma_region_resp));
> > >
> > > create_req->length = umem->length;
> > > - create_req->offset_in_page = umem->address & (page_sz - 1);
> > > + create_req->offset_in_page = ib_umem_dma_offset(umem, page_sz);
> > > create_req->gdma_page_type = order_base_2(page_sz) - PAGE_SHIFT;
> > > create_req->page_count = num_pages_total;
> > >
> > > diff --git a/drivers/infiniband/hw/mana/mana_ib.h
> > > b/drivers/infiniband/hw/mana/mana_ib.h
> > > index 6a03ae645..0a5a8f3f8 100644
> > > --- a/drivers/infiniband/hw/mana/mana_ib.h
> > > +++ b/drivers/infiniband/hw/mana/mana_ib.h
> > > @@ -161,7 +161,7 @@ static inline struct net_device
> > > *mana_ib_get_netdev(struct ib_device *ibdev, u32 int
> > > mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq
> > > *cq);
> > >
> > > int mana_ib_gd_create_dma_region(struct mana_ib_dev *dev, struct
> > ib_umem *umem,
> > > - mana_handle_t *gdma_region);
> > > + mana_handle_t *gdma_region, u64 virt,
> > > + bool force_zero_offset);
> > >
> > > int mana_ib_gd_destroy_dma_region(struct mana_ib_dev *dev,
> > > mana_handle_t gdma_region); diff --git
> > > a/drivers/infiniband/hw/mana/mr.c b/drivers/infiniband/hw/mana/mr.c
> > > index ee4d4f834..856d73ea2 100644
> > > --- a/drivers/infiniband/hw/mana/mr.c
> > > +++ b/drivers/infiniband/hw/mana/mr.c
> > > @@ -127,7 +127,7 @@ struct ib_mr *mana_ib_reg_user_mr(struct ib_pd
> > *ibpd, u64 start, u64 length,
> > > goto err_free;
> > > }
> > >
> > > - err = mana_ib_gd_create_dma_region(dev, mr->umem,
> > &dma_region_handle);
> > > + err = mana_ib_gd_create_dma_region(dev, mr->umem,
> > > + &dma_region_handle, iova, false);
> > > if (err) {
> > > ibdev_dbg(ibdev, "Failed create dma region for user-mr, %d\n",
> > > err);
> > > diff --git a/drivers/infiniband/hw/mana/qp.c
> > > b/drivers/infiniband/hw/mana/qp.c index 5d4c05dcd..02de90317 100644
> > > --- a/drivers/infiniband/hw/mana/qp.c
> > > +++ b/drivers/infiniband/hw/mana/qp.c
> > > @@ -357,8 +357,8 @@ static int mana_ib_create_qp_raw(struct ib_qp
> > *ibqp, struct ib_pd *ibpd,
> > > }
> > > qp->sq_umem = umem;
> > >
> > > - err = mana_ib_gd_create_dma_region(mdev, qp->sq_umem,
> > > - &qp->sq_gdma_region);
> > > + err = mana_ib_gd_create_dma_region(mdev, qp->sq_umem, &qp-
> > >sq_gdma_region,
> > > + ucmd.sq_buf_addr, true);
> > > if (err) {
> > > ibdev_dbg(&mdev->ib_dev,
> > > "Failed to create dma region for create
> > > qp-raw, %d\n", diff --git a/drivers/infiniband/hw/mana/wq.c
> > > b/drivers/infiniband/hw/mana/wq.c index 372d36151..d9c1a2d5d 100644
> > > --- a/drivers/infiniband/hw/mana/wq.c
> > > +++ b/drivers/infiniband/hw/mana/wq.c
> > > @@ -46,7 +46,8 @@ struct ib_wq *mana_ib_create_wq(struct ib_pd *pd,
> > > wq->wq_buf_size = ucmd.wq_buf_size;
> > > wq->rx_object = INVALID_MANA_HANDLE;
> > >
> > > - err = mana_ib_gd_create_dma_region(mdev, wq->umem, &wq-
> > >gdma_region);
> > > + err = mana_ib_gd_create_dma_region(mdev, wq->umem, &wq-
> > >gdma_region,
> > > + ucmd.wq_buf_addr, true);
> > > if (err) {
> > > ibdev_dbg(&mdev->ib_dev,
> > > "Failed to create dma region for create wq,
> > > %d\n",
> > >
> > > base-commit: aafe4cc5096996873817ff4981a3744e8caf7808
> > > --
> > > 2.43.0
> > >
Powered by blists - more mailing lists