| 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: <aQvjQDwU3f0crccT@google.com>
Date: Wed, 5 Nov 2025 23:52:32 +0000
From: David Matlack <dmatlack@...gle.com>
To: Raghavendra Rao Ananta <rananta@...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
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.
> -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.
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;
> +
> + /*
> + * 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);
}
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.
> +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.
Powered by blists - more mailing lists