On Fri, Mar 13, 2020 at 08:47:05AM -0400, Mike Marciniszyn wrote:
> The following modify sequence (loosely based on ipoib) will
> lose a pkey modifcation:
>
> - Modify (pkey index, port)
> - Modify (new pkey index, NO port)
>
> After the first modify, the qp_pps list will have saved the pkey and the
> unit on the main list.
>
> During the second modify, get_new_pps() will fetch the port from qp_pps
> and read the new pkey index from qp_attr->pkey_index. The state will
> still be zero, or IB_PORT_PKEY_NOT_VALID. Because of the invalid state,
> the new values will never replace the one in the qp pps list, losing
> the new pkey.
>
> This happens because the following if statements will never correct the
> state because the first term will be false. If the code had been executed,
> it would incorrectly overwrite valid values.
>
> if ((qp_attr_mask & IB_QP_PKEY_INDEX) && (qp_attr_mask & IB_QP_PORT))
> new_pps->main.state = IB_PORT_PKEY_VALID;
>
>
> if (!(qp_attr_mask & (IB_QP_PKEY_INDEX | IB_QP_PORT)) && qp_pps) {
> new_pps->main.port_num = qp_pps->main.port_num;
> new_pps->main.pkey_index = qp_pps->main.pkey_index;
> if (qp_pps->main.state != IB_PORT_PKEY_NOT_VALID)
> new_pps->main.state = IB_PORT_PKEY_VALID;
> }
>
> Fix by joining the two if statements with an or test to see if qp_pps
> is non-NULL and in the correct state.
>
> Fixes: 1dd017882e01 ("RDMA/core: Fix protection fault in get_pkey_idx_qp_list")
> Reviewed-by: Kaike Wan <kaike.wan@xxxxxxxxx>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx>
Leon? Maor?
> diff --git a/drivers/infiniband/core/security.c b/drivers/infiniband/core/security.c
> index 2d56083..75e7ec0 100644
> +++ b/drivers/infiniband/core/security.c
> @@ -349,16 +349,11 @@ static struct ib_ports_pkeys *get_new_pps(const struct ib_qp *qp,
> else if (qp_pps)
> new_pps->main.pkey_index = qp_pps->main.pkey_index;
>
> - if ((qp_attr_mask & IB_QP_PKEY_INDEX) && (qp_attr_mask & IB_QP_PORT))
> + if (((qp_attr_mask & IB_QP_PKEY_INDEX) &&
> + (qp_attr_mask & IB_QP_PORT)) ||
> + (qp_pps && qp_pps->main.state != IB_PORT_PKEY_NOT_VALID))
> new_pps->main.state = IB_PORT_PKEY_VALID;
>
> - if (!(qp_attr_mask & (IB_QP_PKEY_INDEX | IB_QP_PORT)) && qp_pps) {
> - new_pps->main.port_num = qp_pps->main.port_num;
> - new_pps->main.pkey_index = qp_pps->main.pkey_index;
> - if (qp_pps->main.state != IB_PORT_PKEY_NOT_VALID)
> - new_pps->main.state = IB_PORT_PKEY_VALID;
> - }
> -
> if (qp_attr_mask & IB_QP_ALT_PATH) {
> new_pps->alt.port_num = qp_attr->alt_port_num;
> new_pps->alt.pkey_index = qp_attr->alt_pkey_index;
>
