[PATCH v4 1/5] lxc: Add Real Time Clock device into allowed devices

Daniel P. Berrangé berrange at redhat.com
Tue Feb 25 12:22:51 UTC 2020


On Mon, Feb 24, 2020 at 11:24:24AM -0300, Julio Faracco wrote:
> This commit share host Real Time Clock device (rtc) into LXC containers
> to support hardware clock. This should be available setting up a `rtc`
> timer under clock section. Since this option is not emulated, it should
> be available only for `localtime` clock. This option should be readonly
> due to security reasons.
> 
> Before:
>     root# hwclock --verbose
>     hwclock from util-linux 2.32.1
>     System Time: 1581877557.598365
>     Trying to open: /dev/rtc0
>     Trying to open: /dev/rtc
>     Trying to open: /dev/misc/rtc
>     No usable clock interface found.
>     hwclock: Cannot access the Hardware Clock via any known method.
> 
> Now:
>     root# hwclock
>     2020-02-16 18:23:55.374134+00:00
>     root# hwclock -w
>     hwclock: ioctl(RTC_SET_TIME) to /dev/rtc to set the time failed:
>     Permission denied
> 
> Signed-off-by: Julio Faracco <jcfaracco at gmail.com>
> ---
>  docs/formatdomain.html.in |  2 +-
>  src/lxc/lxc_cgroup.c      | 36 +++++++++++++++++++++
>  src/lxc/lxc_controller.c  | 68 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 4fef2a0a97..5598bf41b4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2465,7 +2465,7 @@
>              being modified, and can be one of
>              "platform" (currently unsupported),
>              "hpet" (libxl, xen, qemu), "kvmclock" (qemu),
> -            "pit" (qemu), "rtc" (qemu), "tsc" (libxl, qemu -
> +            "pit" (qemu), "rtc" (qemu, lxc), "tsc" (libxl, qemu -
>              <span class="since">since 3.2.0</span>), "hypervclock"
>              (qemu - <span class="since">since 1.2.2</span>) or
>              "armvtimer" (qemu - <span class="since">since 6.1.0</span>).
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index 4ebe5ef467..6a103055a4 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -337,6 +337,42 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
>                               VIR_CGROUP_DEVICE_RWM) < 0)
>          return -1;
>  
> +    VIR_DEBUG("Allowing timers char devices");
> +
> +    /* Sync'ed with Host clock */
> +    if (def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME) {
> +        for (i = 0; i < def->clock.ntimers; i++) {
> +            virDomainTimerDefPtr timer = def->clock.timers[i];
> +
> +            switch ((virDomainTimerNameType)timer->name) {
> +            case VIR_DOMAIN_TIMER_NAME_PLATFORM:
> +            case VIR_DOMAIN_TIMER_NAME_TSC:
> +            case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
> +            case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
> +            case VIR_DOMAIN_TIMER_NAME_PIT:
> +            case VIR_DOMAIN_TIMER_NAME_HPET:
> +            case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
> +            case VIR_DOMAIN_TIMER_NAME_LAST:
> +                break;
> +            case VIR_DOMAIN_TIMER_NAME_RTC:
> +                if (!timer->present)
> +                    break;

This field is not a boolean, it is a tri-state.

 * -1 == the hypervisor default - historically with LXC this means
         no /dev/rtc device
 * 0 == disable it  (matches the default for LXC historically)
 * 1 == enable it

So this check needs to be "if (timer->present != 1) "

> +
> +                if (virFileExists("/dev/rtc")) {
> +                    if (virCgroupAllowDevicePath(cgroup, "/dev/rtc",
> +                                                 VIR_CGROUP_DEVICE_READ,
> +                                                 false) < 0)
> +                        return -1;
> +                } else {
> +                    VIR_DEBUG("Ignoring non-existent device /dev/rtc");

We should be raising an error if the user requested RTC and it doesn't
exist.

> +                }
> +                break;
> +            }
> +        }
> +    } else {
> +        VIR_DEBUG("Ignoring non-localtime clock");
> +    }

The setup for <timer> elements should be independant of whether or
not localtime is set.

Ideally we would raise an unsupported config error for containers
which don't have localtime set, but there's way too large a risk
of causing regressions if we do this.

I'd suggest we just don't check the clock offset at all, or at
least do the check independantly of the timer setup.

> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index c3dec0859c..eba6bfe0bf 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -1550,6 +1550,71 @@ static int virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
>  }
>  
>  
> +static int
> +virLXCControllerSetupTimers(virLXCControllerPtr ctrl)
> +{
> +    g_autofree char *path = NULL;
> +    size_t i;
> +    struct stat sb;
> +    virDomainDefPtr def = ctrl->def;
> +
> +    /* Not sync'ed with Host clock */
> +    if (def->clock.offset != VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME)
> +        return 0;
> +
> +    for (i = 0; i < def->clock.ntimers; i++) {
> +        dev_t dev;
> +        virDomainTimerDefPtr timer = def->clock.timers[i];
> +
> +        switch ((virDomainTimerNameType)timer->name) {
> +        case VIR_DOMAIN_TIMER_NAME_PLATFORM:
> +        case VIR_DOMAIN_TIMER_NAME_TSC:
> +        case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
> +        case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
> +        case VIR_DOMAIN_TIMER_NAME_PIT:
> +        case VIR_DOMAIN_TIMER_NAME_HPET:
> +        case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
> +        case VIR_DOMAIN_TIMER_NAME_LAST:
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unsupported timer type (name) '%s'"),
> +                           virDomainTimerNameTypeToString(timer->name));
> +            return -1;
> +        case VIR_DOMAIN_TIMER_NAME_RTC:
> +            if (!timer->present)
> +                break;

Same comments as previously.

> +
> +            if (stat("/dev/rtc", &sb) < 0) {
> +                if (errno == EACCES)
> +                    return -1;

Why this special case ?  You're returning an error here without
setting any error message which is certainly not right.

> +
> +                virReportSystemError(errno,
> +                                     _("Path '%s' is not accessible"),
> +                                     path);
> +                return -1;
> +            }

No error reporting if /dev/rtc is missing.

> +
> +            path = g_strdup_printf("/%s/%s.dev/%s", LXC_STATE_DIR,
> +                                   ctrl->def->name, "/rtc");
> +
> +            dev = makedev(major(sb.st_rdev), minor(sb.st_rdev));
> +            if (mknod(path, S_IFCHR, dev) < 0 ||
> +                chmod(path, sb.st_mode)) {
> +                virReportSystemError(errno,
> +                                     _("Failed to make device %s"),
> +                                     path);
> +                return -1;
> +            }
> +
> +            if (lxcContainerChown(ctrl->def, path) < 0)
> +                return -1;
> +            break;
> +        }
> +    }
> +
> +    return 0;
> +}

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|




More information about the libvir-list mailing list