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

[Bob Pearson <rpearsonhpe@gmail.com>]

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