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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 8 Feb 2024 20:31:15 -0400
From: Jason Gunthorpe <jgg@...pe.ca>
To: Konstantin Taranov <kotaranov@...rosoft.com>
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

> > > > > +
> > > > > + 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?

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.

Jason