[PATCH rdma-next 0/4] RDMA: Use unsigned comparison in CQ cleanup loops

[PATCH rdma-next 0/4] RDMA: Use unsigned comparison in CQ cleanup loops

[Edward Srouji]

mlx5, mlx4, mthca and hns all sweep the CQ backwards when cleaning
completions for a QP that is being destroyed, using the same open-coded
loop (mlx5 shown):
	while ((int) --prod_index - (int) cq->mcq.cons_index >= 0)
Both indexes are free running u32 counters, so the comparison has to be
done modulo 2^32. Casting each operand to int and subtracting does not
do that: the subtraction overflows once the two indexes straddle 2^31,
which is undefined behaviour, and a compiler that assumes signed
overflow cannot occur is free to fold the expression into a plain signed
comparison that is not wraparound safe.
This is not a fix. The kernel is built with -fno-strict-overflow, so
gcc and clang both retain the subtraction, the generated code is
unaffected, and there is no known user-visible impact.  The casts buy
nothing, though, and the correctness of these loops should not rest on
a build flag. The series drops the arithmetic instead:
	while (prod_index != cq->mcq.cons_index) {
		--prod_index;
		...
No functional change intended.

Signed-off-by: Edward Srouji <edwards@nvidia.com>
---
Yishai Hadas (4):
      RDMA/mlx5: Use unsigned comparison in the CQ cleanup loop
      RDMA/mlx4: Use unsigned comparison in the CQ cleanup loop
      RDMA/mthca: Use unsigned comparison in the CQ cleanup loop
      RDMA/hns: Use unsigned comparison in the CQ cleanup loop

 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 3 ++-
 drivers/infiniband/hw/mlx4/cq.c            | 3 ++-
 drivers/infiniband/hw/mlx5/cq.c            | 3 ++-
 drivers/infiniband/hw/mthca/mthca_cq.c     | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)
---
base-commit: 3e1de7f906ab162b23d6fe0eabccf687a98fa25f
change-id: 20260915-fix-cq-cleanup-6512d4346ed7

Best regards,
-- 
Edward Srouji <edwards@nvidia.com>

[PATCH rdma-next 1/4] RDMA/mlx5: Use unsigned comparison in the CQ cleanup loop

[Edward Srouji]

From: Yishai Hadas <yishaih@nvidia.com>

__mlx5_ib_cq_clean() sweeps the CQ backwards from the producer index
down to the consumer index:

  while ((int) --prod_index - (int) cq->mcq.cons_index >= 0)

Both indexes are free running u32 counters, so the comparison has to
be done modulo 2^32.  Casting each operand to int and subtracting
does not do that: the subtraction overflows whenever the two indexes
straddle 2^31, which is undefined behaviour, and a compiler that
assumes signed overflow cannot occur is free to discard the
subtraction and fold the expression into a plain signed comparison.
That comparison is not wraparound safe.

The kernel is built with -fno-strict-overflow, so gcc and clang both
retain the subtraction today and the generated code is unaffected;
there is no known user-visible impact from the current code.  Still,
correctness here shouldn't depend on that build flag.

