On 7/6/22 08:11, Haris Iqbal wrote:
> On Wed, Jul 6, 2022 at 3:10 PM Yanjun Zhu <yanjun.zhu@xxxxxxxxx> wrote:
>>
>> 在 2022/7/5 18:35, Haris Iqbal 写道:
>>> On Tue, Jul 5, 2022 at 8:28 AM <yanjun.zhu@xxxxxxxxx> wrote:
>>>>
>>>> From: Zhu Yanjun <yanjun.zhu@xxxxxxxxx>
>>>>
>>>> The function rxe_create_qp calls rxe_qp_from_init. If some error
>>>> occurs, the error handler of function rxe_qp_from_init will set
>>>> both scq and rcq to NULL.
>>>>
>>>> Then rxe_create_qp calls rxe_put to handle qp. In the end,
>>>> rxe_qp_do_cleanup is called by rxe_put. rxe_qp_do_cleanup directly
>>>> accesses scq and rcq before checking them. This will cause
>>>> null-ptr-deref error.
>>>>
>>>> The call graph is as below:
>>>>
>>>> rxe_create_qp {
>>>> ...
>>>> rxe_qp_from_init {
>>>> ...
>>>> err1:
>>>> ...
>>>> qp->rcq = NULL; <---rcq is set to NULL
>>>> qp->scq = NULL; <---scq is set to NULL
>>>> ...
>>>> }
>>>>
>>>> qp_init:
>>>> rxe_put{
>>>> ...
>>>> rxe_qp_do_cleanup {
>>>> ...
>>>> atomic_dec(&qp->scq->num_wq); <--- scq is accessed
>>>> ...
>>>> atomic_dec(&qp->rcq->num_wq); <--- rcq is accessed
>>>> }
>>>> }
>>>>
>>>> Fixes: 4703b4f0d94a ("RDMA/rxe: Enforce IBA C11-17")
>>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@xxxxxxxxx>
>>>> ---
>>>> V1->V2: Describe the error flows.
>>>> ---
>>>> drivers/infiniband/sw/rxe/rxe_qp.c | 10 ++++++----
>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
>>>> index 22e9b85344c3..b79e1b43454e 100644
>>>> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
>>>> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
>>>> @@ -804,13 +804,15 @@ static void rxe_qp_do_cleanup(struct work_struct *work)
>>>> if (qp->rq.queue)
>>>> rxe_queue_cleanup(qp->rq.queue);
>>>>
>>>> - atomic_dec(&qp->scq->num_wq);
>>>> - if (qp->scq)
>>>> + if (qp->scq) {
>>>> + atomic_dec(&qp->scq->num_wq);
>>>> rxe_put(qp->scq);
>>>> + }
>>>>
>>>> - atomic_dec(&qp->rcq->num_wq);
>>>> - if (qp->rcq)
>>>> + if (qp->rcq) {
>>>> + atomic_dec(&qp->rcq->num_wq);
>>>> rxe_put(qp->rcq);
>>>> + }
>>>>
>>>> if (qp->pd)
>>>> rxe_put(qp->pd);
>>>> --
>>>> 2.34.1
>>>
>>> Looks good.
>>>
>>> Reviewed-by: Md Haris Iqbal <haris.iqbal@xxxxxxxxx>
>>>
>>> Should the check for "rxe_cleanup_task(&qp->comp.task);" also happen
>>> in this commit? or would it be covered in a different one?
>>
>> It is in a different commit. I will send it out very soon.
>
> Okay. Thank you!
>
>>
>> Zhu Yanjun
>>
>>>
>>>>
>>
Agreed.
Reviewed-by: Bob Pearson <rpearsonhpe@xxxxxxxxx>
