On 05/02/2012 05:44 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
sparse on the commit message (I sound like a broken record...)
> +++ b/src/Makefile.am
> @@ -537,7 +537,8 @@ ACCESS_DRIVER_SOURCES = \
> access/viraccessdriver.h \
> access/viraccessdrivernop.h access/viraccessdrivernop.c \
> access/viraccessdriverstack.h access/viraccessdriverstack.c \
> - access/viraccessdriverpolkit.h access/viraccessdriverpolkit.c
> + access/viraccessdriverpolkit.h access/viraccessdriverpolkit.c \
> + access/viraccessdriverselinux.h access/viraccessdriverselinux.c
selinux comes before stack, if you are sorting by name. Or, if you are
listing generic files first (driver, driverstack) followed by
implementations (drivernop, driverpolkit, driverselinux), then drivernop
is too early.
> +
> +struct _virAccessDriverSELinuxPrivate {
> + bool enabled;
This looks like you are doing a one-shot setup, based on the enabled
state at the time the struct is initialized. Normally, I can run
'setenforce [01]' on a live system, and it takes effect immediately,
without restarting processes; but if you do one-shot initialization
instead of live probing on every use, changing the permissions would
require a libvirtd restart. Or am I mixing up enabled vs. enforcing
(where enforcing can change on the fly, but only if enabled is true,
where enabled is one-shot at boot).
> +static int virAccessDriverSELinuxSetup(virAccessManagerPtr manager)
> +{
> + virAccessDriverSELinuxPrivatePtr priv = virAccessManagerGetPrivateData(manager);
> + int r;
> + security_context_t localCon;
Uninit...
> + int ret = -1;
> +
> + if ((r = is_selinux_enabled()) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to determine if SELinux is enabled"));
> + return -1;
> + }
> + priv->enabled = r != 0;
> + priv->localSid = SECSID_WILD;
> +
> + avc_entry_ref_init(&priv->aeref);
> +
> + if (avc_init("avc",
> + &virAccessDriverSELinuxAVCMemCallbacks,
> + &virAccessDriverSELinuxAVCLogCallbacks,
> + &virAccessDriverSELinuxAVCThreadCallbacks,
> + &virAccessDriverSELinuxAVCLockCallbacks) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to initialize AVC system"));
> + goto cleanup;
still uninit,
> + }
> +
> + if (getcon(&localCon) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to get context of daemon"));
> + goto cleanup;
> + }
> +
> + if (avc_context_to_sid(localCon, &priv->localSid) < 0) {
> + virReportSystemError(errno,
> + _("Unable to convert context %s to SID"),
> + (char*)localCon);
> + goto cleanup;
> + }
> + VIR_FREE(localCon);
> +
> + ret = 0;
> +cleanup:
> + if (ret < 0)
> + priv->localSid = SECSID_WILD;
> + freecon(localCon);
and unconditionally freed on error. Oops.
> +
> +
> +static access_vector_t
> +virAccessDriverSELinuxGetObjectPerm(const char *avname, const char *typename, security_class_t objectClass)
Long lines; worth wrapping?
> +
> +
> +static void virAccessDriverSELinuxAVCLog(const char *fmt, ...)
> +{
> + va_list ap;
> + va_start(ap, fmt);
> + virLogVMessage("avc", VIR_LOG_WARN,__func__, __LINE__, 0, fmt, ap);
This will always log with virAccessDriverSELinuxAVCLog as the function,
which isn't quite as useful as stating which function really logged.
For that to work, you'd need to maintain some sort of state stack, where
you push the name and line of a function before calling into any selinux
function that might trigger a callback, then this callback function
inspects the state stack. But that's a lot of work, I can live with
your current solution.
> +
> +static void *virAccessDriverSELinuxAVCCreateThread(void (*run) (void))
> +{
> + virThreadPtr thread;
> +
> + if (VIR_ALLOC(thread) < 0) {
> + virReportOOMError();
> + return NULL;
> + }
> +
> + if (virThreadCreate(thread, false, (virThreadFunc)run, NULL) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to create thread"));
> + VIR_FREE(thread);
> + }
> +
> + return thread;
> +}
> +
> +
> +static void virAccessDriverSELinuxAVCStopThread(void *thread)
> +{
> + virThreadCancel(thread);
> + VIR_FREE(thread);
Since thread cancellation introduces such a different paradigm in how
you code (such as having to properly push and pop cancellation cleanup
code), maybe it would make sense to alter your 2/12 patch to add a
virThreadCreateFlags, and _only_ allow a thread to be cancelled if it
passes in a flag that states it is cancellation-safe. This code would
be the first client of that mode, where virThreadCancel() would fail if
called on any other thread that has not already been audited and marked
to be cancellation-safe. That is, I'd feel safer if the only threads
that we allow libvirt to cancel are those that are known to be
cancellation safe (such as the selinux thread, since it requires
cancellation to work, so it must be safe insofar as all our callbacks
are also cancellation safe).
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list