On Sun, Jan 06, 2019 at 10:51:58AM +0200, Leon Romanovsky wrote:
> On Fri, Jan 04, 2019 at 05:24:21PM +0000, Jason Gunthorpe wrote:
> > On Fri, Jan 04, 2019 at 07:12:54PM +0200, Leon Romanovsky wrote:
> > > On Fri, Jan 04, 2019 at 04:23:49PM +0000, Jason Gunthorpe wrote:
> > > > On Fri, Jan 04, 2019 at 08:05:06AM +0200, Leon Romanovsky wrote:
> > > > > On Thu, Jan 03, 2019 at 10:30:26PM +0000, Jason Gunthorpe wrote:
> > > > > > On Thu, Jan 03, 2019 at 10:05:59AM +0200, Leon Romanovsky wrote:
> > > > > > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > > >
> > > > > > > XArray uses internal lock for updates to XArray. This means that our
> > > > > > > external RW lock is not needed anymore and we can delete it.
> > > > > >
> > > > > > The xarray lock only protects the array itself - not the things it
> > > > > > points to.
> > > > > >
> > > > > > > - down_read(&res->rwsem);
> > > > > > > xa_for_each(&res->xa[type], e, index, ULONG_MAX, XA_PRESENT) {
> > > > > > > if (ns == &init_pid_ns ||
> > > > > > > (!rdma_is_kernel_res(e) &&
> > > > > >
> > > > > > So eg here, 'e' is still needs the locking to protect against
> > > > > > concurrent restrack_del causing e to be freed.
> > > > > >
> > > > > > Most likely the internal xarray locking doesn't save anything in this
> > > > > > usage..
> > > > >
> > > > > The "rwsem" protects hash lists insert/delete and we are still using
> > > > > kref to protect specific entries. And those xarray insert/deletion are
> > > > > handled by xa_lock() now.
> > > >
> > > > This doesn't explain where the locking for 'e' comes from above
> > >
> > > According to the documentation, xa_for_each takes RCU lock while
> > > iterating, so I assume that "e" is safe once it returned from xarray,
> >
> > The documentation means it internally takes/releases the RCU lock (in
> > contrast to radix tree that requires callers to hold the RCU lock to
> > protect the internals of the radix tree).
> >
> > See the implementation, the loop body is run with RCU unlocked.
> >
> > The xarray interfaces only protect its insides, the caller is
> > responsible to protect the returned data.
>
> Thanks for the pointer.
>
> Because XArray ensure that iteration is safe, we can grab kref on entry
> to ensure that it is not released.
I just said that the for_each body is not called with any lock held..
The caller must ensure that the races with freeing 'e' are prevented
through caller provided locking, which you just deleted.
> xa_for_each(&res->xa[type], e, index, ULONG_MAX, XA_PRESENT) {
> + if (!rdma_restrack_get(e))
> + continue;
So 'e' can still be free'd memory here, as there is no locking
protecting it.
Jason
