On Tue, Oct 01, 2019 at 12:38:17PM -0300, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
>
> mlx5_ib_update_xlt() must be protected against parallel free of the MR it
> is accessing, also it must be called single threaded while updating the
> HW. Otherwise we can have races of the form:
>
> CPU0 CPU1
> mlx5_ib_update_xlt()
> mlx5_odp_populate_klm()
> odp_lookup() == NULL
> pklm = ZAP
> implicit_mr_get_data()
> implicit_mr_alloc()
> <update interval tree>
> mlx5_ib_update_xlt
> mlx5_odp_populate_klm()
> odp_lookup() != NULL
> pklm = VALID
> mlx5_ib_post_send_wait()
>
> mlx5_ib_post_send_wait() // Replaces VALID with ZAP
>
> This can be solved by putting both the SRCU and the umem_mutex lock around
> every call to mlx5_ib_update_xlt(). This ensures that the content of the
> interval tree relavent to mlx5_odp_populate_klm() (ie mr->parent == mr)
> will not change while it is running, and thus the posted WRs to update the
> KLM will always reflect the correct information.
>
> The race above will resolve by either having CPU1 wait till CPU0 completes
> the ZAP or CPU0 will run after the add and instead store VALID.
>
> The pagefault path adding children already holds the umem_mutex and SRCU,
> so the only missed lock is during MR destruction.
>
> Fixes: 81713d3788d2 ("IB/mlx5: Add implicit MR support")
> Reviewed-by: Artemy Kovalyov <artemyko@xxxxxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> ---
> drivers/infiniband/hw/mlx5/odp.c | 34 ++++++++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
> index 2e9b4306179745..3401c06b7e54f5 100644
> --- a/drivers/infiniband/hw/mlx5/odp.c
> +++ b/drivers/infiniband/hw/mlx5/odp.c
> @@ -178,6 +178,29 @@ void mlx5_odp_populate_klm(struct mlx5_klm *pklm, size_t offset,
> return;
> }
>
> + /*
> + * The locking here is pretty subtle. Ideally the implicit children
> + * list would be protected by the umem_mutex, however that is not
> + * possible. Instead this uses a weaker update-then-lock pattern:
> + *
> + * srcu_read_lock()
> + * <change children list>
> + * mutex_lock(umem_mutex)
> + * mlx5_ib_update_xlt()
> + * mutex_unlock(umem_mutex)
> + * destroy lkey
> + *
> + * ie any change the children list must be followed by the locked
> + * update_xlt before destroying.
> + *
> + * The umem_mutex provides the acquire/release semantic needed to make
> + * the children list visible to a racing thread. While SRCU is not
> + * technically required, using it gives consistent use of the SRCU
> + * locking around the children list.
> + */
> + lockdep_assert_held(&to_ib_umem_odp(mr->umem)->umem_mutex);
> + lockdep_assert_held(&mr->dev->mr_srcu);
> +
> odp = odp_lookup(offset * MLX5_IMR_MTT_SIZE,
> nentries * MLX5_IMR_MTT_SIZE, mr);
>
> @@ -202,15 +225,22 @@ static void mr_leaf_free_action(struct work_struct *work)
> struct ib_umem_odp *odp = container_of(work, struct ib_umem_odp, work);
> int idx = ib_umem_start(odp) >> MLX5_IMR_MTT_SHIFT;
> struct mlx5_ib_mr *mr = odp->private, *imr = mr->parent;
> + struct ib_umem_odp *odp_imr = to_ib_umem_odp(imr->umem);
> + int srcu_key;
>
> mr->parent = NULL;
> synchronize_srcu(&mr->dev->mr_srcu);
Are you sure that this line is still needed?
>
> - ib_umem_odp_release(odp);
> - if (imr->live)
> + if (imr->live) {
> + srcu_key = srcu_read_lock(&mr->dev->mr_srcu);
> + mutex_lock(&odp_imr->umem_mutex);
> mlx5_ib_update_xlt(imr, idx, 1, 0,
> MLX5_IB_UPD_XLT_INDIRECT |
> MLX5_IB_UPD_XLT_ATOMIC);
> + mutex_unlock(&odp_imr->umem_mutex);
> + srcu_read_unlock(&mr->dev->mr_srcu, srcu_key);
> + }
> + ib_umem_odp_release(odp);
> mlx5_mr_cache_free(mr->dev, mr);
>
> if (atomic_dec_and_test(&imr->num_leaf_free))
> --
> 2.23.0
>
