linux-kernel - Re: [PATCH rdma-next v1 1/1] RDMA/mana_ib: Fix bug in creation of dma regions

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <PAXPR83MB055734F57720593B5577A23DB44B2@PAXPR83MB0557.EURPRD83.prod.outlook.com>
Date: Fri, 9 Feb 2024 21:04:50 +0000
From: Konstantin Taranov <kotaranov@...rosoft.com>
To: Jason Gunthorpe <jgg@...pe.ca>
CC: Long Li <longli@...rosoft.com>, Konstantin Taranov
	<kotaranov@...ux.microsoft.com>, "leon@...nel.org" <leon@...nel.org>,
	"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

> From: Jason Gunthorpe <jgg@...pe.ca>
> > > > > > +
> > > > > > + 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;
> > > > > > +         }
> > > > > > + }
> > > > > > +
> > >
> > > Yes this doesn't look quite right..
> > >
> > > It should flow from the HW capability, the helper you call should be
> > > tightly linked to what the HW can do.
> > >
> > > ib_umem_find_best_pgsz() is used for MRs that have the usual
> > >   offset = IOVA % pgsz
> > >
> > > We've always created other helpers for other restrictions.
> > >
> > > So you should move your "force_zero_offset" into another helper and
> > > describe exactly how the HW works to support the calculation
> > >
> > > It is odd to have the offset loop and be using
> > > ib_umem_find_best_pgsz() with some iova, usually you'd use
> > > ib_umem_find_best_pgoff() in those cases, see the other callers.
> >
> > Hi Jason,
> > Thanks for the comments.
> >
> > To be honest, I do not understand how I could employ
> > ib_umem_find_best_pgoff for my purpose. As well as I do not see any
> mistake in the patch, and I think you neither.
> 
> It does exactly the same thing, it is just intended to be used by things that
> are not doing the IOVA calculation. It is a matter of documentation.
> 
> > I can make a special helper, but I do not think that it will be useful
> > to anyone. Plus, there is no better approach then halving the page
> > size, so the helper will end up with that loop under the hood. As I
> > see mlnx also uses a loop with halving page_sz, but for a different purpose,
> I do not see why our code cannot do the same without a special helper.
> 
> Are you sure you don't need the length check too? You have a granular size
> but not a granular offset?

Yes, we do not have constraints on the size.

> 
> In that case yes, a helper does not seem necessary
> 
> However, you should still be calling ib_umem_find_best_pgoff() for the
> initialize sizing as a matter of clarity since this is not a MR and does not use
> IOVA addressing.

Thanks for the clarification! I agree that the use of ib_umem_find_best_pgoff()
will make the code more understandable, even though it will do the same computation.
I have already prepared the patch. I will send it next week after running tests.

> 
> Jason