| Message ID | 20230509215923.3186420-2-alex.williamson@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,1/3] vfio/pci: add support for VF token | expand |
On 5/9/23 5:59 PM, Alex Williamson wrote: > From: Minwoo Im <minwoo.im@samsung.com> > > VF token was introduced [1] to kernel vfio-pci along with SR-IOV > support [2]. This patch adds support VF token among PF and VF(s). To > passthu PCIe VF to a VM, kernel >= v5.7 needs this. > > It can be configured with UUID like: > > -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,... > > [1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/ > [2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/ > > Cc: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Minwoo Im <minwoo.im@samsung.com> > Reviewed-by: Klaus Jensen <k.jensen@samsung.com> > Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4 > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Hi Minwoo, Alex, I'm seeing a regression in vfio-pci on s390 and bisect points to this commit. I don't believe it's specific to s390 though, but rather when not using this new vf-token, see below... > --- > hw/vfio/pci.c | 13 ++++++++++++- > hw/vfio/pci.h | 1 + > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index ec9a854361ac..cf27f28936cb 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > int groupid; > int i, ret; > bool is_mdev; > + char uuid[UUID_FMT_LEN]; > + char *name; > > if (!vbasedev->sysfsdev) { > if (!(~vdev->host.domain || ~vdev->host.bus || > @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > goto error; > } > > - ret = vfio_get_device(group, vbasedev->name, vbasedev, errp); > + if (!qemu_uuid_is_null(&vdev->vf_token)) { > + qemu_uuid_unparse(&vdev->vf_token, uuid); > + name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid); > + } else { > + name = vbasedev->name; ^ here we copy the pointer when a vf-token was not specified. > + } > + > + ret = vfio_get_device(group, name, vbasedev, errp); > + g_free(name); ^ and then free it regardless. But I don't think we meant to free what vbasedev->name points to, this was meant to free a duplicate string. I'm subsequently seeing qemu crashes later on e.g. during device unplug. I think doing a strdup in either case would fix the issue OR skipping the g_free when qemu_uuid_is_null(&vdev->vf_token). FWIW, I tried the following and it resolved the issue for me: diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index bf27a39905..73874a94de 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2994,7 +2994,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) qemu_uuid_unparse(&vdev->vf_token, uuid); name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid); } else { - name = vbasedev->name; + name = g_strdup(vbasedev->name); } ret = vfio_get_device(group, name, vbasedev, errp);
Hello Matthew, On 5/23/23 18:51, Matthew Rosato wrote: > On 5/9/23 5:59 PM, Alex Williamson wrote: >> From: Minwoo Im <minwoo.im@samsung.com> >> >> VF token was introduced [1] to kernel vfio-pci along with SR-IOV >> support [2]. This patch adds support VF token among PF and VF(s). To >> passthu PCIe VF to a VM, kernel >= v5.7 needs this. >> >> It can be configured with UUID like: >> >> -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,... >> >> [1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/ >> [2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/ >> >> Cc: Alex Williamson <alex.williamson@redhat.com> >> Signed-off-by: Minwoo Im <minwoo.im@samsung.com> >> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> >> Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4 >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > Hi Minwoo, Alex, > > I'm seeing a regression in vfio-pci on s390 and bisect points to this commit. I don't believe it's specific to s390 though, but rather when not using this new vf-token, see below... > >> --- >> hw/vfio/pci.c | 13 ++++++++++++- >> hw/vfio/pci.h | 1 + >> 2 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index ec9a854361ac..cf27f28936cb 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> int groupid; >> int i, ret; >> bool is_mdev; >> + char uuid[UUID_FMT_LEN]; >> + char *name; >> >> if (!vbasedev->sysfsdev) { >> if (!(~vdev->host.domain || ~vdev->host.bus || >> @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> goto error; >> } >> >> - ret = vfio_get_device(group, vbasedev->name, vbasedev, errp); >> + if (!qemu_uuid_is_null(&vdev->vf_token)) { >> + qemu_uuid_unparse(&vdev->vf_token, uuid); >> + name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid); >> + } else { >> + name = vbasedev->name; > > ^ here we copy the pointer when a vf-token was not specified. > >> + } >> + >> + ret = vfio_get_device(group, name, vbasedev, errp); >> + g_free(name); > > ^ and then free it regardless. But I don't think we meant to free what vbasedev->name points to, this was meant to free a duplicate string. I'm subsequently seeing qemu crashes later on e.g. during device unplug. > > I think doing a strdup in either case would fix the issue OR skipping the g_free when qemu_uuid_is_null(&vdev->vf_token). > > FWIW, I tried the following and it resolved the issue for me: Zhenzhong provided the exact same fix : https://lore.kernel.org/qemu-devel/20230517024651.82248-1-zhenzhong.duan@intel.com/ Thanks, C. > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index bf27a39905..73874a94de 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2994,7 +2994,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > qemu_uuid_unparse(&vdev->vf_token, uuid); > name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid); > } else { > - name = vbasedev->name; > + name = g_strdup(vbasedev->name); > } > > ret = vfio_get_device(group, name, vbasedev, errp); > > > >
On Tue, 9 May 2023 at 23:01, Alex Williamson <alex.williamson@redhat.com> wrote: > > From: Minwoo Im <minwoo.im@samsung.com> > > VF token was introduced [1] to kernel vfio-pci along with SR-IOV > support [2]. This patch adds support VF token among PF and VF(s). To > passthu PCIe VF to a VM, kernel >= v5.7 needs this. > > It can be configured with UUID like: > > -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,... > > [1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/ > [2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/ > > Cc: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Minwoo Im <minwoo.im@samsung.com> > Reviewed-by: Klaus Jensen <k.jensen@samsung.com> > Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4 > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Hi; Coverity points out that this change introduces a buffer overrun (CID 1522913). I dunno why it's taken it so long to notice... > --- > hw/vfio/pci.c | 13 ++++++++++++- > hw/vfio/pci.h | 1 + > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index ec9a854361ac..cf27f28936cb 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > int groupid; > int i, ret; > bool is_mdev; > + char uuid[UUID_FMT_LEN]; We define the array uuid[] as UUID_FMT_LEN bytes long... > + char *name; > > if (!vbasedev->sysfsdev) { > if (!(~vdev->host.domain || ~vdev->host.bus || > @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > goto error; > } > > - ret = vfio_get_device(group, vbasedev->name, vbasedev, errp); > + if (!qemu_uuid_is_null(&vdev->vf_token)) { > + qemu_uuid_unparse(&vdev->vf_token, uuid); ...but qemu_uuid_unparse() writes UUID_FMT_LEN + 1 bytes, including a trailing NUL. Every other use of UUID_FMT_LEN to declare an array uses "UUID_FMT_LEN + 1" to avoid this. (In fact, every use of UUID_FMT_LEN at all uses "+ 1", which suggests that perhaps defining it differently (and perhaps with a different name) would reduce the risk of this particular bug...) thanks -- PMM
On 10/20/23 15:32, Peter Maydell wrote: > On Tue, 9 May 2023 at 23:01, Alex Williamson <alex.williamson@redhat.com> wrote: >> >> From: Minwoo Im <minwoo.im@samsung.com> >> >> VF token was introduced [1] to kernel vfio-pci along with SR-IOV >> support [2]. This patch adds support VF token among PF and VF(s). To >> passthu PCIe VF to a VM, kernel >= v5.7 needs this. >> >> It can be configured with UUID like: >> >> -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,... >> >> [1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/ >> [2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/ >> >> Cc: Alex Williamson <alex.williamson@redhat.com> >> Signed-off-by: Minwoo Im <minwoo.im@samsung.com> >> Reviewed-by: Klaus Jensen <k.jensen@samsung.com> >> Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4 >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > Hi; Coverity points out that this change introduces a buffer > overrun (CID 1522913). I dunno why it's taken it so long > to notice... > >> --- >> hw/vfio/pci.c | 13 ++++++++++++- >> hw/vfio/pci.h | 1 + >> 2 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index ec9a854361ac..cf27f28936cb 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> int groupid; >> int i, ret; >> bool is_mdev; >> + char uuid[UUID_FMT_LEN]; > > We define the array uuid[] as UUID_FMT_LEN bytes long... > >> + char *name; >> >> if (!vbasedev->sysfsdev) { >> if (!(~vdev->host.domain || ~vdev->host.bus || >> @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> goto error; >> } >> >> - ret = vfio_get_device(group, vbasedev->name, vbasedev, errp); >> + if (!qemu_uuid_is_null(&vdev->vf_token)) { >> + qemu_uuid_unparse(&vdev->vf_token, uuid); > > ...but qemu_uuid_unparse() writes UUID_FMT_LEN + 1 bytes, > including a trailing NUL. > > Every other use of UUID_FMT_LEN to declare an array > uses "UUID_FMT_LEN + 1" to avoid this. We also have : char uuidstr[37]; in vdi_header_print() and other places like test-uuid. > (In fact, every use of UUID_FMT_LEN at all uses "+ 1", > which suggests that perhaps defining it differently (and > perhaps with a different name) would reduce the risk of > this particular bug...) libuuid defines : #define UUID_STR_LEN 37 QEMU could do the same ? Thanks, C. > > thanks > -- PMM >
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index ec9a854361ac..cf27f28936cb 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) int groupid; int i, ret; bool is_mdev; + char uuid[UUID_FMT_LEN]; + char *name; if (!vbasedev->sysfsdev) { if (!(~vdev->host.domain || ~vdev->host.bus || @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) goto error; } - ret = vfio_get_device(group, vbasedev->name, vbasedev, errp); + if (!qemu_uuid_is_null(&vdev->vf_token)) { + qemu_uuid_unparse(&vdev->vf_token, uuid); + name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid); + } else { + name = vbasedev->name; + } + + ret = vfio_get_device(group, name, vbasedev, errp); + g_free(name); if (ret) { vfio_put_group(group); goto error; @@ -3268,6 +3278,7 @@ static void vfio_instance_init(Object *obj) static Property vfio_pci_dev_properties[] = { DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host), + DEFINE_PROP_UUID_NODEFAULT("vf-token", VFIOPCIDevice, vf_token), DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev), DEFINE_PROP_ON_OFF_AUTO("x-pre-copy-dirty-page-tracking", VFIOPCIDevice, vbasedev.pre_copy_dirty_page_tracking, diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index 177abcc8fb67..2674476d6c77 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -137,6 +137,7 @@ struct VFIOPCIDevice { VFIOVGA *vga; /* 0xa0000, 0x3b0, 0x3c0 */ void *igd_opregion; PCIHostDeviceAddress host; + QemuUUID vf_token; EventNotifier err_notifier; EventNotifier req_notifier; int (*resetfn)(struct VFIOPCIDevice *);