Re:Re: [PATCH v2] RDMA/srpt: Fix srpt_alloc_rw_ctxs() unwind counters

[kensanya]

At 2026-07-17 01:37:39, "Bart Van Assche" <bvanassche@xxxxxxx> wrote:
>On 7/15/26 3:15 AM, kensanya@xxxxxxx wrote:
>> From: TanZheng <tanzheng@xxxxxxxxxx>
>> 
>> When srpt_alloc_rw_ctxs() fails partway through a multi-buffer indirect
>> descriptor, the unwind path destroys RDMA contexts but leaves stale
>> n_rw_ctx and n_rdma values (and a dangling rw_ctxs pointer). Later
>> sq_wr_avail accounting in srpt_queue_response() or srpt_write_pending()
>> can then subtract the wrong number of send queue credits.
>> 
>> Reset the counters and clear rw_ctxs after freeing the heap
>> allocation before returning an error.
>> 
>> Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
>> Signed-off-by: TanZheng <tanzheng@xxxxxxxxxx>
>> ---
>> v2:
>> - After kfree(), set rw_ctxs to NULL instead of &s_rw_ctx
>>    (Leon Romanovsky)
>> 
>>   drivers/infiniband/ulp/srpt/ib_srpt.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> index f66cfd70c263..a9c4995af7a3 100644
>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> @@ -1014,8 +1014,12 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
>>   				ctx->sg, ctx->nents, dir);
>>   		target_free_sgl(ctx->sg, ctx->nents);
>>   	}
>> -	if (ioctx->rw_ctxs != &ioctx->s_rw_ctx)
>> +	if (ioctx->rw_ctxs != &ioctx->s_rw_ctx) {
>>   		kfree(ioctx->rw_ctxs);
>> +		ioctx->rw_ctxs = NULL;
>> +	}
>> +	ioctx->n_rw_ctx = 0;
>> +	ioctx->n_rdma = 0;
>>   	return ret;
>>   }
>
>The above looks wrong to me. In the error path ioctx->n_rw_ctx should be
>restored to the value it had at the start of the function instead of
>resetting it to zero.
>
>Bart.

Hi Bart,

I have a question about restoring n_rw_ctx/n_rdma from local
snapshots versus clearing them to 0 on the unwind path.

Looking at the call chain:
  srpt_handle_new_iu()
    -> srpt_get_send_ioctx()   /* sets n_rdma = 0, n_rw_ctx = 0 */
    -> srpt_get_desc_tbl()
         -> srpt_alloc_rw_ctxs()

so when srpt_alloc_rw_ctxs() is entered, both counters are already
0.  On the current call path, assigning 0 on unwind seems
equivalent to restoring the values saved at function entry.

Is the save/restore preferred because the loop starts from
ioctx->n_rw_ctx (i.e. the function is written as if it may extend
an existing allocation), or is there another reason to prefer it
over clearing to 0?

Thanks,
TanZheng