[libvirt] [PATCH v2] tpm: adapt sysfs cancel path for new TPM driver
Adapt the sysfs TPM command cancel path for the TPM driver that does not use a miscdevice anymore since Linux 4.0. Support old and new paths and check their availability. Add a mockup for the test cases to avoid the testing for availability of the cancel path. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/util/virtpm.c | 22 ++++++++++++++++++++-- tests/qemuxml2argvmock.c | 15 +++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/util/virtpm.c b/src/util/virtpm.c index 88f8361..3872a31 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -23,9 +23,12 @@ #include <config.h> #include <sys/stat.h> +#include <fcntl.h> #include "virstring.h" #include "virerror.h" +#include "viralloc.h" +#include "virfile.h" #include "virtpm.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -41,13 +44,28 @@ virTPMCreateCancelPath(const char *devpath) { char *path = NULL; const char *dev; + const char *prefix[] = {"misc/", "tpm/"}; + size_t i; + int fd; if (devpath) { dev = strrchr(devpath, '/'); if (dev) { dev++; - if (virAsprintf(&path, "/sys/class/misc/%s/device/cancel", - dev) < 0) + for (i = 0; i < ARRAY_CARDINALITY(prefix); i++) { + if (virAsprintf(&path, "/sys/class/%s%s/device/cancel", + prefix[i], dev) < 0) + goto cleanup; + + fd = open(path, O_WRONLY); + if (fd >= 0) { + VIR_FORCE_CLOSE(fd); + break; + } + VIR_FREE(path); + } + /* /dev/null does not allow to cancel cmds but it can be used */ + if (!path && virAsprintf(&path, "/dev/null") < 0) goto cleanup; } else { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index d24725e..e58b8ce 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -24,9 +24,13 @@ #include "virnuma.h" #include "virmock.h" #include "virutil.h" +#include "virstring.h" +#include "virtpm.h" #include <time.h> #include <unistd.h> +#define VIR_FROM_THIS VIR_FROM_NONE + long virGetSystemPageSize(void) { return 4096; @@ -59,3 +63,14 @@ virNumaNodeIsAvailable(int node) return node >= 0 && node <= virNumaGetMaxNode(); } #endif /* WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET */ + +char * +virTPMCreateCancelPath(const char *devpath) +{ + char *path; + (void)devpath; + + ignore_value(VIR_STRDUP(path, "/sys/class/misc/tpm0/device/cancel")); + + return path; +} -- 2.4.3
First, thanks for following up on the fedora bugs so quickly! On 11/17/2015 10:46 AM, Stefan Berger wrote:
Adapt the sysfs TPM command cancel path for the TPM driver that does not use a miscdevice anymore since Linux 4.0. Support old and new paths and check their availability.
Add a mockup for the test cases to avoid the testing for availability of the cancel path.
I see you sent a qemu patch for a similar change. What's the benefit of libvirt setting cancel_path if qemu can (and already attempts to) figure it out? - Cole
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/util/virtpm.c | 22 ++++++++++++++++++++-- tests/qemuxml2argvmock.c | 15 +++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/src/util/virtpm.c b/src/util/virtpm.c index 88f8361..3872a31 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -23,9 +23,12 @@ #include <config.h>
#include <sys/stat.h> +#include <fcntl.h>
#include "virstring.h" #include "virerror.h" +#include "viralloc.h" +#include "virfile.h" #include "virtpm.h"
#define VIR_FROM_THIS VIR_FROM_NONE @@ -41,13 +44,28 @@ virTPMCreateCancelPath(const char *devpath) { char *path = NULL; const char *dev; + const char *prefix[] = {"misc/", "tpm/"}; + size_t i; + int fd;
if (devpath) { dev = strrchr(devpath, '/'); if (dev) { dev++; - if (virAsprintf(&path, "/sys/class/misc/%s/device/cancel", - dev) < 0) + for (i = 0; i < ARRAY_CARDINALITY(prefix); i++) { + if (virAsprintf(&path, "/sys/class/%s%s/device/cancel", + prefix[i], dev) < 0) + goto cleanup; + + fd = open(path, O_WRONLY); + if (fd >= 0) { + VIR_FORCE_CLOSE(fd); + break; + } + VIR_FREE(path); + } + /* /dev/null does not allow to cancel cmds but it can be used */ + if (!path && virAsprintf(&path, "/dev/null") < 0) goto cleanup; } else { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index d24725e..e58b8ce 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -24,9 +24,13 @@ #include "virnuma.h" #include "virmock.h" #include "virutil.h" +#include "virstring.h" +#include "virtpm.h" #include <time.h> #include <unistd.h>
+#define VIR_FROM_THIS VIR_FROM_NONE + long virGetSystemPageSize(void) { return 4096; @@ -59,3 +63,14 @@ virNumaNodeIsAvailable(int node) return node >= 0 && node <= virNumaGetMaxNode(); } #endif /* WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET */ + +char * +virTPMCreateCancelPath(const char *devpath) +{ + char *path; + (void)devpath; + + ignore_value(VIR_STRDUP(path, "/sys/class/misc/tpm0/device/cancel")); + + return path; +}
On 11/17/2015 02:12 PM, Cole Robinson wrote:
First, thanks for following up on the fedora bugs so quickly!
On 11/17/2015 10:46 AM, Stefan Berger wrote:
Adapt the sysfs TPM command cancel path for the TPM driver that does not use a miscdevice anymore since Linux 4.0. Support old and new paths and check their availability.
Add a mockup for the test cases to avoid the testing for availability of the cancel path.
I see you sent a qemu patch for a similar change. What's the benefit of libvirt setting cancel_path if qemu can (and already attempts to) figure it out?
The only benefit is that libvirt is controlling the parameter. Stefan
On 11/17/2015 03:49 PM, Stefan Berger wrote:
On 11/17/2015 02:12 PM, Cole Robinson wrote:
First, thanks for following up on the fedora bugs so quickly!
On 11/17/2015 10:46 AM, Stefan Berger wrote:
Adapt the sysfs TPM command cancel path for the TPM driver that does not use a miscdevice anymore since Linux 4.0. Support old and new paths and check their availability.
Add a mockup for the test cases to avoid the testing for availability of the cancel path.
I see you sent a qemu patch for a similar change. What's the benefit of libvirt setting cancel_path if qemu can (and already attempts to) figure it out?
The only benefit is that libvirt is controlling the parameter.
Then IMO we should let qemu handle it and just drop this path handling entirely. If there's ever a need to differentiate from qemu's logic or let the user override this value then we can add it back without much trouble Thanks, Cole
Cole Robinson <crobinso@redhat.com> wrote on 11/17/2015 03:55:13 PM:
From: Cole Robinson <crobinso@redhat.com> To: Stefan Berger <stefanb@linux.vnet.ibm.com>, Stefan Berger/ Watson/IBM@IBMUS, libvir-list@redhat.com, berrange@redhat.com Date: 11/17/2015 03:55 PM Subject: Re: [PATCH v2] tpm: adapt sysfs cancel path for new TPM driver
On 11/17/2015 03:49 PM, Stefan Berger wrote:
On 11/17/2015 02:12 PM, Cole Robinson wrote:
First, thanks for following up on the fedora bugs so quickly!
On 11/17/2015 10:46 AM, Stefan Berger wrote:
Adapt the sysfs TPM command cancel path for the TPM driver that does not use a miscdevice anymore since Linux 4.0. Support old and new paths and check their availability.
Add a mockup for the test cases to avoid the testing for availability of the cancel path.
I see you sent a qemu patch for a similar change. What's the benefit of libvirt setting cancel_path if qemu can (and already attempts to)figure it out?
The only benefit is that libvirt is controlling the parameter.
Then IMO we should let qemu handle it and just drop this path handling entirely. If there's ever a need to differentiate from qemu's logic or let the user override this value then we can add it back without much trouble
Forgot about that: SELinux labeling of that file requires it afaik. Stefan
On 11/17/2015 10:46 AM, Stefan Berger wrote:
Adapt the sysfs TPM command cancel path for the TPM driver that does not use a miscdevice anymore since Linux 4.0. Support old and new paths and check their availability.
Add a mockup for the test cases to avoid the testing for availability of the cancel path.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/util/virtpm.c | 22 ++++++++++++++++++++-- tests/qemuxml2argvmock.c | 15 +++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/src/util/virtpm.c b/src/util/virtpm.c index 88f8361..3872a31 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -23,9 +23,12 @@ #include <config.h>
#include <sys/stat.h> +#include <fcntl.h>
#include "virstring.h" #include "virerror.h" +#include "viralloc.h" +#include "virfile.h" #include "virtpm.h"
#define VIR_FROM_THIS VIR_FROM_NONE @@ -41,13 +44,28 @@ virTPMCreateCancelPath(const char *devpath) { char *path = NULL; const char *dev; + const char *prefix[] = {"misc/", "tpm/"}; + size_t i; + int fd;
if (devpath) { dev = strrchr(devpath, '/'); if (dev) { dev++; - if (virAsprintf(&path, "/sys/class/misc/%s/device/cancel", - dev) < 0) + for (i = 0; i < ARRAY_CARDINALITY(prefix); i++) { + if (virAsprintf(&path, "/sys/class/%s%s/device/cancel", + prefix[i], dev) < 0) + goto cleanup; + + fd = open(path, O_WRONLY); + if (fd >= 0) { + VIR_FORCE_CLOSE(fd); + break; + } + VIR_FREE(path); + }
You can use virFileExists(path) to simplify this
+ /* /dev/null does not allow to cancel cmds but it can be used */ + if (!path && virAsprintf(&path, "/dev/null") < 0) goto cleanup; } else {
Hmm, does this mean we might selinux relabel /dev/null? Maybe just fall back to no cancel-path if we want this to be non-fatal - Cole
virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index d24725e..e58b8ce 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -24,9 +24,13 @@ #include "virnuma.h" #include "virmock.h" #include "virutil.h" +#include "virstring.h" +#include "virtpm.h" #include <time.h> #include <unistd.h>
+#define VIR_FROM_THIS VIR_FROM_NONE + long virGetSystemPageSize(void) { return 4096; @@ -59,3 +63,14 @@ virNumaNodeIsAvailable(int node) return node >= 0 && node <= virNumaGetMaxNode(); } #endif /* WITH_NUMACTL && HAVE_NUMA_BITMASK_ISBITSET */ + +char * +virTPMCreateCancelPath(const char *devpath) +{ + char *path; + (void)devpath; + + ignore_value(VIR_STRDUP(path, "/sys/class/misc/tpm0/device/cancel")); + + return path; +}
Cole Robinson <crobinso@redhat.com> wrote on 11/17/2015 04:37:56 PM:
From: Cole Robinson <crobinso@redhat.com> To: Stefan Berger/Watson/IBM@IBMUS, libvir-list@redhat.com, berrange@redhat.com Cc: Stefan Berger <stefanb@linux.vnet.ibm.com> Date: 11/17/2015 04:38 PM Subject: Re: [PATCH v2] tpm: adapt sysfs cancel path for new TPM driver
On 11/17/2015 10:46 AM, Stefan Berger wrote:
Adapt the sysfs TPM command cancel path for the TPM driver that does not use a miscdevice anymore since Linux 4.0. Support old and new paths and check their availability.
Add a mockup for the test cases to avoid the testing for availability of the cancel path.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/util/virtpm.c | 22 ++++++++++++++++++++-- tests/qemuxml2argvmock.c | 15 +++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/src/util/virtpm.c b/src/util/virtpm.c index 88f8361..3872a31 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -23,9 +23,12 @@ #include <config.h>
#include <sys/stat.h> +#include <fcntl.h>
#include "virstring.h" #include "virerror.h" +#include "viralloc.h" +#include "virfile.h" #include "virtpm.h"
#define VIR_FROM_THIS VIR_FROM_NONE @@ -41,13 +44,28 @@ virTPMCreateCancelPath(const char *devpath) { char *path = NULL; const char *dev; + const char *prefix[] = {"misc/", "tpm/"}; + size_t i; + int fd;
if (devpath) { dev = strrchr(devpath, '/'); if (dev) { dev++; - if (virAsprintf(&path, "/sys/class/misc/%s/device/cancel", - dev) < 0) + for (i = 0; i < ARRAY_CARDINALITY(prefix); i++) { + if (virAsprintf(&path, "/sys/class/%s%s/device/cancel", + prefix[i], dev) < 0) + goto cleanup; + + fd = open(path, O_WRONLY); + if (fd >= 0) { + VIR_FORCE_CLOSE(fd); + break; + } + VIR_FREE(path); + }
You can use virFileExists(path) to simplify this
Right.
+ /* /dev/null does not allow to cancel cmds but it canbe
used */
+ if (!path && virAsprintf(&path, "/dev/null") < 0) goto cleanup; } else {
Hmm, does this mean we might selinux relabel /dev/null? Maybe just fall back to no cancel-path if we want this to be non-fatal
We have to pass something on the command line otherwise QEMU will try to find a path and SELinux will not allow it to open the file. Basically either one of these sysfs files has to be there. I'll resort to displaying an error instead. Regards, Stefan
participants (3)
-
Cole Robinson -
Stefan Berger -
Stefan Berger