On Sun, Dec 09, 2018 at 09:24:37AM +0200, Yuval Shaia wrote:
> On Thu, Dec 06, 2018 at 09:47:01AM -0800, Bart Van Assche wrote:
> > On Thu, 2018-12-06 at 13:11 +0200, Yuval Shaia wrote:
> > > Pool state is set to 'invalid' indirectly by function rxe_pool_put which
> > > is called anyway here so no need to update the state twice.
> > >
> > > Signed-off-by: Yuval Shaia <yuval.shaia@xxxxxxxxxx>
> > > drivers/infiniband/sw/rxe/rxe_pool.c | 1 -
> > > 1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
> > > index 66728086169b..cfe8051c2683 100644
> > > +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
> > > @@ -248,7 +248,6 @@ void rxe_pool_cleanup(struct rxe_pool *pool)
> > > unsigned long flags;
> > >
> > > write_lock_irqsave(&pool->pool_lock, flags);
> > > - pool->state = RXE_POOL_STATE_INVALID;
> > > if (atomic_read(&pool->num_elem) > 0)
> > > pr_warn("%s pool destroyed with unfree'd elem\n",
> > > pool_name(pool));
> >
> > rxe_pool_put() only causes the pool state to change after the pool reference
> > count has dropped to zero. So I think the pool state change in rxe_pool_cleanup()
> > is helpful to catch use-after-free of a pool.
>
> But isn't it the idea with using the ref-count, i.e. the pool *is not*
> cleaned until all reference are freed?
>
> At least from the flow it looks like that pool is valid until last
> reference is released so why declaring that it is not?
>
> If we want pool to be invalid in the first call to cleanup then let's drop
> this entire ref-counting.
This is what should be done, yes.
The driver *must* fully clean up everything related to it during
unregister. Otherwise it will have module unload races and other bad
bugs.
So if a refcount is used it must be bounded and fenced to zero as part
of cleanup.
Jason
