[PATCH v2] RDMA/rxe: Set wqe->status correctly if an unexpected response is received

[PATCH v2] RDMA/rxe: Set wqe->status correctly if an unexpected response is received

[Bart Van Assche]

Every function that returns COMPST_ERROR must set wqe->status to
another value than IB_WC_SUCCESS before returning COMPST_ERROR. Fix
the only code path for which this is not yet the case.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Zhu Yanjun <yanjun.zhu@oracle.com>
Cc: Jianchao Wang <jianchao.w.wang@oracle.com>
Cc: Yuval Shaia <yuval.shaia@oracle.com>
Cc: <stable@vger.kernel.org>
---

Changes compared to v1: left out a pr_err() statement as requested by Jason.

 drivers/infiniband/sw/rxe/rxe_comp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
index 98d470d1f3fc..83311dd07019 100644
--- a/drivers/infiniband/sw/rxe/rxe_comp.c
+++ b/drivers/infiniband/sw/rxe/rxe_comp.c
@@ -276,6 +276,7 @@ static inline enum comp_state check_ack(struct rxe_qp *qp,
 	case IB_OPCODE_RC_RDMA_READ_RESPONSE_MIDDLE:
 		if (wqe->wr.opcode != IB_WR_RDMA_READ &&
 		    wqe->wr.opcode != IB_WR_RDMA_READ_WITH_INV) {
+			wqe->status = IB_WC_FATAL_ERR;
 			return COMPST_ERROR;
 		}
 		reset_retry_counters(qp);
-- 
2.17.1

Re: [PATCH v2] RDMA/rxe: Set wqe->status correctly if an unexpected response is received

[Yuval Shaia]

On Tue, Jun 26, 2018 at 08:39:36AM -0700, Bart Van Assche wrote:
> Every function that returns COMPST_ERROR must set wqe->status to
> another value than IB_WC_SUCCESS before returning COMPST_ERROR. Fix
> the only code path for which this is not yet the case.

It is unclear to me why other places left out from this fix (e.x. lines
248, 288 etc).

> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Zhu Yanjun <yanjun.zhu@oracle.com>
> Cc: Jianchao Wang <jianchao.w.wang@oracle.com>
> Cc: Yuval Shaia <yuval.shaia@oracle.com>
> Cc: <stable@vger.kernel.org>
> ---
> 
> Changes compared to v1: left out a pr_err() statement as requested by Jason.
> 
>  drivers/infiniband/sw/rxe/rxe_comp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index 98d470d1f3fc..83311dd07019 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
> @@ -276,6 +276,7 @@ static inline enum comp_state check_ack(struct rxe_qp *qp,
>  	case IB_OPCODE_RC_RDMA_READ_RESPONSE_MIDDLE:
>  		if (wqe->wr.opcode != IB_WR_RDMA_READ &&
>  		    wqe->wr.opcode != IB_WR_RDMA_READ_WITH_INV) {
> +			wqe->status = IB_WC_FATAL_ERR;
>  			return COMPST_ERROR;
>  		}
>  		reset_retry_counters(qp);
> -- 
> 2.17.1
> 

Re: [PATCH v2] RDMA/rxe: Set wqe->status correctly if an unexpected response is received

[Bart Van Assche]

On 06/27/18 09:32, Yuval Shaia wrote:
> On Tue, Jun 26, 2018 at 08:39:36AM -0700, Bart Van Assche wrote:
>> Every function that returns COMPST_ERROR must set wqe->status to
>> another value than IB_WC_SUCCESS before returning COMPST_ERROR. Fix
>> the only code path for which this is not yet the case.
> 
> It is unclear to me why other places left out from this fix (e.x. lines
> 248, 288 etc).

Hello Yuval,

wqe->status is already set before the code that you mentioned has been 
reached.

Bart.

Re: [PATCH v2] RDMA/rxe: Set wqe->status correctly if an unexpected response is received

[Moni Shoua]

>
> wqe->status is already set before the code that you mentioned has been
> reached.
can you be more specific?
for instance I didn't find similar behavior for the example below
(also in check_ack())

        case IB_OPCODE_RC_ATOMIC_ACKNOWLEDGE:
                syn = aeth_syn(pkt);

                if ((syn & AETH_TYPE_MASK) != AETH_ACK)
                        return COMPST_ERROR;

                if (wqe->wr.opcode != IB_WR_ATOMIC_CMP_AND_SWP &&
                    wqe->wr.opcode != IB_WR_ATOMIC_FETCH_AND_ADD)
                        return COMPST_ERROR;   <<<<<<<<<<<<<<<<<<<<<<<<<<
                reset_retry_counters(qp);
                return COMPST_ATOMIC;

Re: [PATCH v2] RDMA/rxe: Set wqe->status correctly if an unexpected response is received

[Bart Van Assche]

On 06/28/18 01:58, Moni Shoua wrote:
>> wqe->status is already set before the code that you mentioned has been
>> reached.
 >
> can you be more specific?
> for instance I didn't find similar behavior for the example below
> (also in check_ack())
> 
>          case IB_OPCODE_RC_ATOMIC_ACKNOWLEDGE:
>                  syn = aeth_syn(pkt);
> 
>                  if ((syn & AETH_TYPE_MASK) != AETH_ACK)
>                          return COMPST_ERROR;
> 
>                  if (wqe->wr.opcode != IB_WR_ATOMIC_CMP_AND_SWP &&
>                      wqe->wr.opcode != IB_WR_ATOMIC_FETCH_AND_ADD)
>                          return COMPST_ERROR;   <<<<<<<<<<<<<<<<<<<<<<<<<<
>                  reset_retry_counters(qp);
>                  return COMPST_ATOMIC;

Hello Moni,

I may have been wrong when I claimed that all other paths that return 
COMPST_ERROR already set wqe->status to an error value. However, I only 
have a test case for the path I modified. Hence my patch.

Bart.

Re: [PATCH v2] RDMA/rxe: Set wqe->status correctly if an unexpected response is received

[Jason Gunthorpe]

On Tue, Jun 26, 2018 at 08:39:36AM -0700, Bart Van Assche wrote:
> Every function that returns COMPST_ERROR must set wqe->status to
> another value than IB_WC_SUCCESS before returning COMPST_ERROR. Fix
> the only code path for which this is not yet the case.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Zhu Yanjun <yanjun.zhu@oracle.com>
> Cc: Jianchao Wang <jianchao.w.wang@oracle.com>
> Cc: Yuval Shaia <yuval.shaia@oracle.com>
> Cc: <stable@vger.kernel.org>
> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
> 
> Changes compared to v1: left out a pr_err() statement as requested by Jason.
> 
>  drivers/infiniband/sw/rxe/rxe_comp.c | 1 +
>  1 file changed, 1 insertion(+)

applied to for-next

Thanks,
Jason