linux-kernel - Re: [PATCH 6/7] vdpa/mlx5: Avoid using reslock in event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACGkMEt+7kKD8_q_OFKURbFR1W=YbJpcuwZq5bf5jC4qzE8PEA@mail.gmail.com>
Date:   Mon, 14 Nov 2022 15:53:09 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Eli Cohen <elic@...dia.com>
Cc:     mst@...hat.com, linux-kernel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org, si-wei.liu@...cle.com,
        eperezma@...hat.com, lulu@...hat.com
Subject: Re: [PATCH 6/7] vdpa/mlx5: Avoid using reslock in event_handler

On Sun, Nov 13, 2022 at 9:45 PM Eli Cohen <elic@...dia.com> wrote:
>
> event_handler runs under atomic context and may not acquire reslock. We
> can still guarantee that the handler won't be called after suspend by
> clearing nb_registered, unregistering the handler and flushing the
> workqueue.
>
> Signed-off-by: Eli Cohen <elic@...dia.com>
> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 6e6490c85be2..bebfba530247 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2872,8 +2872,8 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
>         int i;
>
>         down_write(&ndev->reslock);
> -       mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
>         ndev->nb_registered = false;
> +       mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);

I wonder why this can help anything. And if it does, we have simliar
logic in mlx5_vdpa_dev_del() do we need to fix that as well?

Thanks

>         flush_workqueue(ndev->mvdev.wq);
>         for (i = 0; i < ndev->cur_num_vqs; i++) {
>                 mvq = &ndev->vqs[i];
> @@ -3051,7 +3051,7 @@ static void update_carrier(struct work_struct *work)
>         else
>                 ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
>
> -       if (ndev->config_cb.callback)
> +       if (ndev->nb_registered && ndev->config_cb.callback)
>                 ndev->config_cb.callback(ndev->config_cb.private);
>
>         kfree(wqent);
> @@ -3068,21 +3068,13 @@ static int event_handler(struct notifier_block *nb, unsigned long event, void *p
>                 switch (eqe->sub_type) {
>                 case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
>                 case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> -                       down_read(&ndev->reslock);
> -                       if (!ndev->nb_registered) {
> -                               up_read(&ndev->reslock);
> -                               return NOTIFY_DONE;
> -                       }
>                         wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> -                       if (!wqent) {
> -                               up_read(&ndev->reslock);
> +                       if (!wqent)
>                                 return NOTIFY_DONE;
> -                       }
>
>                         wqent->mvdev = &ndev->mvdev;
>                         INIT_WORK(&wqent->work, update_carrier);
>                         queue_work(ndev->mvdev.wq, &wqent->work);
> -                       up_read(&ndev->reslock);
>                         ret = NOTIFY_OK;
>                         break;
>                 default:
> --
> 2.38.1
>