linux-kernel - Re: [PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem

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]

Message-ID: <20210204200026.GP4247@nvidia.com>
Date:   Thu, 4 Feb 2021 16:00:26 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     John Hubbard <jhubbard@...dia.com>
CC:     Joao Martins <joao.m.martins@...cle.com>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, <linux-rdma@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Doug Ledford <dledford@...hat.com>,
        Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem_release()

On Wed, Feb 03, 2021 at 04:15:53PM -0800, John Hubbard wrote:
> > diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> > index 2dde99a9ba07..ea4ebb3261d9 100644
> > +++ b/drivers/infiniband/core/umem.c
> > @@ -47,17 +47,17 @@
> >   static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
> >   {
> > -	struct sg_page_iter sg_iter;
> > -	struct page *page;
> > +	bool make_dirty = umem->writable && dirty;
> > +	struct scatterlist *sg;
> > +	int i;
> 
> Maybe unsigned int is better, so as to perfectly match the scatterlist.length.

Yes please

> >   	if (umem->nmap > 0)
> >   		ib_dma_unmap_sg(dev, umem->sg_head.sgl, umem->sg_nents,
> >   				DMA_BIDIRECTIONAL);
> > -	for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
> > -		page = sg_page_iter_page(&sg_iter);
> > -		unpin_user_pages_dirty_lock(&page, 1, umem->writable && dirty);
> > -	}
> > +	for_each_sg(umem->sg_head.sgl, sg, umem->nmap, i)
> 
> The change from umem->sg_nents to umem->nmap looks OK, although we should get
> IB people to verify that there is not some odd bug or reason to leave it as is.

No, nmap wouldn't be right here. nmap is the number of dma mapped SGLs
in the list and should only be used by things doing sg_dma* stuff.

umem->sg_nents is the number of CPU SGL entries and is the correct
thing here.

> > +		unpin_user_page_range_dirty_lock(sg_page(sg),
> > +			DIV_ROUND_UP(sg->length, PAGE_SIZE), make_dirty);
> 
> Is it really OK to refer directly to sg->length? The scatterlist library goes
> to some effort to avoid having callers directly access the struct member variables.

Yes, only the dma length has acessors

Jason