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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM4PR12MB5841EB20B82969B6D67638AF99A49@DM4PR12MB5841.namprd12.prod.outlook.com>
Date:   Wed, 8 Jun 2022 10:29:47 +0000
From:   "Dawar, Gautam" <gautam.dawar@....com>
To:     Jason Wang <jasowang@...hat.com>
CC:     netdev <netdev@...r.kernel.org>,
        "linux-net-drivers (AMD-Xilinx)" <linux-net-drivers@....com>,
        "Anand, Harpreet" <harpreet.anand@....com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Zhu Lingshan <lingshan.zhu@...el.com>,
        Xie Yongji <xieyongji@...edance.com>,
        Eli Cohen <elic@...dia.com>, Parav Pandit <parav@...dia.com>,
        Si-Wei Liu <si-wei.liu@...cle.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Wan Jiabing <wanjiabing@...o.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] vdpa: allow vdpa dev_del management operation to return
 failure

[AMD Official Use Only - General]

Hi Gautam:
[GD>>] Hi Jason,

On Fri, Jun 3, 2022 at 6:34 PM Gautam Dawar <gautam.dawar@....com> wrote:
>
> Currently, the vdpa_nl_cmd_dev_del_set_doit() implementation allows
> returning a value to depict the operation status but the return type
> of dev_del() callback is void. So, any error while deleting the vdpa
> device in the vdpa parent driver can't be returned to the management
> layer.

I wonder under which cognition we can hit an error in dev_del()?
[GD>>] In the AMD-Xilinx vDPA driver, on receiving vdpa device deletion request, I try to identify if the vdpa device is in use by any virtio-net driver (through any vdpa bus driver) by looking at the vdpa device status value. In case the vdpa device status is >= VIRTIO_CONFIG_S_DRIVER, -EBUSY is returned.
This is to avoid side-effects as noted in https://bugzilla.kernel.org/show_bug.cgi?id=213179 caused by deleting the vdpa device when it is being used.
>

Thanks

> This patch changes the return type of dev_del() callback to int to
> allow returning an error code in case of failure.
>
> Signed-off-by: Gautam Dawar <gautam.dawar@....com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_main.c      |  3 ++-
>  drivers/vdpa/mlx5/net/mlx5_vnet.c    |  3 ++-
>  drivers/vdpa/vdpa.c                  | 11 ++++++++---
>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c |  3 ++-
> drivers/vdpa/vdpa_sim/vdpa_sim_net.c |  3 ++-
>  drivers/vdpa/vdpa_user/vduse_dev.c   |  3 ++-
>  include/linux/vdpa.h                 |  5 +++--
>  7 files changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
> b/drivers/vdpa/ifcvf/ifcvf_main.c index 4366320fb68d..6a967935478b
> 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -800,13 +800,14 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>         return ret;
>  }
>
> -static void ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct
> vdpa_device *dev)
> +static int ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct
> +vdpa_device *dev)
>  {
>         struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev;
>
>         ifcvf_mgmt_dev = container_of(mdev, struct ifcvf_vdpa_mgmt_dev, mdev);
>         _vdpa_unregister_device(dev);
>         ifcvf_mgmt_dev->adapter = NULL;
> +       return 0;
>  }
>
>  static const struct vdpa_mgmtdev_ops ifcvf_vdpa_mgmt_dev_ops = { diff
> --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index e0de44000d92..b06204c2f3e8 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2775,7 +2775,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>         return err;
>  }
>
> -static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct
> vdpa_device *dev)
> +static int mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct
> +vdpa_device *dev)
>  {
>         struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
>         struct mlx5_vdpa_dev *mvdev = to_mvdev(dev); @@ -2788,6
> +2788,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *
>         destroy_workqueue(wq);
>         _vdpa_unregister_device(dev);
>         mgtdev->ndev = NULL;
> +       return 0;
>  }
>
>  static const struct vdpa_mgmtdev_ops mdev_ops = { diff --git
> a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> 2b75c00b1005..65dc8bf2f37f 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -363,10 +363,11 @@ static int vdpa_match_remove(struct device *dev,
> void *data)  {
>         struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
>         struct vdpa_mgmt_dev *mdev = vdev->mdev;
> +       int err = 0;
>
>         if (mdev == data)
> -               mdev->ops->dev_del(mdev, vdev);
> -       return 0;
> +               err = mdev->ops->dev_del(mdev, vdev);
> +       return err;
>  }
>
>  void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev) @@ -673,7
> +674,11 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i
>                 goto mdev_err;
>         }
>         mdev = vdev->mdev;
> -       mdev->ops->dev_del(mdev, vdev);
> +       err = mdev->ops->dev_del(mdev, vdev);
> +       if (err) {
> +               NL_SET_ERR_MSG_MOD(info->extack, "ops->dev_del failed");
> +               goto dev_err;
> +       }
>  mdev_err:
>         put_device(dev);
>  dev_err:
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index 42d401d43911..443d4b94268f 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -280,12 +280,13 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>         return ret;
>  }
>
> -static void vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev,
> +static int vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev,
>                                 struct vdpa_device *dev)  {
>         struct vdpasim *simdev = container_of(dev, struct vdpasim,
> vdpa);
>
>         _vdpa_unregister_device(&simdev->vdpa);
> +       return 0;
>  }
>
>  static const struct vdpa_mgmtdev_ops vdpasim_blk_mgmtdev_ops = { diff
> --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> index d5324f6fd8c7..9e5a5ad34e65 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> @@ -167,12 +167,13 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>         return ret;
>  }
>
> -static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev,
> +static int vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev,
>                                 struct vdpa_device *dev)  {
>         struct vdpasim *simdev = container_of(dev, struct vdpasim,
> vdpa);
>
>         _vdpa_unregister_device(&simdev->vdpa);
> +       return 0;
>  }
>
>  static const struct vdpa_mgmtdev_ops vdpasim_net_mgmtdev_ops = { diff
> --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f85d1a08ed87..33ff45e70ff7 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1540,9 +1540,10 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>         return 0;
>  }
>
> -static void vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct
> vdpa_device *dev)
> +static int vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct
> +vdpa_device *dev)
>  {
>         _vdpa_unregister_device(dev);
> +       return 0;
>  }
>
>  static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops = { diff
> --git a/include/linux/vdpa.h b/include/linux/vdpa.h index
> 8943a209202e..e547c9dfdfce 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -443,12 +443,13 @@ void vdpa_set_status(struct vdpa_device *vdev, u8 status);
>   *          @mdev: parent device to use for device removal
>   *          @dev: vdpa device to remove
>   *          Driver need to remove the specified device by calling
> - *          _vdpa_unregister_device().
> + *          _vdpa_unregister_device(). Driver must return 0
> + *          on success or appropriate error code in failure case.
>   */
>  struct vdpa_mgmtdev_ops {
>         int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name,
>                        const struct vdpa_dev_set_config *config);
> -       void (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev);
> +       int (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device
> + *dev);
>  };
>
>  /**
> --
> 2.30.1
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