Replace the loop with a plain unsigned equality check instead.

  while (prod_index != cq->mcq.cons_index) {
          --prod_index;
          ...

The preceding forward scan starts prod_index at cons_index and stops no
later than cons_index + cq->ibcq.cqe, so the two indexes are at most
cq->ibcq.cqe apart.  Decrementing prod_index reaches cons_index in
exactly that many iterations regardless of whether either counter has
wrapped, because the loop no longer compares magnitudes at all.

Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
Signed-off-by: Edward Srouji <edwards@nvidia.com>
---
 drivers/infiniband/hw/mlx5/cq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
index 49b4bf148a4a017079c93a9b267690bf3a23169a..51892911e32a4b1c90c80e36c523e1906feeab8b 100644
--- a/drivers/infiniband/hw/mlx5/cq.c
+++ b/drivers/infiniband/hw/mlx5/cq.c
@@ -1169,7 +1169,8 @@ void __mlx5_ib_cq_clean(struct mlx5_ib_cq *cq, u32 rsn, struct mlx5_ib_srq *srq)
 	/* Now sweep backwards through the CQ, removing CQ entries
 	 * that match our QP by copying older entries on top of them.
 	 */
-	while ((int) --prod_index - (int) cq->mcq.cons_index >= 0) {
+	while (prod_index != cq->mcq.cons_index) {
+		--prod_index;
 		cqe = get_cqe(cq, prod_index & cq->ibcq.cqe);
 		cqe64 = (cq->mcq.cqe_sz == 64) ? cqe : cqe + 64;
 		if (is_equal_rsn(cqe64, rsn)) {

-- 
2.49.0

[PATCH rdma-next 2/4] RDMA/mlx4: Use unsigned comparison in the CQ cleanup loop

[Edward Srouji]

From: Yishai Hadas <yishaih@nvidia.com>

__mlx4_ib_cq_clean() sweeps the CQ backwards from the producer index
down to the consumer index:

  while ((int) --prod_index - (int) cq->mcq.cons_index >= 0)

Both indexes are free running u32 counters, so the comparison has to
be done modulo 2^32.  Casting each operand to int and subtracting
does not do that: the subtraction overflows whenever the two indexes
straddle 2^31, which is undefined behaviour, and a compiler that
assumes signed overflow cannot occur is free to discard the
subtraction and fold the expression into a plain signed comparison.
That comparison is not wraparound safe.

The kernel is built with -fno-strict-overflow, so gcc and clang both
retain the subtraction today and the generated code is unaffected;
there is no known user-visible impact from the current code.  Still,
correctness here shouldn't depend on that build flag.

Replace the loop with a plain unsigned equality check instead:

  while (prod_index != cq->mcq.cons_index) {
          --prod_index;
          ...

The preceding forward scan starts prod_index at cons_index and stops no
later than cons_index + cq->ibcq.cqe, so the two indexes are at most
cq->ibcq.cqe apart.  Decrementing prod_index reaches cons_index in
exactly that many iterations regardless of whether either counter has
wrapped, because the loop no longer compares magnitudes at all.

Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
Signed-off-by: Edward Srouji <edwards@nvidia.com>
---
 drivers/infiniband/hw/mlx4/cq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx4/cq.c b/drivers/infiniband/hw/mlx4/cq.c
index 8879124697420c2ba03622c2af3b350808dc0aa2..98e3eb5cf96e54efdec6e4d8c588fc865742337a 100644
--- a/drivers/infiniband/hw/mlx4/cq.c
+++ b/drivers/infiniband/hw/mlx4/cq.c
@@ -989,7 +989,8 @@ void __mlx4_ib_cq_clean(struct mlx4_ib_cq *cq, u32 qpn, struct mlx4_ib_srq *srq)
 	 * Now sweep backwards through the CQ, removing CQ entries
 	 * that match our QP by copying older entries on top of them.
 	 */
-	while ((int) --prod_index - (int) cq->mcq.cons_index >= 0) {
+	while (prod_index != cq->mcq.cons_index) {
+		--prod_index;
 		cqe = get_cqe(cq, prod_index & cq->ibcq.cqe);
 		cqe += cqe_inc;
 

-- 
2.49.0

[PATCH rdma-next 3/4] RDMA/mthca: Use unsigned comparison in the CQ cleanup loop

[Edward Srouji]

From: Yishai Hadas <yishaih@nvidia.com>

mthca_cq_clean() sweeps the CQ backwards from the producer index
down to the consumer index:

  while ((int) --prod_index - (int) cq->cons_index >= 0)

Both indexes are free running u32 counters, so the comparison has to
be done modulo 2^32.  Casting each operand to int and subtracting
does not do that: the subtraction overflows whenever the two indexes
straddle 2^31, which is undefined behaviour, and a compiler that
assumes signed overflow cannot occur is free to discard the
subtraction and fold the expression into a plain signed comparison.
That comparison is not wraparound safe.

The kernel is built with -fno-strict-overflow, so gcc and clang both
retain the subtraction today and the generated code is unaffected;
there is no known user-visible impact from the current code.  Still,
correctness here shouldn't depend on that build flag.

Replace the loop with a plain unsigned equality check instead:

  while (prod_index != cq->cons_index) {
          --prod_index;
          ...

The preceding forward scan starts prod_index at cons_index and stops no
later than cons_index + cq->ibcq.cqe, so the two indexes are at most
cq->ibcq.cqe apart.  Decrementing prod_index reaches cons_index in
exactly that many iterations regardless of whether either counter has
wrapped, because the loop no longer compares magnitudes at all.

Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
Signed-off-by: Edward Srouji <edwards@nvidia.com>
---
 drivers/infiniband/hw/mthca/mthca_cq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mthca/mthca_cq.c b/drivers/infiniband/hw/mthca/mthca_cq.c
index 26c3408dcacaea9a0187257cc16de1b90f43fa40..39d4ff15849e7f8c2c3d47b4504306a4a4431c5d 100644
--- a/drivers/infiniband/hw/mthca/mthca_cq.c
+++ b/drivers/infiniband/hw/mthca/mthca_cq.c
@@ -300,7 +300,8 @@ void mthca_cq_clean(struct mthca_dev *dev, struct mthca_cq *cq, u32 qpn,
 	 * Now sweep backwards through the CQ, removing CQ entries
 	 * that match our QP by copying older entries on top of them.
 	 */
-	while ((int) --prod_index - (int) cq->cons_index >= 0) {
+	while (prod_index != cq->cons_index) {
+		--prod_index;
 		cqe = get_cqe(cq, prod_index & cq->ibcq.cqe);
 		if (cqe->my_qpn == cpu_to_be32(qpn)) {
 			if (srq && is_recv_cqe(cqe))

-- 
2.49.0

[PATCH rdma-next 4/4] RDMA/hns: Use unsigned comparison in the CQ cleanup loop

[Edward Srouji]

From: Yishai Hadas <yishaih@nvidia.com>

__hns_roce_v2_cq_clean() sweeps the CQ backwards from the producer
index down to the consumer index:

  while ((int) --prod_index - (int) hr_cq->cons_index >= 0)

Both indexes are free running u32 counters, so the comparison has to
be done modulo 2^32.  Casting each operand to int and subtracting
does not do that: the subtraction overflows whenever the two indexes
straddle 2^31, which is undefined behaviour, and a compiler that
assumes signed overflow cannot occur is free to discard the
subtraction and fold the expression into a plain signed comparison.
That comparison is not wraparound safe.

The kernel is built with -fno-strict-overflow, so gcc and clang both
retain the subtraction today and the generated code is unaffected;
there is no known user-visible impact from the current code.  Still,
correctness here shouldn't depend on that build flag.

Replace the loop with a plain unsigned equality check instead:

  while (prod_index != hr_cq->cons_index) {
          --prod_index;
          ...

The preceding forward scan starts prod_index at cons_index and advances
it only while get_sw_cqe_v2() still reports a software owned entry, so
the two indexes stay within one CQ's worth of entries.  Decrementing
prod_index reaches cons_index in exactly that many iterations regardless
of whether either counter has wrapped, because the loop no longer
compares magnitudes at all.

Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
Signed-off-by: Edward Srouji <edwards@nvidia.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 1b2504301ebea4279eb0dcb2537c3574d0122d95..35be5b038f6861f1c330739f5c97817a4c3940a7 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -3828,7 +3828,8 @@ static void __hns_roce_v2_cq_clean(struct hns_roce_cq *hr_cq, u32 qpn,
 	 * Now backwards through the CQ, removing CQ entries
 	 * that match our QP by overwriting them with next entries.
 	 */
-	while ((int) --prod_index - (int) hr_cq->cons_index >= 0) {
+	while (prod_index != hr_cq->cons_index) {
+		--prod_index;
 		cqe = get_cqe_v2(hr_cq, prod_index & hr_cq->ib_cq.cqe);
 		if (hr_reg_read(cqe, CQE_LCL_QPN) == qpn) {
 			if (srq && hr_reg_read(cqe, CQE_S_R)) {

-- 
2.49.0