| Message ID | 20221208210945.28607-1-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next] RDMA/rxe: Fix incorrect responder length checking | expand |
On Fri, Dec 9, 2022 6:10 AM Bob Pearson wrote: > The code in rxe_resp.c at check_length() is incorrect as it > compares pkt->opcode an 8 bit value against various mask bits > which are all higher than 256 so nothing is ever reported. > > This patch rewrites this to compare against pkt->mask which is > correct. However this now exposes another error. For UD send > packets the value of the pmtu cannot be determined from qp->mtu. > All that is required here is to later check if the payload fits > into the posted receive buffer in that case. > > Fixes: 837a55847ead ("RDMA/rxe: Implement packet length validation on responder") > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> Looks good. Thank you for the fix. Reviewed-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com> > --- > drivers/infiniband/sw/rxe/rxe_resp.c | 62 ++++++++++++++++------------ > 1 file changed, 36 insertions(+), 26 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c > index 6ac544477f3f..42261537772c 100644 > --- a/drivers/infiniband/sw/rxe/rxe_resp.c > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c > @@ -392,36 +392,46 @@ static enum resp_states check_resource(struct rxe_qp *qp, > return RESPST_CHK_LENGTH; > } > > -static enum resp_states check_length(struct rxe_qp *qp, > - struct rxe_pkt_info *pkt) > +static enum resp_states rxe_resp_check_length(struct rxe_qp *qp, > + struct rxe_pkt_info *pkt) > { > - int mtu = qp->mtu; > - u32 payload = payload_size(pkt); > - u32 dmalen = reth_len(pkt); > - > - /* RoCEv2 packets do not have LRH. > - * Let's skip checking it. > + /* > + * See IBA C9-92 > + * For UD QPs we only check if the packet will fit in the > + * receive buffer later. For rmda operations additional > + * length checks are performed in check_rkey. > */ > - > - if ((pkt->opcode & RXE_START_MASK) && > - (pkt->opcode & RXE_END_MASK)) { > - /* "only" packets */ > - if (payload > mtu) > - return RESPST_ERR_LENGTH; > - } else if ((pkt->opcode & RXE_START_MASK) || > - (pkt->opcode & RXE_MIDDLE_MASK)) { > - /* "first" or "middle" packets */ > - if (payload != mtu) > - return RESPST_ERR_LENGTH; > - } else if (pkt->opcode & RXE_END_MASK) { > - /* "last" packets */ > - if ((payload == 0) || (payload > mtu)) > - return RESPST_ERR_LENGTH; > + if (pkt->mask & RXE_PAYLOAD_MASK && ((qp_type(qp) == IB_QPT_RC) || > + (qp_type(qp) == IB_QPT_UC))) { > + unsigned int mtu = qp->mtu; > + unsigned int payload = payload_size(pkt); > + > + if ((pkt->mask & RXE_START_MASK) && > + (pkt->mask & RXE_END_MASK)) { > + if (unlikely(payload > mtu)) { > + rxe_dbg_qp(qp, "only packet too long"); > + return RESPST_ERR_LENGTH; > + } > + } else if ((pkt->mask & RXE_START_MASK) || > + (pkt->mask & RXE_MIDDLE_MASK)) { > + if (unlikely(payload != mtu)) { > + rxe_dbg_qp(qp, "first or middle packet not mtu"); > + return RESPST_ERR_LENGTH; > + } > + } else if (pkt->mask & RXE_END_MASK) { > + if (unlikely((payload == 0) || (payload > mtu))) { > + rxe_dbg_qp(qp, "last packet zero or too long"); > + return RESPST_ERR_LENGTH; > + } > + } > } > > - if (pkt->opcode & (RXE_WRITE_MASK | RXE_READ_MASK)) { > - if (dmalen > (1 << 31)) > + /* See IBA C9-94 */ > + if (pkt->mask & RXE_RETH_MASK) { > + if (reth_len(pkt) > (1U << 31)) { > + rxe_dbg_qp(qp, "dma length too long"); > return RESPST_ERR_LENGTH; > + } > } > > return RESPST_CHK_RKEY; > @@ -1397,7 +1407,7 @@ int rxe_responder(void *arg) > state = check_resource(qp, pkt); > break; > case RESPST_CHK_LENGTH: > - state = check_length(qp, pkt); > + state = rxe_resp_check_length(qp, pkt); > break; > case RESPST_CHK_RKEY: > state = check_rkey(qp, pkt); > -- > 2.37.2
On Thu, Dec 08, 2022 at 03:09:46PM -0600, Bob Pearson wrote: > The code in rxe_resp.c at check_length() is incorrect as it > compares pkt->opcode an 8 bit value against various mask bits > which are all higher than 256 so nothing is ever reported. > > This patch rewrites this to compare against pkt->mask which is > correct. However this now exposes another error. For UD send > packets the value of the pmtu cannot be determined from qp->mtu. > All that is required here is to later check if the payload fits > into the posted receive buffer in that case. > > Fixes: 837a55847ead ("RDMA/rxe: Implement packet length validation on responder") > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> > Reviewed-by: Daisuke Matsuda <matsuda-daisuke@fujitsu.com> > --- > drivers/infiniband/sw/rxe/rxe_resp.c | 62 ++++++++++++++++------------ > 1 file changed, 36 insertions(+), 26 deletions(-) Applied to for next, thanks Jason
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c index 6ac544477f3f..42261537772c 100644 --- a/drivers/infiniband/sw/rxe/rxe_resp.c +++ b/drivers/infiniband/sw/rxe/rxe_resp.c @@ -392,36 +392,46 @@ static enum resp_states check_resource(struct rxe_qp *qp, return RESPST_CHK_LENGTH; } -static enum resp_states check_length(struct rxe_qp *qp, - struct rxe_pkt_info *pkt) +static enum resp_states rxe_resp_check_length(struct rxe_qp *qp, + struct rxe_pkt_info *pkt) { - int mtu = qp->mtu; - u32 payload = payload_size(pkt); - u32 dmalen = reth_len(pkt); - - /* RoCEv2 packets do not have LRH. - * Let's skip checking it. + /* + * See IBA C9-92 + * For UD QPs we only check if the packet will fit in the + * receive buffer later. For rmda operations additional + * length checks are performed in check_rkey. */ - - if ((pkt->opcode & RXE_START_MASK) && - (pkt->opcode & RXE_END_MASK)) { - /* "only" packets */ - if (payload > mtu) - return RESPST_ERR_LENGTH; - } else if ((pkt->opcode & RXE_START_MASK) || - (pkt->opcode & RXE_MIDDLE_MASK)) { - /* "first" or "middle" packets */ - if (payload != mtu) - return RESPST_ERR_LENGTH; - } else if (pkt->opcode & RXE_END_MASK) { - /* "last" packets */ - if ((payload == 0) || (payload > mtu)) - return RESPST_ERR_LENGTH; + if (pkt->mask & RXE_PAYLOAD_MASK && ((qp_type(qp) == IB_QPT_RC) || + (qp_type(qp) == IB_QPT_UC))) { + unsigned int mtu = qp->mtu; + unsigned int payload = payload_size(pkt); + + if ((pkt->mask & RXE_START_MASK) && + (pkt->mask & RXE_END_MASK)) { + if (unlikely(payload > mtu)) { + rxe_dbg_qp(qp, "only packet too long"); + return RESPST_ERR_LENGTH; + } + } else if ((pkt->mask & RXE_START_MASK) || + (pkt->mask & RXE_MIDDLE_MASK)) { + if (unlikely(payload != mtu)) { + rxe_dbg_qp(qp, "first or middle packet not mtu"); + return RESPST_ERR_LENGTH; + } + } else if (pkt->mask & RXE_END_MASK) { + if (unlikely((payload == 0) || (payload > mtu))) { + rxe_dbg_qp(qp, "last packet zero or too long"); + return RESPST_ERR_LENGTH; + } + } } - if (pkt->opcode & (RXE_WRITE_MASK | RXE_READ_MASK)) { - if (dmalen > (1 << 31)) + /* See IBA C9-94 */ + if (pkt->mask & RXE_RETH_MASK) { + if (reth_len(pkt) > (1U << 31)) { + rxe_dbg_qp(qp, "dma length too long"); return RESPST_ERR_LENGTH; + } } return RESPST_CHK_RKEY; @@ -1397,7 +1407,7 @@ int rxe_responder(void *arg) state = check_resource(qp, pkt); break; case RESPST_CHK_LENGTH: - state = check_length(qp, pkt); + state = rxe_resp_check_length(qp, pkt); break; case RESPST_CHK_RKEY: state = check_rkey(qp, pkt);
The code in rxe_resp.c at check_length() is incorrect as it compares pkt->opcode an 8 bit value against various mask bits which are all higher than 256 so nothing is ever reported. This patch rewrites this to compare against pkt->mask which is correct. However this now exposes another error. For UD send packets the value of the pmtu cannot be determined from qp->mtu. All that is required here is to later check if the payload fits into the posted receive buffer in that case. Fixes: 837a55847ead ("RDMA/rxe: Implement packet length validation on responder") Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> --- drivers/infiniband/sw/rxe/rxe_resp.c | 62 ++++++++++++++++------------ 1 file changed, 36 insertions(+), 26 deletions(-)