On Fri, Feb 25, 2022 at 01:57:46PM -0600, Bob Pearson wrote:
> int __rxe_add_ref(struct rxe_pool_elem *elem)
> @@ -262,3 +258,36 @@ int __rxe_drop_ref(struct rxe_pool_elem *elem)
> return kref_put_lock_irqsave(&elem->ref_cnt, rxe_elem_release,
> &pool->xa.xa_lock);
Also can't touch the xa_lock to do stuff like this,
> +int __rxe_drop_wait(struct rxe_pool_elem *elem)
I think I would call this something else since it it basically unconditionally
frees the memory.
> +{
> + struct rxe_pool *pool = elem->pool;
> + static int timeout = RXE_POOL_TIMEOUT;
> + int ret;
> +
> + __rxe_drop_ref(elem);
> + if (timeout) {
> + ret = wait_for_completion_timeout(&elem->complete, timeout);
> + if (!ret) {
> + pr_warn("Timed out waiting for %s#%d\n",
> + pool->name + 4, elem->index);
This is a WARN_ON event, kernel is broken, and you should leak the
memory rather than cause memory corruption.
> + if (++pool->timeouts == RXE_MAX_POOL_TIMEOUTS) {
> + timeout = 0;
> + pr_warn("Reached max %s timeouts.\n",
> + pool->name + 4);
> + }
Why?
> + }
> + }
> +
> + if (pool->cleanup)
> + pool->cleanup(elem);
> +
> + if (pool->flags & RXE_POOL_ALLOC)
> + kfree(elem->obj);
> +
> + atomic_dec(&pool->num_elem);
> +
> + return ret;
And we return a failure code but freed the memory? This shouldn't fail
But the idea is right..
Jason
