On 7/16/26 8:13 AM, Peter Krempa wrote:
On Mon, Jun 22, 2026 at 14:38:00 -0600, Jim Fehlig via Devel wrote:
From: Jim Fehlig <[email protected]>
In a clustered environment it may be desirable to provision disk leases
for use by a specific domain. This patch adds a 'check_disk_lease_owner'
option to the sanlock driver to check that leases are owned by the domain
attempting to acquire them. sanlock's Lock Value Block (LVB) is used to
store the owning domain UUID within the lease. When a domain is started
and attempts to acquire its leases, the sanlock driver will ensure its
UUID matches the UUID recorded in the leases. Any mismatches will cause
lease acquisition and domain startup to fail.
The 'check_disk_lease_owner' option is disabled by default. When enabled,
it can be used with auto_disk_leases or leases managed by an external
application.
Signed-off-by: Jim Fehlig <[email protected]>
---
Inspired by Claudio, implemented by Jim :-)
src/locking/libvirt_sanlock.aug | 1 +
src/locking/lock_driver_sanlock.c | 78 ++++++++++++++++++++++++-
src/locking/sanlock.conf | 23 ++++++++
src/locking/test_libvirt_sanlock.aug.in | 1 +
4 files changed, 102 insertions(+), 1 deletion(-)
diff --git a/src/locking/libvirt_sanlock.aug b/src/locking/libvirt_sanlock.aug
index 62d0672952..af6c25755f 100644
--- a/src/locking/libvirt_sanlock.aug
+++ b/src/locking/libvirt_sanlock.aug
@@ -25,6 +25,7 @@ module Libvirt_sanlock =
| int_entry "io_timeout"
| str_entry "user"
| str_entry "group"
+ | bool_entry "check_disk_lease_owner"
let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ .
del /\n/ "\n" ]
let empty = [ label "#empty" . eol ]
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index a07f3652c1..7ed1081cbd 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -67,6 +67,7 @@ struct _virLockManagerSanlockDriver {
unsigned int hostID;
bool autoDiskLease;
char *autoDiskLeasePath;
+ bool checkDiskLeaseOwner;
unsigned int io_timeout;
/* under which permissions does sanlock run */
@@ -147,6 +148,10 @@
virLockManagerSanlockLoadConfig(virLockManagerSanlockDriver *driver,
if (virConfGetValueBool(conf, "require_lease_for_disks",
&driver->requireLeaseForDisks) < 0)
return -1;
+ if (virConfGetValueBool(conf, "check_disk_lease_owner",
+ &driver->checkDiskLeaseOwner) < 0)
+ return -1;
+
if (virConfGetValueUInt(conf, "io_timeout", &driver->io_timeout) < 0)
return -1;
@@ -839,6 +844,73 @@ virLockManagerSanlockRegisterKillscript(int sock,
return 0;
}
+static int virLockManagerSanlockCheckOwner(virLockManagerSanlockDriver *driver,
+ virLockManagerSanlockPrivate *priv)
+{
+ char lvb[VIR_UUID_STRING_BUFLEN] = {0};
+ char vm_uuidstr[VIR_UUID_STRING_BUFLEN];
+ int rv;
+ size_t i;
+
+ if (!driver->checkDiskLeaseOwner)
+ return 0;
+
+ virUUIDFormat(priv->vm_uuid, vm_uuidstr);
+
+ for (i = 0; i < priv->res_count; i++) {
+ memset(lvb, 0, sizeof(lvb));
+
+ rv = sanlock_get_lvb(0, priv->res_args[i], lvb, sizeof(lvb));
While I didn't see any documentation, the implementation of this API
doesn't guarantee that 'lvb' is filled by a NUL-terminator, so if you
pass sizeof(lvb) as argument and the currently stored value is larger
than that the buffer will contain a non-terminated buffer thus allowing
overrun in the functions that do strcmp.
You need to pass 1 byte more of a buffer than you allow the function to
fill.
+ if (rv < 0) {
+ /* Failed to read LVB, treat as "legacy" lease
+ * without ownership tracking and skip the check.
+ */
+ VIR_DEBUG("Failed to read LVB, skipping ownership check");
+ continue;
+ }
+
+ if (lvb[0] == '\0') {
+ VIR_DEBUG("Writing VM UUID to empty LVB");
+ /* Empty LVB: this lease does not yet have an owner recorded.
Write
+ * the domain's UUID so that subsequent acquire attempts by other
+ * domains will be rejected. This handles the auto_disk_leases case
+ * where lease files are created by libvirt. rather
+ */
+ if ((rv = sanlock_set_lvb(0, priv->res_args[i], vm_uuidstr,
+ sizeof(vm_uuidstr))) < 0) {
+ char *err = NULL;
g_autofree.
+ if (virLockManagerSanlockError(rv, &err)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to set LVB for lease %1$s: %2$s"),
+ priv->res_args[i]->disks[0].path,
NULLSTR(err));
+ VIR_FREE(err);
+ } else {
+ virReportSystemError(-rv,
+ _("Failed to set LVB for lease %1$s"),
+ priv->res_args[i]->disks[0].path);
+ }
+ return -1;
+ }
+ continue;
+ }
+
+ VIR_DEBUG("Comparing LVB UUID '%s' to VM UUID '%s'", lvb, vm_uuidstr);
This can theoretically contain binary garbage from different usage.
I left this one since it's debug and delimited by '.
+ if (STRNEQ(lvb, vm_uuidstr)) {
+ /*
+ * LVB contains a different domain UUID. This lease
+ * was provisioned for another domain and must not
+ * be acquired by this one.
+ */
+ virReportError(VIR_ERR_RESOURCE_BUSY,
+ _("Disk lease %1$s is already assigned to domain %2$s,
not %3$s"),
+ priv->res_args[i]->disks[0].path, lvb, vm_uuidstr);
same here, the source is not guartanteed to be an UUID but can be
arbitrary data. I'm not sure if it's a good idea to print it.
Even if you think it is you should contain it in '' or some other marker
as otherwise it's hard to see what is part of the error message and what
is some arbitrary data or identifiers
+ return -1;
+ }
+ }
+ return 0;
+}
+
+
static int virLockManagerSanlockAcquire(virLockManager *lock,
const char *state,
unsigned int flags,
@@ -934,7 +1006,8 @@ static int virLockManagerSanlockAcquire(virLockManager
*lock,
if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
VIR_DEBUG("Acquiring object %u", priv->res_count);
- if ((rv = sanlock_acquire(sock, priv->vm_pid, 0,
+ if ((rv = sanlock_acquire(sock, priv->vm_pid,
+ driver->checkDiskLeaseOwner ?
SANLK_ACQUIRE_LVB : 0,
priv->res_count, priv->res_args,
opt)) < 0) {
char *err = NULL;
@@ -949,6 +1022,9 @@ static int virLockManagerSanlockAcquire(virLockManager
*lock,
}
goto error;
}
+
+ if (virLockManagerSanlockCheckOwner(driver, priv) < 0)
+ goto error;
}
VIR_FREE(opt);
diff --git a/src/locking/sanlock.conf b/src/locking/sanlock.conf
index 3c356bef9c..e7d6938e84 100644
--- a/src/locking/sanlock.conf
+++ b/src/locking/sanlock.conf
@@ -67,3 +67,26 @@
# access them. Accepted values are described in qemu.conf.
#user = "root"
#group = "root"
+
+#
+# Flag to enable verifying if disk leases have been provisioned
+# for domains attempting to acquire them. This is accomplished
+# using sanlock's Lock Value Block (LVB), a 512 byte application
+# specific block in the lease lockspace where the domain's UUID
+# can be stored when the lease is created. When libvirt starts a
+# domain, the sanlock driver will check for a valid and matching
+# UUID in the LVB of all leases. This prevents a lease that was
+# provisioned for one domain from accidentally being used by a
+# different domain.
+#
+# When enabled, if a lease file has an empty LVB, the UUID of the
+# acquiring domain is written into the LVB automatically, so that
+# subsequent lease acquisition attempts by other domains will be
+# rejected. This behavior accommodates the auto_disk_leases case,
+# where leases are created by libvirt. If leases are managed by
+# an external application, it must write the domain UUID into the
+# LVB during lease creation.
While it's implied I'd explicitly mention that the UUID is never removed
from the LVB and thus if the image is to be ever used with a different
VM it will need to be cleared or changed manually
+#
+# Defaults to disabled.
+#
+#check_disk_lease_owner = 0
diff --git a/src/locking/test_libvirt_sanlock.aug.in
b/src/locking/test_libvirt_sanlock.aug.in
index 5eabb6726d..e468a2cdcd 100644
--- a/src/locking/test_libvirt_sanlock.aug.in
+++ b/src/locking/test_libvirt_sanlock.aug.in
@@ -9,3 +9,4 @@ module Test_libvirt_sanlock =
{ "io_timeout" = "0" }
{ "user" = "root" }
{ "group" = "root" }
+{ "check_disk_lease_owner" = "0" }
--
2.51.0
a
I also didn;'t check what versions of sanlock this is available or
compare it with supported platforms.
If you agree with the changes I've proposed and make sure that all
supported platforms support the API you're about to use you can use:
I agree with the changes and have squashed the below diff in my local branch.
WRT supported platforms, I asked AI to investigate. sanlock_{get,set}_lvb first
appeared in sanlock 3.3.0. All supported plaforms contain something newer than that.
Reviewed-by: Peter Krempa <[email protected]>
Thanks! Before pushing, your opinion on the changes would be appreciated.
Regards,
Jim
diff --git a/src/locking/lock_driver_sanlock.c
b/src/locking/lock_driver_sanlock.c
index 7ed1081cbd..26e84c7efe 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -860,7 +860,7 @@ static int
virLockManagerSanlockCheckOwner(virLockManagerSanlockDriver *driver,
for (i = 0; i < priv->res_count; i++) {
memset(lvb, 0, sizeof(lvb));
- rv = sanlock_get_lvb(0, priv->res_args[i], lvb, sizeof(lvb));
+ rv = sanlock_get_lvb(0, priv->res_args[i], lvb, sizeof(lvb) - 1);
if (rv < 0) {
/* Failed to read LVB, treat as "legacy" lease
* without ownership tracking and skip the check.
@@ -878,12 +878,11 @@ static int
virLockManagerSanlockCheckOwner(virLockManagerSanlockDriver *driver,
*/
if ((rv = sanlock_set_lvb(0, priv->res_args[i], vm_uuidstr,
sizeof(vm_uuidstr))) < 0) {
- char *err = NULL;
+ g_autofree char *err = NULL;
if (virLockManagerSanlockError(rv, &err)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to set LVB for lease %1$s: %2$s"),
priv->res_args[i]->disks[0].path,
NULLSTR(err));
- VIR_FREE(err);
} else {
virReportSystemError(-rv,
_("Failed to set LVB for lease %1$s"),
@@ -902,8 +901,8 @@ static int
virLockManagerSanlockCheckOwner(virLockManagerSanlockDriver *driver,
* be acquired by this one.
*/
virReportError(VIR_ERR_RESOURCE_BUSY,
- _("Disk lease %1$s is already assigned to domain
%2$s, not %3$s"),
- priv->res_args[i]->disks[0].path, lvb, vm_uuidstr);
+ _("Disk lease %1$s is not owned by domain with UUID
%2$s"),
+ priv->res_args[i]->disks[0].path, vm_uuidstr);
return -1;
}
}
diff --git a/src/locking/sanlock.conf b/src/locking/sanlock.conf
index e7d6938e84..6953e5889b 100644
--- a/src/locking/sanlock.conf
+++ b/src/locking/sanlock.conf
@@ -87,6 +87,11 @@
# an external application, it must write the domain UUID into the
# LVB during lease creation.
#
+# libvirt will never delete the contents of the LVB, even if it
+# previously wrote the UUID to an empty LVB with auto_disk_leases
+# enabled. An external application must clear or update the LVB
+# if the associated lease is to be used with a different domain.
+#
# Defaults to disabled.
#
