[libvirt] [PATCH] sanlock: Enhance error message to point to possible problem with selinux
If the connection to the sanlock daemon is forbidden by selinux the error message was not clear enough. This patch adds a check if proper configuration for selinux is used while trying to connect to sanlock. *src/locking/lock_driver_sanlock.c: - add macro virLockSystemError that checks for selinux and reports an improved error message - modify calls of virReportSystemError to the new macro in apropriate places Background: https://bugzilla.redhat.com/show_bug.cgi?id=770488 --- src/locking/lock_driver_sanlock.c | 83 +++++++++++++++++++++++-------------- 1 files changed, 52 insertions(+), 31 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index d344d6a..d5634f9 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -35,6 +35,10 @@ #include <sanlock_resource.h> #include <sanlock_admin.h> +#if HAVE_SELINUX +# include <selinux/selinux.h> +#endif + #include "lock_driver.h" #include "logging.h" #include "virterror_internal.h" @@ -51,7 +55,23 @@ #define virLockError(code, ...) \ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) - +#if HAVE_SELINUX +# define virLockSystemError(theerrno, format, ...) \ + do { \ + if ((theerrno)==EACCES && \ + security_get_boolean_active("virt_use_sanlock") == 0) { \ + char errbuff[1024]; \ + snprintf(errbuff, sizeof(errbuff), "%s %s", (format), \ + _("(Consider setting virt_use_sanlock selinux variable)"));\ + virReportSystemError((theerrno), errbuff, __VA_ARGS__); \ + } else { \ + virReportSystemError((theerrno), (format), __VA_ARGS__); \ + } \ + } while(0); +#else +# define virLockSystemError(...) \ + virReportSystemError(__VA_ARGS__); +#endif #define VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE "__LIBVIRT__DISKS__" @@ -186,9 +206,9 @@ static int virLockManagerSanlockSetupLockspace(void) _("Unable to query sector size %s: error %d"), path, rv); else - virReportSystemError(-rv, - _("Unable to query sector size %s"), - path); + virLockSystemError(-rv, + _("Unable to query sector size %s"), + path); goto error_unlink; } @@ -215,9 +235,9 @@ static int virLockManagerSanlockSetupLockspace(void) _("Unable to initialize lockspace %s: error %d"), path, rv); else - virReportSystemError(-rv, - _("Unable to initialize lockspace %s"), - path); + virLockSystemError(-rv, + _("Unable to initialize lockspace %s"), + path); goto error_unlink; } VIR_DEBUG("Lockspace %s has been initialized", path); @@ -236,9 +256,9 @@ static int virLockManagerSanlockSetupLockspace(void) _("Unable to add lockspace %s: error %d"), path, rv); else - virReportSystemError(-rv, - _("Unable to add lockspace %s"), - path); + virLockSystemError(-rv, + _("Unable to add lockspace %s"), + path); goto error_unlink; } else { VIR_DEBUG("Lockspace %s is already registered", path); @@ -559,9 +579,9 @@ static int virLockManagerSanlockCreateLease(struct sanlk_resource *res) _("Unable to query sector size %s: error %d"), res->disks[0].path, rv); else - virReportSystemError(-rv, - _("Unable to query sector size %s"), - res->disks[0].path); + virLockSystemError(-rv, + _("Unable to query sector size %s"), + res->disks[0].path); goto error_unlink; } @@ -588,9 +608,9 @@ static int virLockManagerSanlockCreateLease(struct sanlk_resource *res) _("Unable to initialize lease %s: error %d"), res->disks[0].path, rv); else - virReportSystemError(-rv, - _("Unable to initialize lease %s"), - res->disks[0].path); + virLockSystemError(-rv, + _("Unable to initialize lease %s"), + res->disks[0].path); goto error_unlink; } VIR_DEBUG("Lease %s has been initialized", res->disks[0].path); @@ -711,9 +731,9 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, _("Unable to parse lock state %s: error %d"), state, rv); else - virReportSystemError(-rv, - _("Unable to parse lock state %s"), - state); + virLockSystemError(-rv, + _("Unable to parse lock state %s"), + state); goto error; } res_free = true; @@ -736,8 +756,9 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, _("Failed to open socket to sanlock daemon: error %d"), sock); else - virReportSystemError(-sock, "%s", - _("Failed to open socket to sanlock daemon")); + virLockSystemError(-sock, "%s", + _("Failed to open socket to sanlock daemon")); + goto error; } @@ -750,8 +771,8 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, virLockError(VIR_ERR_INTERNAL_ERROR, _("Failed to acquire lock: error %d"), rv); else - virReportSystemError(-rv, "%s", - _("Failed to acquire lock")); + virLockSystemError(-rv, "%s", + _("Failed to acquire lock")); goto error; } } @@ -774,8 +795,8 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, virLockError(VIR_ERR_INTERNAL_ERROR, _("Failed to restrict process: error %d"), rv); else - virReportSystemError(-rv, "%s", - _("Failed to restrict process")); + virLockSystemError(-rv, "%s", + _("Failed to restrict process")); goto error; } } @@ -823,8 +844,8 @@ static int virLockManagerSanlockRelease(virLockManagerPtr lock, virLockError(VIR_ERR_INTERNAL_ERROR, _("Failed to inquire lock: error %d"), rv); else - virReportSystemError(-rv, "%s", - _("Failed to inquire lock")); + virLockSystemError(-rv, "%s", + _("Failed to inquire lock")); return -1; } @@ -837,8 +858,8 @@ static int virLockManagerSanlockRelease(virLockManagerPtr lock, virLockError(VIR_ERR_INTERNAL_ERROR, _("Failed to release lock: error %d"), rv); else - virReportSystemError(-rv, "%s", - _("Failed to release lock")); + virLockSystemError(-rv, "%s", + _("Failed to release lock")); return -1; } @@ -866,8 +887,8 @@ static int virLockManagerSanlockInquire(virLockManagerPtr lock, virLockError(VIR_ERR_INTERNAL_ERROR, _("Failed to inquire lock: error %d"), rv); else - virReportSystemError(-rv, "%s", - _("Failed to inquire lock")); + virLockSystemError(-rv, "%s", + _("Failed to inquire lock")); return -1; } -- 1.7.3.4
On Thu, Mar 15, 2012 at 05:23:09PM +0100, Peter Krempa wrote:
If the connection to the sanlock daemon is forbidden by selinux the error message was not clear enough. This patch adds a check if proper configuration for selinux is used while trying to connect to sanlock.
*src/locking/lock_driver_sanlock.c: - add macro virLockSystemError that checks for selinux and reports an improved error message - modify calls of virReportSystemError to the new macro in apropriate places
Background: https://bugzilla.redhat.com/show_bug.cgi?id=770488
IMHO this is not something we should do here. You're outputing the message regardless of whether there is even an NFS volume involved, and harcoding details of the SELinux policy. Finally I don't think we should blindly tell people to change SELinux tunables without explaining the implications, which is not practical in an error message. So, IMHO, this belongs in documentation, not in the error messages here. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/15/2012 10:35 AM, Daniel P. Berrange wrote:
On Thu, Mar 15, 2012 at 05:23:09PM +0100, Peter Krempa wrote:
If the connection to the sanlock daemon is forbidden by selinux the error message was not clear enough. This patch adds a check if proper configuration for selinux is used while trying to connect to sanlock.
*src/locking/lock_driver_sanlock.c: - add macro virLockSystemError that checks for selinux and reports an improved error message - modify calls of virReportSystemError to the new macro in apropriate places
Background: https://bugzilla.redhat.com/show_bug.cgi?id=770488
IMHO this is not something we should do here. You're outputing the message regardless of whether there is even an NFS volume involved, and harcoding details of the SELinux policy. Finally I don't think we should blindly tell people to change SELinux tunables without explaining the implications, which is not practical in an error message.
We've done this sort of targeted error message before; but there we were careful to _only_ issue the message after checking that we were indeed dealing with NFS; see commit 1888363d. [Hmm - should that patch be revisited, to mention virt_use_samba if it was samba rather than nfs that caused the SELinux denial, since those are different bools?]
So, IMHO, this belongs in documentation, not in the error messages here.
I think both are appropriate - we definitely need to mention virt_use_sanlock in locking.html.in, with full implications, but I would also like to see the error message, provided that you can be sure that the error message only mentions virt_use_sanlock in the actual case where SELinux is enforcing and the error is confirmed to be due to NFS causing us to need the bool. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Mar 15, 2012 at 10:55:20AM -0600, Eric Blake wrote:
On 03/15/2012 10:35 AM, Daniel P. Berrange wrote:
On Thu, Mar 15, 2012 at 05:23:09PM +0100, Peter Krempa wrote:
If the connection to the sanlock daemon is forbidden by selinux the error message was not clear enough. This patch adds a check if proper configuration for selinux is used while trying to connect to sanlock.
*src/locking/lock_driver_sanlock.c: - add macro virLockSystemError that checks for selinux and reports an improved error message - modify calls of virReportSystemError to the new macro in apropriate places
Background: https://bugzilla.redhat.com/show_bug.cgi?id=770488
IMHO this is not something we should do here. You're outputing the message regardless of whether there is even an NFS volume involved, and harcoding details of the SELinux policy. Finally I don't think we should blindly tell people to change SELinux tunables without explaining the implications, which is not practical in an error message.
We've done this sort of targeted error message before; but there we were careful to _only_ issue the message after checking that we were indeed dealing with NFS; see commit 1888363d. [Hmm - should that patch be revisited, to mention virt_use_samba if it was samba rather than nfs that caused the SELinux denial, since those are different bools?]
I don't really agree with that previous commit either for the same reasons.
So, IMHO, this belongs in documentation, not in the error messages here.
I think both are appropriate - we definitely need to mention virt_use_sanlock in locking.html.in, with full implications, but I would also like to see the error message, provided that you can be sure that the error message only mentions virt_use_sanlock in the actual case where SELinux is enforcing and the error is confirmed to be due to NFS causing us to need the bool.
I still don't think it belongs in the error message - with this kind of message, they'll just see the instruction & blindly follow it Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa