[bug report] RDMA/rxe: Protect QP state with qp->state_lock

[bug report] RDMA/rxe: Protect QP state with qp->state_lock

[Dan Carpenter]

Hello Bob Pearson,

The patch f605f26ea196: "RDMA/rxe: Protect QP state with
qp->state_lock" from Apr 4, 2023, leads to the following Smatch
static checker warning:

	drivers/infiniband/sw/rxe/rxe_qp.c:716 rxe_qp_to_attr()
	error: double unlocked '&qp->state_lock' (orig line 713)

drivers/infiniband/sw/rxe/rxe_qp.c
    705         rxe_av_to_attr(&qp->pri_av, &attr->ah_attr);
    706         rxe_av_to_attr(&qp->alt_av, &attr->alt_ah_attr);
    707 
    708         /* Applications that get this state typically spin on it.
    709          * Yield the processor
    710          */
    711         spin_lock_bh(&qp->state_lock);
    712         if (qp->attr.sq_draining) {
    713                 spin_unlock_bh(&qp->state_lock);
                             ^^^^^^
Unlock

    714                 cond_resched();
    715         }
--> 716         spin_unlock_bh(&qp->state_lock);
                     ^^^^^^
Double unlock

    717 
    718         return 0;
    719 }

regards,
dan carpenter

Re: [bug report] RDMA/rxe: Protect QP state with qp->state_lock

[Leon Romanovsky]

On Thu, May 04, 2023 at 10:28:59AM +0300, Dan Carpenter wrote:
> Hello Bob Pearson,
> 
> The patch f605f26ea196: "RDMA/rxe: Protect QP state with
> qp->state_lock" from Apr 4, 2023, leads to the following Smatch
> static checker warning:
> 
> 	drivers/infiniband/sw/rxe/rxe_qp.c:716 rxe_qp_to_attr()
> 	error: double unlocked '&qp->state_lock' (orig line 713)
> 
> drivers/infiniband/sw/rxe/rxe_qp.c
>     705         rxe_av_to_attr(&qp->pri_av, &attr->ah_attr);
>     706         rxe_av_to_attr(&qp->alt_av, &attr->alt_ah_attr);
>     707 
>     708         /* Applications that get this state typically spin on it.
>     709          * Yield the processor
>     710          */
>     711         spin_lock_bh(&qp->state_lock);
>     712         if (qp->attr.sq_draining) {
>     713                 spin_unlock_bh(&qp->state_lock);
>                              ^^^^^^
> Unlock
> 
>     714                 cond_resched();
>     715         }

Arguably, lines 708-716 should be deleted.

Thanks

> --> 716         spin_unlock_bh(&qp->state_lock);
>                      ^^^^^^
> Double unlock
> 
>     717 
>     718         return 0;
>     719 }
> 
> regards,
> dan carpenter

Re: [bug report] RDMA/rxe: Protect QP state with qp->state_lock

[Bob Pearson]

On 5/4/23 03:07, Leon Romanovsky wrote:
> On Thu, May 04, 2023 at 10:28:59AM +0300, Dan Carpenter wrote:
>> Hello Bob Pearson,
>>
>> The patch f605f26ea196: "RDMA/rxe: Protect QP state with
>> qp->state_lock" from Apr 4, 2023, leads to the following Smatch
>> static checker warning:
>>
>> 	drivers/infiniband/sw/rxe/rxe_qp.c:716 rxe_qp_to_attr()
>> 	error: double unlocked '&qp->state_lock' (orig line 713)
>>
>> drivers/infiniband/sw/rxe/rxe_qp.c
>>     705         rxe_av_to_attr(&qp->pri_av, &attr->ah_attr);
>>     706         rxe_av_to_attr(&qp->alt_av, &attr->alt_ah_attr);
>>     707 
>>     708         /* Applications that get this state typically spin on it.
>>     709          * Yield the processor
>>     710          */
>>     711         spin_lock_bh(&qp->state_lock);
>>     712         if (qp->attr.sq_draining) {
>>     713                 spin_unlock_bh(&qp->state_lock);
>>                              ^^^^^^
>> Unlock
>>
>>     714                 cond_resched();
>>     715         }
> 
> Arguably, lines 708-716 should be deleted.
> 
> Thanks
> 
>> --> 716         spin_unlock_bh(&qp->state_lock);

Bad fix as you suggest. Should have been

       715         } else {
       716                spin_unlock_bh(...);
       717         }

Fortunately I have never seen anyone using the SQD 'state' (which includes sq_draining) for anything, but
it is in the IBA spec. I was trying to protect all references to qp 'state' by the spinlock which provides
smp safe accesses. The cond_resched() call was already there so I didn't want to mix gratuitous changes
in with the spinlock changes.

I think we should fix this, as above, and if it makes sense drop the cond_resched() call in a separate
patch.

There is another patch changing all these to _irqsave/irqrestore spinlocks from Guoqing Jiang which fixes a bug
in blktests so one of these has to go in first and then the other. Which ever one is easier.

Bob
>>                      ^^^^^^
>> Double unlock
>>
>>     717 
>>     718         return 0;
>>     719 }
>>
>> regards,
>> dan carpenter