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
> > --- a/drivers/infiniband/sw/rxe/rxe_pool.c
> > +++ 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.
I'm probably missing something here so will be glad to understand the logic
better.
>
> Bart.
