[openib-general] Re: race in mthca_cq.c?

[Michael S. Tsirkin]

Quoting r. Michael S. Tsirkin <mst at mellanox.co.il>:
> Subject: race in mthca_cq.c?
> 
> Roland, I think I see a race in mthca: let's assume that
> a QP is destroyed. We remove the qpn from qp_table.
> 
> Before we have the chance to cleanup the CQ, another QP is created
> and put in the same slot in table. If the user now polls the CQ he'll see a
> completion for a wrong QP, since poll CQ does:
> 
>                *cur_qp = mthca_array_get(&dev->qp_table.qp,
>                                           be32_to_cpu(cqe->my_qpn) &
>                                           (dev->limits.num_qps - 1));
> 
> Is this analysis right?
> If yes, I think we can fix this by testing (*cur_qp)->qpn ==
> be32_to_cpu(cqe->my_qpn), does this make sense?
> 
> Same for userspace I guess?
> 
> It seems a similiar issue exists for CQs, does it not?
> And I think it can be solved in a similiar way, checking the CQN?


The following seems to work. How does it look?

---

Make sure completion/completion event is not for a stale QP/CQ
before reporting to user.

Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>

--- openib/drivers/infiniband/hw/mthca/mthca_cq.c	2006-05-09 21:07:28.623383000 +0300
+++ /mswg/work/mst/tmp/infiniband1/hw/mthca/mthca_cq.c	2006-06-08 23:46:52.404499000 +0300
@@ -217,9 +217,9 @@ void mthca_cq_completion(struct mthca_de
 {
 	struct mthca_cq *cq;
 
 	cq = mthca_array_get(&dev->cq_table.cq, cqn & (dev->limits.num_cqs - 1));
 
-	if (!cq) {
+	if (!cq || cq->cqn != cqn) {
 		mthca_warn(dev, "Completion event for bogus CQ %08x\n", cqn);
 		return;
 	}
@@ -513,10 +515,10 @@ static inline int mthca_poll_one(struct 
 		 * because CQs will be locked while QPs are removed
 		 * from the table.
 		 */
 		*cur_qp = mthca_array_get(&dev->qp_table.qp,
 					  be32_to_cpu(cqe->my_qpn) &
 					  (dev->limits.num_qps - 1));
-		if (!*cur_qp) {
+		if (!*cur_qp || (*cur_qp)->qpn != be32_to_cpu(cqe->my_qpn)) {
 			mthca_warn(dev, "CQ entry for unknown QP %06x\n",
 				   be32_to_cpu(cqe->my_qpn) & 0xffffff);
 			err = -EINVAL;

-- 
MST