> On 22 Jun 2021, at 15:20, Håkon Bugge <haakon.bugge@xxxxxxxxxx> wrote:
>
> The struct rdma_id_private contains three bit-fields, tos_set,
> timeout_set, and min_rnr_timer_set. These are set by accessor
> functions without any synchronization. If two or all accessor
> functions are invoked in close proximity in time, there will be
> Read-Modify-Write from several contexts to the same variable, and the
> result will be intermittent.
>
> Fixed by protecting the bit-fields by the qp_mutex in the accessor
> functions.
>
> The consumer of timeout_set and min_rnr_timer_set is in
> rdma_init_qp_attr(), which is called with qp_mutex held for connected
> QPs. Explicit locking is added for the consumers of tos and tos_set.
>
> This commit depends on ("RDMA/cma: Remove unnecessary INIT->INIT
> transition"), since the call to rdma_init_qp_attr() from
> cma_init_conn_qp() does not hold the qp_mutex.
>
> Fixes: 2c1619edef61 ("IB/cma: Define option to set ack timeout and pack tos_set")
> Fixes: 3aeffc46afde ("IB/cma: Introduce rdma_set_min_rnr_timer()")
> Signed-off-by: Håkon Bugge <haakon.bugge@xxxxxxxxxx>
Sorry, not my day. Overlooked the access to timeout_set/timeout in cma_resolve_iboe_route().
Have to send a v2.
Håkon
> ---
> drivers/infiniband/core/cma.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index e3f52c5..6b41527 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -2457,8 +2457,10 @@ static int cma_iw_listen(struct rdma_id_private *id_priv, int backlog)
> if (IS_ERR(id))
> return PTR_ERR(id);
>
> + mutex_lock(&id_priv->qp_mutex);
> id->tos = id_priv->tos;
> id->tos_set = id_priv->tos_set;
> + mutex_unlock(&id_priv->qp_mutex);
> id->afonly = id_priv->afonly;
> id_priv->cm_id.iw = id;
>
> @@ -2519,8 +2521,10 @@ static int cma_listen_on_dev(struct rdma_id_private *id_priv,
> cma_id_get(id_priv);
> dev_id_priv->internal_id = 1;
> dev_id_priv->afonly = id_priv->afonly;
> + mutex_lock(&id_priv->qp_mutex);
> dev_id_priv->tos_set = id_priv->tos_set;
> dev_id_priv->tos = id_priv->tos;
> + mutex_unlock(&id_priv->qp_mutex);
>
> ret = rdma_listen(&dev_id_priv->id, id_priv->backlog);
> if (ret)
> @@ -2567,8 +2571,10 @@ void rdma_set_service_type(struct rdma_cm_id *id, int tos)
> struct rdma_id_private *id_priv;
>
> id_priv = container_of(id, struct rdma_id_private, id);
> + mutex_lock(&id_priv->qp_mutex);
> id_priv->tos = (u8) tos;
> id_priv->tos_set = true;
> + mutex_unlock(&id_priv->qp_mutex);
> }
> EXPORT_SYMBOL(rdma_set_service_type);
>
> @@ -2595,8 +2601,10 @@ int rdma_set_ack_timeout(struct rdma_cm_id *id, u8 timeout)
> return -EINVAL;
>
> id_priv = container_of(id, struct rdma_id_private, id);
> + mutex_lock(&id_priv->qp_mutex);
> id_priv->timeout = timeout;
> id_priv->timeout_set = true;
> + mutex_unlock(&id_priv->qp_mutex);
>
> return 0;
> }
> @@ -2632,8 +2640,10 @@ int rdma_set_min_rnr_timer(struct rdma_cm_id *id, u8 min_rnr_timer)
> return -EINVAL;
>
> id_priv = container_of(id, struct rdma_id_private, id);
> + mutex_lock(&id_priv->qp_mutex);
> id_priv->min_rnr_timer = min_rnr_timer;
> id_priv->min_rnr_timer_set = true;
> + mutex_unlock(&id_priv->qp_mutex);
>
> return 0;
> }
> @@ -3019,8 +3029,11 @@ static int cma_resolve_iboe_route(struct rdma_id_private *id_priv)
>
> u8 default_roce_tos = id_priv->cma_dev->default_roce_tos[id_priv->id.port_num -
> rdma_start_port(id_priv->cma_dev->device)];
> - u8 tos = id_priv->tos_set ? id_priv->tos : default_roce_tos;
> + u8 tos;
>
> + mutex_lock(&id_priv->qp_mutex);
> + tos = id_priv->tos_set ? id_priv->tos : default_roce_tos;
> + mutex_unlock(&id_priv->qp_mutex);
>
> work = kzalloc(sizeof *work, GFP_KERNEL);
> if (!work)
> @@ -4092,8 +4105,11 @@ static int cma_connect_iw(struct rdma_id_private *id_priv,
> if (IS_ERR(cm_id))
> return PTR_ERR(cm_id);
>
> + mutex_lock(&id_priv->qp_mutex);
> cm_id->tos = id_priv->tos;
> cm_id->tos_set = id_priv->tos_set;
> + mutex_unlock(&id_priv->qp_mutex);
> +
> id_priv->cm_id.iw = cm_id;
>
> memcpy(&cm_id->local_addr, cma_src_addr(id_priv),
> --
> 1.8.3.1
>
