Re: [PATCH rdma-next 0/6] RDMA: Fix restrack UAF in QP/CQ/SRQ destroy

[Patrisious Haddad <phaddad@nvidia.com>]

On 6/11/2026 10:11 PM, Jason Gunthorpe wrote:
> On Sun, Jun 07, 2026 at 09:18:07PM +0300, Edward Srouji wrote:
>> The resource-tracking (restrack) database is the back-end for the netlink
>> "rdma resource show" interface which pins objects with
>> rdma_restrack_get().
>> The QP/CQ/SRQ destroy flows call rdma_restrack_del() at the end of
>> ib_destroy_*_user(), after device->ops.destroy_*() had already freed the
>> vendor object. Therefore, a concurrent netlink dump could look the
>> object up and touch freed memory, causing a use-after-free via
>> ib_query_qp() for instance.
>>
>> Fix this by splitting the delete into a begin/commit/abort sequence:
>> begin_del() parks the entry as XA_ZERO_ENTRY (so lookups return NULL),
>> drops the birth reference and waits for in-flight readers to drain,
>> while keeping the index reserved. The destroy paths run begin_del()
>> first, then commit_del() on success or abort_del() on error.
>> abort_del() re-inserts into the reserved slot, so it needs no allocation
>> and cannot fail.
>>
>> The first two patches remove DCT and raw RSS QP restrack tracking as
>> they have never worked (their ID is unset/reserved at create time).
>>
>> Signed-off-by: Edward Srouji <edwards@nvidia.com>
>> ---
>> Patrisious Haddad (6):
>>        RDMA/mlx5: Remove DCT restrack tracking
>>        RDMA/mlx5: Remove raw RSS QP restrack tracking
>>        RDMA/core: Add rdma_restrack_begin/abort/commit_del() operations
>>        RDMA/core: Fix use after free in ib_query_qp()
>>        RDMA/core: Fix potential use after free in ib_destroy_cq_user()
>>        RDMA/core: Fix potential use after free in ib_destroy_srq_user()
> The pre-existing sashiko issues look real too, can you fix them also:
Sure but one of them is a false-positive though:
Before destroy_qp() is called, the counter is unconditionally unbound:
         rdma_counter_unbind_qp(qp, qp->port, true);
         ret = qp->device->ops.destroy_qp(qp, udata);
If destroy_qp() fails and we abort destruction here, the kref on the
counter was dropped in rdma_counter_unbind_qp(), but qp->counter is never
set to NULL.

This is actually wrong the qp->counter is actually set to NULL(inside 
the driver though not the core) so subsequent calls will hit the NULL 
check and return safely.
>
> https://sashiko.dev/#/patchset/20260607-restrack-uaf-fix-v1-0-d72e45eb76c2%40nvidia.com
>
> The sashiko notes about XA_ZERO_ENTRY seems to be really obviously
> wrong:
>
> void *__xa_cmpxchg(struct xarray *xa, unsigned long index,
> 			void *old, void *entry, gfp_t gfp)
> {
> 	return xa_zero_to_null(__xa_cmpxchg_raw(xa, index, old, entry, gfp));
> }
> EXPORT_SYMBOL(__xa_cmpxchg);
>
> This looks legit:
>
> For instance, in drivers/infiniband/core/cq.c:ib_free_cq():
>      ret = cq->device->ops.destroy_cq(cq, NULL);
>      WARN_ONCE(ret, "Destroy of kernel CQ shouldn't fail");
>      rdma_restrack_del(&cq->res);
Agreed I seem to have missed this one and the counter_release_one().
>
> and so on
>
> Please send a series switching more/all places to commit/abort,
> probably there should be very few/no calls to a naked del left.

I wonder what about places where switching to commit/abort doesnt fix an 
actual bug.

I had a bigger series that changes all places but decided to send only 
critical ones instead.

For instance "ib_dealloc_pd_user()" I didnt fix since there is no 
rdma_restrack_get() for it currently so it cant hit this issue - should 
I fix that as well ?

and ib_dereg_mr_user() actually calls the delete at start so it doesnt 
have this issue (but it also doesnt readd it to restrack when driver OP 
fails) - but here I think thats by design since the MR would be in weird 
state and we dont want to track it ?
>
> This doesn't apply on top of the restrack_sync addition, please rebase
> it.
>
> You should probably be refactoring rdma_restrack_sync() and using its
> parts in this implementation since it does the same things.
>
> I don't think this should NULL the task on abort either, it doesn't
> seem necessary.
I dont NULL the task on abort(I do NULL it on commit_del() though , or 
were you talking about the restrack_sync() ?
>
> Jason