Re: [PATCH] RDMA: null pointer in __ib_umem_release causes kernel panic

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Jan 05, 2022 at 03:02:34PM +0000, Trond Myklebust wrote:
> On Wed, 2022-01-05 at 10:37 -0400, Jason Gunthorpe wrote:
> > On Wed, Jan 05, 2022 at 09:18:41AM -0500, trondmy@kernel.org wrote:
> > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > 
> > > When doing RPC/RDMA, we're seeing a kernel panic when
> > > __ib_umem_release()
> > > iterates over the scatter gather list and hits NULL pages.
> > > 
> > > It turns out that commit 79fbd3e1241c ended up changing the
> > > iteration
> > > from being over only the mapped entries to being over the original
> > > list
> > > size.
> > 
> > You mean this?
> > 
> > -       for_each_sg(umem->sg_head.sgl, sg, umem->sg_nents, i)
> > +       for_each_sgtable_sg(&umem->sgt_append.sgt, sg, i)
> > 
> > I don't see what changed there? The invarient should be that
> > 
> >   umem->sg_nents == sgt->orig_nents
> > 
> > > @@ -55,7 +55,7 @@ static void __ib_umem_release(struct ib_device
> > > *dev, struct ib_umem *umem, int d
> > >                 ib_dma_unmap_sgtable_attrs(dev, &umem-
> > > >sgt_append.sgt,
> > >                                            DMA_BIDIRECTIONAL, 0);
> > >  
> > > -       for_each_sgtable_sg(&umem->sgt_append.sgt, sg, i)
> > > +       for_each_sgtable_dma_sg(&umem->sgt_append.sgt, sg, i)
> > >                 unpin_user_page_range_dirty_lock(sg_page(sg),
> > 
> > Calling sg_page() from under a dma_sg iterator is unconditionally
> > wrong..
> > 
> > More likely your case is something has gone wrong when the sgtable
> > was
> > created and it has the wrong value in orig_nents..
> 
> Can you define "wrong value" in this case? Chuck's RPC/RDMA code
> appears to call ib_alloc_mr() with an 'expected maximum number of
> entries' (depth) in net/sunrpc/xprtrdma/frwr_ops.c:frwr_mr_init().
> 
> It then fills that table with a set of n <= depth pages in
> net/sunrpc/xprtrdma/frwr_ops.c:frwr_map() and calls ib_dma_map_sg() to
> map them, and then adjusts the sgtable with a call to ib_map_mr_sg().

I'm confused, RPC/RDMA should never touch a umem at all.

Is this really the other bug where user and kernel MR are getting
confused?

Jason