| 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
| ||
|
Message-ID: <CAJHc60xb_=v9k46MEo=6S5QmMXKnd_1FiuWQr9dkCnE_XtTkfQ@mail.gmail.com>
Date: Thu, 6 Nov 2025 21:56:27 +0530
From: Raghavendra Rao Ananta <rananta@...gle.com>
To: David Matlack <dmatlack@...gle.com>
Cc: Alex Williamson <alex@...zbot.org>, Alex Williamson <alex.williamson@...hat.com>,
Josh Hilke <jrhilke@...gle.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in
device init
Hi David,
On Thu, Nov 6, 2025 at 5:22 AM David Matlack <dmatlack@...gle.com> wrote:
>
> On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
>
> > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode);
> > +struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > + const char *iommu_mode,
> > + const char *vf_token);
>
> Vipin is also looking at adding an optional parameter to
> vfio_pci_device_init():
> https://lore.kernel.org/kvm/20251018000713.677779-20-vipinsh@google.com/
>
> I am wondering if we should support an options struct for such
> parameters. e.g. something like this
>
> diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> index b01068d98fda..cee837fe561c 100644
> --- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
> +++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> @@ -160,6 +160,10 @@ struct vfio_pci_driver {
> int msi;
> };
>
> +struct vfio_pci_device_options {
> + const char *vf_token;
> +};
> +
> struct vfio_pci_device {
> int fd;
>
> @@ -202,9 +206,18 @@ const char *vfio_pci_get_cdev_path(const char *bdf);
>
> extern const char *default_iommu_mode;
>
> -struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> - const char *iommu_mode,
> - const char *vf_token);
> +struct vfio_pci_device *__vfio_pci_device_init(const char *bdf,
> + const char *iommu_mode,
> + const struct vfio_pci_device_options *options);
> +
> +static inline struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> + const char *iommu_mode)
> +{
> + static const struct vfio_pci_device_options default_options = {};
> +
> + return __vfio_pci_device_init(bdf, iommu_mode, &default_options);
> +}
> +
>
> This will avoid you having to update every test.
>
> You can create a helper function in vfio_pci_sriov_uapi_test.c to call
> __vfio_pci_device_init() and abstract away the options stuff from your
> test.
>
I like the idea of an optional expandable struct. I'll implement this in v2.
> > -static void vfio_pci_container_setup(struct vfio_pci_device *device, const char *bdf)
> > +static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
>
> Let's name this vfio_pci_group_get_device_fd() since it's getting the
> device FD from the group using ioctl(VFIO_GROUP_GET_DEVICE_FD).
>
> > + const char *bdf,
> > + const char *vf_token)
>
> There's an extra space before these arguments. Align them all vertically
> with the first argument.
>
> I noticed this throughout this patch, so please fix throughout the whole
> series in v2.
>
I may have to fix my editor to display this right. Thanks for catching this.
> Also please run checkpatch.pl. It will catch things like this for you.
>
> CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
> #78: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:335:
> +static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
> + const char *bdf,
>
> > +{
> > + char *arg = (char *) bdf;
>
> No space necessary after a cast. This is another one checkpatch.pl will
> catch for you.
>
> CHECK:SPACING: No space is necessary after a cast
> #81: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:338:
> + char *arg = (char *) bdf;
>
Actually, I did run checkpatch.pl on the entire series as:
.$ ./scripts/checkpatch.pl *.patch
I didn't see any of these warnings. Are there any other options to consider?
> > +
> > + /*
> > + * If a vf_token exists, argument to VFIO_GROUP_GET_DEVICE_FD
> > + * will be in the form of the following example:
> > + * "0000:04:10.0 vf_token=bd8d9d2b-5a5f-4f5a-a211-f591514ba1f3"
> > + */
> > + if (vf_token) {
> > + size_t sz = strlen(bdf) + strlen(" "VF_TOKEN_ARG) +
> > + strlen(vf_token) + 1;
> > +
> > + arg = calloc(1, sz);
> > + VFIO_ASSERT_NOT_NULL(arg);
> > +
> > + snprintf(arg, sz, "%s %s%s", bdf, VF_TOKEN_ARG, vf_token);
> > + }
>
> UUIDs are 16 bytes so I think we could create a pretty reasonably fixed
> size array to hold the argument and simplify this code, make it more
> self-documenting, and eliminate VF_TOKEN_ARG. This is test code, so we
> can make the array bigger in the future if we need to. Keeping the code
> simple is more important IMO.
>
> static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
> const char *bdf,
> const char *vf_token)
> {
> char arg[64];
>
> if (vf_token)
> snprintf(arg, ARRAY_SIZE(arg), "%s vf_token=%s", bdf, vf_token);
> else
> snprintf(arg, ARRAY_SIZE(arg), "%s", bdf);
>
> device->fd = ioctl(device->group_fd, VFIO_GROUP_GET_DEVICE_FD, arg);
> VFIO_ASSERT_GE(device->fd, 0);
> }
>
Yeah, this is a good idea! I'll implement it in v2.
> And to protect against writing off the end of arg, we can introduce a
> snprintf_assert() in a separate commit. There are actually a few
> snprintf() calls in vfio_pci_device.c that would be nice to convert to
> snprintf_assert().
>
> #define snprintf_assert(_s, _size, _fmt, ...) do { \
> int __ret = snprintf(_s, _size, _fmt, ##__VA_ARGS__); \
> VFIO_ASSERT_LT(__ret, _size); \
> } while (0)
>
> snprintf_assert() could be added in a precursor commit to this one.
>
Sounds good. I'll add a commit for this and convert all the
snprintf()s to snprintf_assert()s.
> > +static void vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
> > {
> > struct vfio_device_bind_iommufd args = {
> > .argsz = sizeof(args),
> > .iommufd = iommufd,
> > };
> > + uuid_t token_uuid = {0};
> > +
> > + if (vf_token) {
> > + VFIO_ASSERT_EQ(uuid_parse(vf_token, token_uuid), 0);
> > + args.flags = VFIO_DEVICE_BIND_FLAG_TOKEN;
>
> Maybe make this |= instead of = ? I had to double-check that this wasn't
> overwriting any flags set above this. Obviously it's not since this is a
> small function, but |= would make that super obvious.
Oh yes, absolutely!
Thank you.
Raghavendra
Powered by blists - more mailing lists