On 3/15/22 18:45, Jason Gunthorpe wrote:
> On Thu, Mar 03, 2022 at 06:08:04PM -0600, Bob Pearson wrote:
>> void rxe_pool_cleanup(struct rxe_pool *pool)
>> {
>> struct rxe_pool_elem *elem;
>> + struct xarray *xa = &pool->xa;
>> + unsigned long index = 0;
>> + unsigned long max = ULONG_MAX;
>> + unsigned int elem_count = 0;
>> + unsigned int obj_count = 0;
>> +
>> + do {
>> + elem = xa_find(xa, &index, max, XA_PRESENT);
>> + if (elem) {
>> + elem_count++;
>> + xa_erase(xa, index);
>> + if (pool->flags & RXE_POOL_ALLOC) {
>> + kfree(elem->obj);
>> + obj_count++;
>> + }
>> }
>> + } while (elem);
>>
>> + if (WARN_ON(elem_count || obj_count))
>> + pr_debug("Freed %d indices and %d objects from pool %s\n",
>> + elem_count, obj_count, pool->name);
>
> Can this just be
>
> WARN_ON(!xa_empty(xa));
>
> ?
>
> Freeing memory that is still in use upgrades a resource leak to a UAF
> security bug, so that is usually not good.
>
> Jason
FWIW rxe_pool_cleanup() gets called in rxe_dealloc() which is the .dealloc_driver member in ib_device_ops.
In other words not until you are unloading the driver and only MRs are freed and the memory will never get used again. I have found it useful when debugging ref count mistakes.
OTOH, I can save the hunk and apply it when I need it so I don't really care if it goes upstream. So feel free
to go ahead and change it. The one line should go into rxe_dealloc() in rxe.c.
Bob
