| Message ID | 4b0a31bbc372842613286a10d7a8cbb0ee6069c7.1635400472.git.leonro@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [rdma-next] RDMA/core: Rely on vendors to set right IOVA | expand |
On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote: > From: Aharon Landau <aharonl@nvidia.com> > > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS > flag is provided. How is a "vendor" involved with this? This should all be upstream code.
On Thu, Oct 28, 2021 at 12:56:46AM -0700, Christoph Hellwig wrote: > On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote: > > From: Aharon Landau <aharonl@nvidia.com> > > > > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't > > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS > > flag is provided. > > How is a "vendor" involved with this? This should all be upstream code. "vendor" is wrong word here. I wanted to say that all drivers which support ".rereg_user_mr()" callback and return new_mr should set everything. In case of IB_MR_REREG_TRANS flow, it is IOVA which is not cmd.hca_va, but mr->iova. Thanks
On Thu, Oct 28, 2021 at 12:58:53PM +0300, Leon Romanovsky wrote: > "vendor" is wrong word here. > > I wanted to say that all drivers which support ".rereg_user_mr()" > callback and return new_mr should set everything. In case of IB_MR_REREG_TRANS > flow, it is IOVA which is not cmd.hca_va, but mr->iova. Thanks, that makes a lot more sense.
On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote: > From: Aharon Landau <aharonl@nvidia.com> > > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS > flag is provided. > > Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr") This isn't really a fixes type patch.. > Signed-off-by: Aharon Landau <aharonl@nvidia.com> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > drivers/infiniband/core/uverbs_cmd.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > index 740e6b2efe0e..d1345d76d9b1 100644 > +++ b/drivers/infiniband/core/uverbs_cmd.c > @@ -837,11 +837,8 @@ static int ib_uverbs_rereg_mr(struct uverbs_attr_bundle *attrs) > new_mr->device = new_pd->device; > new_mr->pd = new_pd; > new_mr->type = IB_MR_TYPE_USER; > - new_mr->dm = NULL; > - new_mr->sig_attrs = NULL; > new_mr->uobject = uobj; > atomic_inc(&new_pd->usecnt); > - new_mr->iova = cmd.hca_va; > new_uobj->object = new_mr; It is like this because the reg_mr path does it this way, if you want to change it here then change reg_mr as well, but that needs auditing all the drivers.. And I'd also suggest removing the other set a few lines below Jason
On Fri, Oct 29, 2021 at 01:27:02PM -0300, Jason Gunthorpe wrote: > On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote: > > From: Aharon Landau <aharonl@nvidia.com> > > > > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't > > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS > > flag is provided. > > > > Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr") > > This isn't really a fixes type patch.. Why? We see that without this patch MR IOVA is not as expected. > > > Signed-off-by: Aharon Landau <aharonl@nvidia.com> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > drivers/infiniband/core/uverbs_cmd.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > > index 740e6b2efe0e..d1345d76d9b1 100644 > > +++ b/drivers/infiniband/core/uverbs_cmd.c > > @@ -837,11 +837,8 @@ static int ib_uverbs_rereg_mr(struct uverbs_attr_bundle *attrs) > > new_mr->device = new_pd->device; > > new_mr->pd = new_pd; > > new_mr->type = IB_MR_TYPE_USER; > > - new_mr->dm = NULL; > > - new_mr->sig_attrs = NULL; > > new_mr->uobject = uobj; > > atomic_inc(&new_pd->usecnt); > > - new_mr->iova = cmd.hca_va; > > new_uobj->object = new_mr; > > It is like this because the reg_mr path does it this way, if you want > to change it here then change reg_mr as well, but that needs auditing > all the drivers.. > > And I'd also suggest removing the other set a few lines below I decided to be safe than sorry and removed only 100% correct attributes that does require minimal audit. Thanks > > Jason
On Fri, Oct 29, 2021 at 07:50:37PM +0300, Leon Romanovsky wrote: > On Fri, Oct 29, 2021 at 01:27:02PM -0300, Jason Gunthorpe wrote: > > On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote: > > > From: Aharon Landau <aharonl@nvidia.com> > > > > > > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't > > > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS > > > flag is provided. > > > > > > Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr") > > > > This isn't really a fixes type patch.. > > Why? We see that without this patch MR IOVA is not as expected. How so? Aharon should explain that in the commit message Jason
On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote: > From: Aharon Landau <aharonl@nvidia.com> > > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS > flag is provided. > > Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr") > Signed-off-by: Aharon Landau <aharonl@nvidia.com> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > --- > drivers/infiniband/core/uverbs_cmd.c | 3 --- > 1 file changed, 3 deletions(-) I rewrote the commit message: RDMA/core: Require the driver to set the IOVA correctly during rereg_mr If the driver returns a new MR during rereg it has to fill it with the IOVA from the proper source. If IB_MR_REREG_TRANS is set then the IOVA is cmd.hca_va, otherwise the IOVA comes from the old MR. mlx5 for example has two calls inside rereg_mr: return create_real_mr(new_pd, umem, mr->ibmr.iova, new_access_flags); and return create_real_mr(new_pd, new_umem, iova, new_access_flags); Unconditionally overwriting the iova in the newly allocated MR will corrupt the iova if the first path is used. Remove the redundant initializations from ib_uverbs_rereg_mr(). Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr") Link: https://lore.kernel.org/r/4b0a31bbc372842613286a10d7a8cbb0ee6069c7.1635400472.git.leonro@nvidia.com Signed-off-by: Aharon Landau <aharonl@nvidia.com> Signed-off-by: Leon Romanovsky <leonro@nvidia.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Wed, Nov 03, 2021 at 09:51:44AM -0300, Jason Gunthorpe wrote: > On Thu, Oct 28, 2021 at 08:55:22AM +0300, Leon Romanovsky wrote: > > From: Aharon Landau <aharonl@nvidia.com> > > > > The vendors set the IOVA of newly created MRs in rereg_user_mr, so don't > > overwrite it. That ensures that this field is set only if IB_MR_REREG_TRANS > > flag is provided. > > > > Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr") > > Signed-off-by: Aharon Landau <aharonl@nvidia.com> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > --- > > drivers/infiniband/core/uverbs_cmd.c | 3 --- > > 1 file changed, 3 deletions(-) > > I rewrote the commit message: Thanks a lot. > > RDMA/core: Require the driver to set the IOVA correctly during rereg_mr > > If the driver returns a new MR during rereg it has to fill it with the > IOVA from the proper source. If IB_MR_REREG_TRANS is set then the IOVA is > cmd.hca_va, otherwise the IOVA comes from the old MR. mlx5 for example has > two calls inside rereg_mr: > > return create_real_mr(new_pd, umem, mr->ibmr.iova, > new_access_flags); > and > return create_real_mr(new_pd, new_umem, iova, new_access_flags); > > Unconditionally overwriting the iova in the newly allocated MR will > corrupt the iova if the first path is used. > > Remove the redundant initializations from ib_uverbs_rereg_mr(). > > Fixes: 6e0954b11c05 ("RDMA/uverbs: Allow drivers to create a new HW object during rereg_mr") > Link: https://lore.kernel.org/r/4b0a31bbc372842613286a10d7a8cbb0ee6069c7.1635400472.git.leonro@nvidia.com > Signed-off-by: Aharon Landau <aharonl@nvidia.com> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > Jason
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index 740e6b2efe0e..d1345d76d9b1 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -837,11 +837,8 @@ static int ib_uverbs_rereg_mr(struct uverbs_attr_bundle *attrs) new_mr->device = new_pd->device; new_mr->pd = new_pd; new_mr->type = IB_MR_TYPE_USER; - new_mr->dm = NULL; - new_mr->sig_attrs = NULL; new_mr->uobject = uobj; atomic_inc(&new_pd->usecnt); - new_mr->iova = cmd.hca_va; new_uobj->object = new_mr; rdma_restrack_new(&new_mr->res, RDMA_RESTRACK_MR);