On Fri, Jan 11, 2019 at 07:31:24PM -0700, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
>
> Work must hold a kref on the ib_device otherwise the dev pointer can
> become free before the work runs.
How can it occur exactly?
"dev" will be released after mlx5_ib will exist, mlx5_ib exist will
drain advise_mr queue before it. Such drain will ensure that no works
are running.
>
> Remove the bogus use of 'reg_state':
> - While in uverbs the reg_state is guaranteed to always be
> REGISTERED
> - Testing reg_state with no locking is bogus. Use ib_device_try_get()
> to get back into a region that prevents unregistration.
>
> Fixes: 813e90b1aeaa ("IB/mlx5: Add advise_mr() support")
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> ---
> drivers/infiniband/hw/mlx5/odp.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
> index 8d46b1dc56580f..a241a65834d4ca 100644
> --- a/drivers/infiniband/hw/mlx5/odp.c
> +++ b/drivers/infiniband/hw/mlx5/odp.c
> @@ -1595,10 +1595,12 @@ static void mlx5_ib_prefetch_mr_work(struct work_struct *work)
> struct prefetch_mr_work *w =
> container_of(work, struct prefetch_mr_work, work);
>
> - if (w->dev->ib_dev.reg_state == IB_DEV_REGISTERED)
> + if (ib_device_try_get(&w->dev->ib_dev)) {
> mlx5_ib_prefetch_sg_list(w->dev, w->pf_flags, w->sg_list,
> w->num_sge);
> -
> + ib_device_put(&w->dev->ib_dev);
> + }
> + put_device(&w->dev->ib_dev.dev);
> kfree(w);
> }
>
> @@ -1617,15 +1619,13 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd,
> return mlx5_ib_prefetch_sg_list(dev, pf_flags, sg_list,
> num_sge);
>
> - if (dev->ib_dev.reg_state != IB_DEV_REGISTERED)
> - return -ENODEV;
> -
> work = kvzalloc(struct_size(work, sg_list, num_sge), GFP_KERNEL);
> if (!work)
> return -ENOMEM;
>
> memcpy(work->sg_list, sg_list, num_sge * sizeof(struct ib_sge));
>
> + get_device(&dev->ib_dev.dev);
It looks extremely wrong to sprinkle driver code with general
get_device/put_device.
> work->dev = dev;
> work->pf_flags = pf_flags;
> work->num_sge = num_sge;
> --
> 2.20.1
>
Attachment:
signature.asc
Description: PGP signature
