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
