[libvirt] [PATCH 0/5] util: open pci config read-only for querying
A bunch of functions opened the PCI device config as read-write even though they only read from it. Ján Tomko (5): util: introduce virPCIDeviceConfigOpenInternal util: Introduce virPCIDeviceConfigOpenWrite util: introduce readonly attribute to virPCIDeviceConfigOpenInternal util: introduce virPCIDeviceConfigOpenTry util: default to read-only in virPCIDeviceConfigOpen src/util/virpci.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-) -- 2.20.1
A thin wrapper to allow creating new functions. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virpci.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 61a6b359e5..f4be907a10 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -291,7 +291,7 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name) static int -virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal) +virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool fatal) { int fd; @@ -314,6 +314,12 @@ virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal) return fd; } +static int +virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal) +{ + return virPCIDeviceConfigOpenInternal(dev, fatal); +} + static void virPCIDeviceConfigClose(virPCIDevicePtr dev, int cfgfd) { -- 2.20.1
Only a handful of function need write access to the PCI config space. Create a wrapper function for those so that we can open it read only by default. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virpci.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index f4be907a10..c1dad1e69f 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -320,6 +320,12 @@ virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal) return virPCIDeviceConfigOpenInternal(dev, fatal); } +static int +virPCIDeviceConfigOpenWrite(virPCIDevicePtr dev) +{ + return virPCIDeviceConfigOpenInternal(dev, true); +} + static void virPCIDeviceConfigClose(virPCIDevicePtr dev, int cfgfd) { @@ -808,7 +814,7 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev, dev->name); return -1; } - if ((parentfd = virPCIDeviceConfigOpen(parent, true)) < 0) + if ((parentfd = virPCIDeviceConfigOpenWrite(parent)) < 0) goto out; VIR_DEBUG("%s %s: doing a secondary bus reset", dev->id, dev->name); @@ -957,7 +963,7 @@ virPCIDeviceReset(virPCIDevicePtr dev, } VIR_DEBUG("Resetting device %s", dev->name); - if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0) + if ((fd = virPCIDeviceConfigOpenWrite(dev)) < 0) goto cleanup; if (virPCIDeviceInit(dev, fd) < 0) -- 2.20.1
Allow wrappers to open PCI config as read-only. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virpci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index c1dad1e69f..e45dfbc631 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -291,11 +291,11 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name) static int -virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool fatal) +virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool readonly, bool fatal) { int fd; - fd = open(dev->path, O_RDWR); + fd = open(dev->path, readonly ? O_RDONLY : O_RDWR); if (fd < 0) { if (fatal) { @@ -317,13 +317,13 @@ virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool fatal) static int virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal) { - return virPCIDeviceConfigOpenInternal(dev, fatal); + return virPCIDeviceConfigOpenInternal(dev, false, fatal); } static int virPCIDeviceConfigOpenWrite(virPCIDevicePtr dev) { - return virPCIDeviceConfigOpenInternal(dev, true); + return virPCIDeviceConfigOpenInternal(dev, false, true); } static void -- 2.20.1
For callers that only need read-only access and don't want an error reported. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virpci.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index e45dfbc631..2758ee6f49 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -320,6 +320,12 @@ virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal) return virPCIDeviceConfigOpenInternal(dev, false, fatal); } +static int +virPCIDeviceConfigOpenTry(virPCIDevicePtr dev) +{ + return virPCIDeviceConfigOpenInternal(dev, true, false); +} + static int virPCIDeviceConfigOpenWrite(virPCIDevicePtr dev) { @@ -692,7 +698,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) if (dev->address.domain != check->address.domain) return 0; - if ((fd = virPCIDeviceConfigOpen(check, false)) < 0) + if ((fd = virPCIDeviceConfigOpenTry(check)) < 0) return 0; /* Is it a bridge? */ @@ -740,7 +746,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) int bestfd; uint8_t best_secondary; - if ((bestfd = virPCIDeviceConfigOpen(*best, false)) < 0) + if ((bestfd = virPCIDeviceConfigOpenTry(*best)) < 0) goto cleanup; best_secondary = virPCIDeviceRead8(*best, bestfd, PCI_SECONDARY_BUS); virPCIDeviceConfigClose(*best, bestfd); -- 2.20.1
All the callers left require virPCIDeviceConfigOpen to be fatal and only use read-only access to the config file. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virpci.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 2758ee6f49..7ca755c1a8 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -315,9 +315,9 @@ virPCIDeviceConfigOpenInternal(virPCIDevicePtr dev, bool readonly, bool fatal) } static int -virPCIDeviceConfigOpen(virPCIDevicePtr dev, bool fatal) +virPCIDeviceConfigOpen(virPCIDevicePtr dev) { - return virPCIDeviceConfigOpenInternal(dev, false, fatal); + return virPCIDeviceConfigOpenInternal(dev, true, true); } static int @@ -2426,7 +2426,7 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev) int ret = 0; uint16_t device_class; - if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0) + if ((fd = virPCIDeviceConfigOpen(dev)) < 0) return -1; if (virPCIDeviceInit(dev, fd) < 0) { @@ -3168,7 +3168,7 @@ virPCIDeviceIsPCIExpress(virPCIDevicePtr dev) int fd; int ret = -1; - if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0) + if ((fd = virPCIDeviceConfigOpen(dev)) < 0) return ret; if (virPCIDeviceInit(dev, fd) < 0) @@ -3188,7 +3188,7 @@ virPCIDeviceHasPCIExpressLink(virPCIDevicePtr dev) int ret = -1; uint16_t cap, type; - if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0) + if ((fd = virPCIDeviceConfigOpen(dev)) < 0) return ret; if (virPCIDeviceInit(dev, fd) < 0) @@ -3216,7 +3216,7 @@ virPCIDeviceGetLinkCapSta(virPCIDevicePtr dev, int fd; int ret = -1; - if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0) + if ((fd = virPCIDeviceConfigOpen(dev)) < 0) return ret; if (virPCIDeviceInit(dev, fd) < 0) @@ -3254,7 +3254,7 @@ int virPCIGetHeaderType(virPCIDevicePtr dev, int *hdrType) *hdrType = -1; - if ((fd = virPCIDeviceConfigOpen(dev, true)) < 0) + if ((fd = virPCIDeviceConfigOpen(dev)) < 0) return -1; type = virPCIDeviceRead8(dev, fd, PCI_HEADER_TYPE); -- 2.20.1
On Tue, Aug 13, 2019 at 03:45:19PM +0200, Ján Tomko wrote:
All the callers left require virPCIDeviceConfigOpen to be fatal and only use read-only access to the config file.
Signed-off-by: Ján Tomko <jtomko@redhat.com> ---
FYI this patch broke make check, both virpcitest and virhostdevtest. Erik
On Thu, Aug 15, 2019 at 10:10:06 +0200, Erik Skultety wrote:
On Tue, Aug 13, 2019 at 03:45:19PM +0200, Ján Tomko wrote:
All the callers left require virPCIDeviceConfigOpen to be fatal and only use read-only access to the config file.
Signed-off-by: Ján Tomko <jtomko@redhat.com> ---
FYI this patch broke make check, both virpcitest and virhostdevtest.
Actually it's the previous patch according to my bisection and it happens only with gcc. Clang works fine. This hints to some inlining weridness happening together with the pci mocking code.
On 8/13/19 3:45 PM, Ján Tomko wrote:
A bunch of functions opened the PCI device config as read-write even though they only read from it.
Ján Tomko (5): util: introduce virPCIDeviceConfigOpenInternal util: Introduce virPCIDeviceConfigOpenWrite util: introduce readonly attribute to virPCIDeviceConfigOpenInternal util: introduce virPCIDeviceConfigOpenTry util: default to read-only in virPCIDeviceConfigOpen
src/util/virpci.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (4)
-
Erik Skultety -
Ján Tomko -
Michal Privoznik -
Peter Krempa