| Message ID | 20230401144429.88673-4-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce new methods for verifying ownership in vfio PCI hot reset | expand |
Hi Yi, On 4/1/23 16:44, Yi Liu wrote: > This prepares to add another method for hot reset. The major hot reset logic > are moved to vfio_pci_ioctl_pci_hot_reset_groups(). > > No functional change is intended. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Tested-by: Yanting Jiang <yanting.jiang@intel.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/vfio/pci/vfio_pci_core.c | 56 +++++++++++++++++++------------- > 1 file changed, 33 insertions(+), 23 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index 5d745c9abf05..3696b8e58445 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -1255,29 +1255,17 @@ static int vfio_pci_ioctl_get_pci_hot_reset_info( > return ret; > } > > -static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, > - struct vfio_pci_hot_reset __user *arg) > +static int > +vfio_pci_ioctl_pci_hot_reset_groups(struct vfio_pci_core_device *vdev, > + struct vfio_pci_hot_reset *hdr, nit why don't you simply pass the user group count as decoded earlier. hdr sounds like a dup of arg. > + bool slot, > + struct vfio_pci_hot_reset __user *arg) > { > - unsigned long minsz = offsetofend(struct vfio_pci_hot_reset, count); > - struct vfio_pci_hot_reset hdr; > int32_t *group_fds; > struct file **files; > struct vfio_pci_group_info info; > - bool slot = false; > int file_idx, count = 0, ret = 0; > > - if (copy_from_user(&hdr, arg, minsz)) > - return -EFAULT; > - > - if (hdr.argsz < minsz || hdr.flags) > - return -EINVAL; > - > - /* Can we do a slot or bus reset or neither? */ > - if (!pci_probe_reset_slot(vdev->pdev->slot)) > - slot = true; > - else if (pci_probe_reset_bus(vdev->pdev->bus)) > - return -ENODEV; > - > /* > * We can't let userspace give us an arbitrarily large buffer to copy, > * so verify how many we think there could be. Note groups can have > @@ -1289,11 +1277,11 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, > return ret; > > /* Somewhere between 1 and count is OK */ > - if (!hdr.count || hdr.count > count) > + if (!hdr->count || hdr->count > count) > return -EINVAL; > > - group_fds = kcalloc(hdr.count, sizeof(*group_fds), GFP_KERNEL); > - files = kcalloc(hdr.count, sizeof(*files), GFP_KERNEL); > + group_fds = kcalloc(hdr->count, sizeof(*group_fds), GFP_KERNEL); > + files = kcalloc(hdr->count, sizeof(*files), GFP_KERNEL); > if (!group_fds || !files) { > kfree(group_fds); > kfree(files); > @@ -1301,7 +1289,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, > } > > if (copy_from_user(group_fds, arg->group_fds, > - hdr.count * sizeof(*group_fds))) { > + hdr->count * sizeof(*group_fds))) { > kfree(group_fds); > kfree(files); > return -EFAULT; > @@ -1311,7 +1299,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, > * Get the group file for each fd to ensure the group held across > * the reset > */ > - for (file_idx = 0; file_idx < hdr.count; file_idx++) { > + for (file_idx = 0; file_idx < hdr->count; file_idx++) { > struct file *file = fget(group_fds[file_idx]); > > if (!file) { > @@ -1335,7 +1323,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, > if (ret) > goto hot_reset_release; > > - info.count = hdr.count; > + info.count = hdr->count; > info.files = files; > > ret = vfio_pci_dev_set_hot_reset(vdev->vdev.dev_set, &info); > @@ -1348,6 +1336,28 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, > return ret; > } > > +static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, > + struct vfio_pci_hot_reset __user *arg) > +{ > + unsigned long minsz = offsetofend(struct vfio_pci_hot_reset, count); > + struct vfio_pci_hot_reset hdr; > + bool slot = false; > + > + if (copy_from_user(&hdr, arg, minsz)) > + return -EFAULT; > + > + if (hdr.argsz < minsz || hdr.flags) > + return -EINVAL; > + > + /* Can we do a slot or bus reset or neither? */ > + if (!pci_probe_reset_slot(vdev->pdev->slot)) > + slot = true; > + else if (pci_probe_reset_bus(vdev->pdev->bus)) > + return -ENODEV; > + > + return vfio_pci_ioctl_pci_hot_reset_groups(vdev, &hdr, slot, arg); > +} > + > static int vfio_pci_ioctl_ioeventfd(struct vfio_pci_core_device *vdev, > struct vfio_device_ioeventfd __user *arg) > { Besides Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric
> From: Eric Auger <eric.auger@redhat.com> > Sent: Tuesday, April 4, 2023 10:00 PM > > Hi Yi, > > On 4/1/23 16:44, Yi Liu wrote: > > This prepares to add another method for hot reset. The major hot reset logic > > are moved to vfio_pci_ioctl_pci_hot_reset_groups(). > > > > No functional change is intended. > > > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > Tested-by: Yanting Jiang <yanting.jiang@intel.com> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > --- > > drivers/vfio/pci/vfio_pci_core.c | 56 +++++++++++++++++++------------- > > 1 file changed, 33 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > > index 5d745c9abf05..3696b8e58445 100644 > > --- a/drivers/vfio/pci/vfio_pci_core.c > > +++ b/drivers/vfio/pci/vfio_pci_core.c > > @@ -1255,29 +1255,17 @@ static int vfio_pci_ioctl_get_pci_hot_reset_info( > > return ret; > > } > > > > -static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, > > - struct vfio_pci_hot_reset __user *arg) > > +static int > > +vfio_pci_ioctl_pci_hot_reset_groups(struct vfio_pci_core_device *vdev, > > + struct vfio_pci_hot_reset *hdr, > nit why don't you simply pass the user group count as decoded earlier. > hdr sounds like a dup of arg. indeed. only hdr->count is needed. > > + bool slot, > > + struct vfio_pci_hot_reset __user *arg) > > { > > - unsigned long minsz = offsetofend(struct vfio_pci_hot_reset, count); > > - struct vfio_pci_hot_reset hdr; > > int32_t *group_fds; > > struct file **files; > > struct vfio_pci_group_info info; > > - bool slot = false; > > int file_idx, count = 0, ret = 0; > > > > - if (copy_from_user(&hdr, arg, minsz)) > > - return -EFAULT; > > - > > - if (hdr.argsz < minsz || hdr.flags) > > - return -EINVAL; > > - > > - /* Can we do a slot or bus reset or neither? */ > > - if (!pci_probe_reset_slot(vdev->pdev->slot)) > > - slot = true; > > - else if (pci_probe_reset_bus(vdev->pdev->bus)) > > - return -ENODEV; > > - > > /* > > * We can't let userspace give us an arbitrarily large buffer to copy, > > * so verify how many we think there could be. Note groups can have > > @@ -1289,11 +1277,11 @@ static int vfio_pci_ioctl_pci_hot_reset(struct > vfio_pci_core_device *vdev, > > return ret; > > > > /* Somewhere between 1 and count is OK */ > > - if (!hdr.count || hdr.count > count) > > + if (!hdr->count || hdr->count > count) > > return -EINVAL; > > > > - group_fds = kcalloc(hdr.count, sizeof(*group_fds), GFP_KERNEL); > > - files = kcalloc(hdr.count, sizeof(*files), GFP_KERNEL); > > + group_fds = kcalloc(hdr->count, sizeof(*group_fds), GFP_KERNEL); > > + files = kcalloc(hdr->count, sizeof(*files), GFP_KERNEL); > > if (!group_fds || !files) { > > kfree(group_fds); > > kfree(files); > > @@ -1301,7 +1289,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct > vfio_pci_core_device *vdev, > > } > > > > if (copy_from_user(group_fds, arg->group_fds, > > - hdr.count * sizeof(*group_fds))) { > > + hdr->count * sizeof(*group_fds))) { > > kfree(group_fds); > > kfree(files); > > return -EFAULT; > > @@ -1311,7 +1299,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct > vfio_pci_core_device *vdev, > > * Get the group file for each fd to ensure the group held across > > * the reset > > */ > > - for (file_idx = 0; file_idx < hdr.count; file_idx++) { > > + for (file_idx = 0; file_idx < hdr->count; file_idx++) { > > struct file *file = fget(group_fds[file_idx]); > > > > if (!file) { > > @@ -1335,7 +1323,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct > vfio_pci_core_device *vdev, > > if (ret) > > goto hot_reset_release; > > > > - info.count = hdr.count; > > + info.count = hdr->count; > > info.files = files; > > > > ret = vfio_pci_dev_set_hot_reset(vdev->vdev.dev_set, &info); > > @@ -1348,6 +1336,28 @@ static int vfio_pci_ioctl_pci_hot_reset(struct > vfio_pci_core_device *vdev, > > return ret; > > } > > > > +static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, > > + struct vfio_pci_hot_reset __user *arg) > > +{ > > + unsigned long minsz = offsetofend(struct vfio_pci_hot_reset, count); > > + struct vfio_pci_hot_reset hdr; > > + bool slot = false; > > + > > + if (copy_from_user(&hdr, arg, minsz)) > > + return -EFAULT; > > + > > + if (hdr.argsz < minsz || hdr.flags) > > + return -EINVAL; > > + > > + /* Can we do a slot or bus reset or neither? */ > > + if (!pci_probe_reset_slot(vdev->pdev->slot)) > > + slot = true; > > + else if (pci_probe_reset_bus(vdev->pdev->bus)) > > + return -ENODEV; > > + > > + return vfio_pci_ioctl_pci_hot_reset_groups(vdev, &hdr, slot, arg); > > +} > > + > > static int vfio_pci_ioctl_ioeventfd(struct vfio_pci_core_device *vdev, > > struct vfio_device_ioeventfd __user *arg) > > { > Besides > Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks, Yi Liu
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 5d745c9abf05..3696b8e58445 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -1255,29 +1255,17 @@ static int vfio_pci_ioctl_get_pci_hot_reset_info( return ret; } -static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, - struct vfio_pci_hot_reset __user *arg) +static int +vfio_pci_ioctl_pci_hot_reset_groups(struct vfio_pci_core_device *vdev, + struct vfio_pci_hot_reset *hdr, + bool slot, + struct vfio_pci_hot_reset __user *arg) { - unsigned long minsz = offsetofend(struct vfio_pci_hot_reset, count); - struct vfio_pci_hot_reset hdr; int32_t *group_fds; struct file **files; struct vfio_pci_group_info info; - bool slot = false; int file_idx, count = 0, ret = 0; - if (copy_from_user(&hdr, arg, minsz)) - return -EFAULT; - - if (hdr.argsz < minsz || hdr.flags) - return -EINVAL; - - /* Can we do a slot or bus reset or neither? */ - if (!pci_probe_reset_slot(vdev->pdev->slot)) - slot = true; - else if (pci_probe_reset_bus(vdev->pdev->bus)) - return -ENODEV; - /* * We can't let userspace give us an arbitrarily large buffer to copy, * so verify how many we think there could be. Note groups can have @@ -1289,11 +1277,11 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, return ret; /* Somewhere between 1 and count is OK */ - if (!hdr.count || hdr.count > count) + if (!hdr->count || hdr->count > count) return -EINVAL; - group_fds = kcalloc(hdr.count, sizeof(*group_fds), GFP_KERNEL); - files = kcalloc(hdr.count, sizeof(*files), GFP_KERNEL); + group_fds = kcalloc(hdr->count, sizeof(*group_fds), GFP_KERNEL); + files = kcalloc(hdr->count, sizeof(*files), GFP_KERNEL); if (!group_fds || !files) { kfree(group_fds); kfree(files); @@ -1301,7 +1289,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, } if (copy_from_user(group_fds, arg->group_fds, - hdr.count * sizeof(*group_fds))) { + hdr->count * sizeof(*group_fds))) { kfree(group_fds); kfree(files); return -EFAULT; @@ -1311,7 +1299,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, * Get the group file for each fd to ensure the group held across * the reset */ - for (file_idx = 0; file_idx < hdr.count; file_idx++) { + for (file_idx = 0; file_idx < hdr->count; file_idx++) { struct file *file = fget(group_fds[file_idx]); if (!file) { @@ -1335,7 +1323,7 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, if (ret) goto hot_reset_release; - info.count = hdr.count; + info.count = hdr->count; info.files = files; ret = vfio_pci_dev_set_hot_reset(vdev->vdev.dev_set, &info); @@ -1348,6 +1336,28 @@ static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, return ret; } +static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev, + struct vfio_pci_hot_reset __user *arg) +{ + unsigned long minsz = offsetofend(struct vfio_pci_hot_reset, count); + struct vfio_pci_hot_reset hdr; + bool slot = false; + + if (copy_from_user(&hdr, arg, minsz)) + return -EFAULT; + + if (hdr.argsz < minsz || hdr.flags) + return -EINVAL; + + /* Can we do a slot or bus reset or neither? */ + if (!pci_probe_reset_slot(vdev->pdev->slot)) + slot = true; + else if (pci_probe_reset_bus(vdev->pdev->bus)) + return -ENODEV; + + return vfio_pci_ioctl_pci_hot_reset_groups(vdev, &hdr, slot, arg); +} + static int vfio_pci_ioctl_ioeventfd(struct vfio_pci_core_device *vdev, struct vfio_device_ioeventfd __user *arg) {