Re: [PATCHv6 01/36] ns: Introduce Time Namespace
Andrei,
On Thu, 15 Aug 2019, Andrei Vagin wrote:
> On Thu, Aug 15, 2019 at 07:19:12PM +0200, Thomas Gleixner wrote:
> > > +static int timens_install(struct nsproxy *nsproxy, struct ns_common *new)
> > > +{
> > > + struct time_namespace *ns = to_time_ns(new);
> > > +
> > > + if (!current_is_single_threaded())
> > > + return -EUSERS;
> > > +
> > > + if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN) ||
> > > + !ns_capable(current_user_ns(), CAP_SYS_ADMIN))
> > > + return -EPERM;
> > > +
> > > + get_time_ns(ns);
> > > + get_time_ns(ns);
> >
> > Why is this a double get?
>
> Because we change nsproxy->time_ns and nsproxy->time_ns_for_children.
>
> Probably, I need to reorgonize the code this way:
>
> get_time_ns(ns);
> put_time_ns(nsproxy->time_ns);
> nsproxy->time_ns = ns;
>
> get_time_ns(ns);
> put_time_ns(nsproxy->time_ns_for_children);
> nsproxy->time_ns_for_children = ns;
That's better. A few comments about refcounting might be useful as well.
> > > + put_time_ns(nsproxy->time_ns);
> > > + put_time_ns(nsproxy->time_ns_for_children);
> > > + nsproxy->time_ns = ns;
> > > + nsproxy->time_ns_for_children = ns;
> > > + ns->initialized = true;
> > > + return 0;
> > > +}
> > > +
> > > +int timens_on_fork(struct nsproxy *nsproxy, struct task_struct *tsk)
> > > +{
> > > + struct ns_common *nsc = &nsproxy->time_ns_for_children->ns;
> > > + struct time_namespace *ns = to_time_ns(nsc);
> > > +
> > > + if (nsproxy->time_ns == nsproxy->time_ns_for_children)
> > > + return 0;
Doesn't this need to take a refcount on fork? Maybe not, but see below.
> > > +
> > > + get_time_ns(ns);
> > > + put_time_ns(nsproxy->time_ns);
> > > + nsproxy->time_ns = ns;
> > > + ns->initialized = true;
> >
> > Isn't that one initialized already?
>
> When we discussed time namespaces last year, we decided that clock
> offsets can be set only before any task enters a namespace.
>
> When a namespace is created, ns->initialized is set to false. When a
> task enters the namespace, ns->initialized is set to true.
Right. I'm probably just hopelessly confused by this nsproxy->time_ns
vs. nsproxy->time_ns_for_children->ns thing.
> Namespace clock offsets can be changed only if ns->initialized is false.
>
> A new task can be forked to a specified time namespace or it can call
> setns, so we set ns->initialized to true in timens_on_fork and
> timens_install.
Some comments explaining that logic above would be really helpful.
Thanks,
tglx
Re: [PATCHv6 01/36] ns: Introduce Time Namespace
On Thu, Aug 15, 2019 at 07:19:12PM +0200, Thomas Gleixner wrote:
> Dmitry,
>
> On Thu, 15 Aug 2019, Dmitry Safonov wrote:
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 420567d1519a..97b7737f5aba 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12898,6 +12898,8 @@ T: git
> > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
> > S: Maintained
> > F: fs/timerfd.c
> > F: include/linux/timer*
> > +F: include/linux/time_namespace.h
> > +F: kernel/time_namespace.c
>
> Shouldn't this be kernel/time/namespace.c so all that stuff is lumped
> together. No strong opinion though.
Sure, we can move this in kernel/time. I don't remember why I decided to
place it in kernel/.
>
> > +++ b/kernel/time_namespace.c
> > +static struct ucounts *inc_time_namespaces(struct user_namespace *ns)
> > +{
> > + return inc_ucount(ns, current_euid(), UCOUNT_TIME_NAMESPACES);
> > +}
> > +
> > +static void dec_time_namespaces(struct ucounts *ucounts)
> > +{
> > + dec_ucount(ucounts, UCOUNT_TIME_NAMESPACES);
> > +}
> > +
> > +static struct time_namespace *create_time_ns(void)
> > +{
> > + struct time_namespace *time_ns;
> > +
> > + time_ns = kmalloc(sizeof(struct time_namespace), GFP_KERNEL);
>
> Shouldn't this use kzalloc()? There are tons of members in that struct.
I don't think so. All of other members are initialized in clone_time_ns.
Maybe we don't need this helper. When I wrote this code, I looked at
kernel/utsname.c. I will think what we can do here to make this code
more clear.
>
> > + if (time_ns) {
> > + kref_init(&time_ns->kref);
> > + time_ns->initialized = false;
>
> And you spare this one.
This one should be initialized in clone_time_ns too.
>
> > + }
> > + return time_ns;
> > +}
> > +
> > +/*
> > + * Clone a new ns copying @old_ns, setting refcount to 1
> > + * @old_ns: namespace to clone
> > + * Return the new ns or ERR_PTR.
>
> If you use kernel-doc style then please use te proper syntax
>
> /*
> * clone_time_ns - Clone a time namespace
> * @old_ns: Namespace to clone
> *
> * Clone @old_ns and set the clone refcount to 1
> *
> * Return: The new namespace or ERR_PTR.
> */
Will fix. Thanks!
>
> > + */
> > +static struct time_namespace *clone_time_ns(struct user_namespace *user_ns,
> > + struct time_namespace *old_ns)
> > +{
> > + struct time_namespace *ns;
> > + struct ucounts *ucounts;
> > + int err;
> > +
> > + err = -ENOSPC;
> > + ucounts = inc_time_namespaces(user_ns);
> > + if (!ucounts)
> > + goto fail;
> > +
> > + err = -ENOMEM;
> > + ns = create_time_ns();
> > + if (!ns)
> > + goto fail_dec;
> > +
> > + err = ns_alloc_inum(&ns->ns);
> > + if (err)
> > + goto fail_free;
> > +
> > + ns->ucounts = ucounts;
> > + ns->ns.ops = &timens_operations;
> > + ns->user_ns = get_user_ns(user_ns);
> > + return ns;
> > +
> > +fail_free:
> > + kfree(ns);
> > +fail_dec:
> > + dec_time_namespaces(ucounts);
> > +fail:
> > + return ERR_PTR(err);
> > +}
> > +
> > +/*
> > + * Add a reference to old_ns, or clone it if @flags specify CLONE_NEWTIME.
> > + * In latter case, changes to the time of this process won't be seen by
> > parent,
> > + * and vice versa.
>
> Ditto
Will fix.
>
> > + */
> > +struct time_namespace *copy_time_ns(unsigned long flags,
> > + struct user_namespace *user_ns, struct time_namespace *old_ns)
> > +{
> > + if (!(flags & CLONE_NEWTIME))
> > + return get_time_ns(old_ns);
> > +
> > + return clone_time_ns(user_ns, old_ns);
> > +}
> > +
> > +void free_time_ns(struct kref *kref)
> > +{
> > + struct time_namespace *ns;
> > +
> > + ns = container_of(kref, struct time_namespace, kref);
> > + dec_time_namespaces(ns->ucounts);
> > + put_user_ns(ns->user_ns);
> > + ns_free_inum(&ns->ns);
> > + kfree(ns);
> > +}
> > +
> > +static struct time_namespace *to_time_ns(struct ns_common *ns)
> > +{
> > + return container_of(ns, struct time_namespace, ns);
> > +}
> > +
> > +static struct ns_common *timens_get(struct task_struct *task)
> > +{
> > + struct time_namespace *ns = NULL;
> > + struct nsproxy *nsproxy;
> > +
> > + task_lock(task);
> > + nsproxy = task->nsproxy;
> > + if (nsproxy) {
> > + ns = nsproxy->time_ns;
> > + get_time_ns(ns);
> > + }
> > + task_unlock(task);
> > +
> > + return ns ? &ns->ns : NULL;
> > +}
> > +
> > +static struct ns_common *timens_for_children_get(struct task_struct *task)
> > +{
> > + struct time_namespace *ns = NULL;
> > + struct nsproxy *nsproxy;
> > +
> > + task_lock(task);
> > + nsproxy = task->nsproxy;
> > + if (nsproxy) {
> > + ns = nsproxy->time_ns_for_children;
> > + get_time_ns(ns);
> > + }
> > + task_unlock(task);
> > +
> > + return ns ? &ns->ns : NULL;
> > +}
> > +
> > +static void timens_put(struct ns_common *ns)
> > +{
> > + put_time_ns(to_time_ns(ns));
> > +}
> > +
> > +static int timen
Re: [PATCHv6 01/36] ns: Introduce Time Namespace
Dmitry,
On Thu, 15 Aug 2019, Dmitry Safonov wrote:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 420567d1519a..97b7737f5aba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12898,6 +12898,8 @@ T:git
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
> S: Maintained
> F: fs/timerfd.c
> F: include/linux/timer*
> +F: include/linux/time_namespace.h
> +F: kernel/time_namespace.c
Shouldn't this be kernel/time/namespace.c so all that stuff is lumped
together. No strong opinion though.
> +++ b/kernel/time_namespace.c
> +static struct ucounts *inc_time_namespaces(struct user_namespace *ns)
> +{
> + return inc_ucount(ns, current_euid(), UCOUNT_TIME_NAMESPACES);
> +}
> +
> +static void dec_time_namespaces(struct ucounts *ucounts)
> +{
> + dec_ucount(ucounts, UCOUNT_TIME_NAMESPACES);
> +}
> +
> +static struct time_namespace *create_time_ns(void)
> +{
> + struct time_namespace *time_ns;
> +
> + time_ns = kmalloc(sizeof(struct time_namespace), GFP_KERNEL);
Shouldn't this use kzalloc()? There are tons of members in that struct.
> + if (time_ns) {
> + kref_init(&time_ns->kref);
> + time_ns->initialized = false;
And you spare this one.
> + }
> + return time_ns;
> +}
> +
> +/*
> + * Clone a new ns copying @old_ns, setting refcount to 1
> + * @old_ns: namespace to clone
> + * Return the new ns or ERR_PTR.
If you use kernel-doc style then please use te proper syntax
/*
* clone_time_ns - Clone a time namespace
* @old_ns: Namespace to clone
*
* Clone @old_ns and set the clone refcount to 1
*
* Return: The new namespace or ERR_PTR.
*/
> + */
> +static struct time_namespace *clone_time_ns(struct user_namespace *user_ns,
> + struct time_namespace *old_ns)
> +{
> + struct time_namespace *ns;
> + struct ucounts *ucounts;
> + int err;
> +
> + err = -ENOSPC;
> + ucounts = inc_time_namespaces(user_ns);
> + if (!ucounts)
> + goto fail;
> +
> + err = -ENOMEM;
> + ns = create_time_ns();
> + if (!ns)
> + goto fail_dec;
> +
> + err = ns_alloc_inum(&ns->ns);
> + if (err)
> + goto fail_free;
> +
> + ns->ucounts = ucounts;
> + ns->ns.ops = &timens_operations;
> + ns->user_ns = get_user_ns(user_ns);
> + return ns;
> +
> +fail_free:
> + kfree(ns);
> +fail_dec:
> + dec_time_namespaces(ucounts);
> +fail:
> + return ERR_PTR(err);
> +}
> +
> +/*
> + * Add a reference to old_ns, or clone it if @flags specify CLONE_NEWTIME.
> + * In latter case, changes to the time of this process won't be seen by
> parent,
> + * and vice versa.
Ditto
> + */
> +struct time_namespace *copy_time_ns(unsigned long flags,
> + struct user_namespace *user_ns, struct time_namespace *old_ns)
> +{
> + if (!(flags & CLONE_NEWTIME))
> + return get_time_ns(old_ns);
> +
> + return clone_time_ns(user_ns, old_ns);
> +}
> +
> +void free_time_ns(struct kref *kref)
> +{
> + struct time_namespace *ns;
> +
> + ns = container_of(kref, struct time_namespace, kref);
> + dec_time_namespaces(ns->ucounts);
> + put_user_ns(ns->user_ns);
> + ns_free_inum(&ns->ns);
> + kfree(ns);
> +}
> +
> +static struct time_namespace *to_time_ns(struct ns_common *ns)
> +{
> + return container_of(ns, struct time_namespace, ns);
> +}
> +
> +static struct ns_common *timens_get(struct task_struct *task)
> +{
> + struct time_namespace *ns = NULL;
> + struct nsproxy *nsproxy;
> +
> + task_lock(task);
> + nsproxy = task->nsproxy;
> + if (nsproxy) {
> + ns = nsproxy->time_ns;
> + get_time_ns(ns);
> + }
> + task_unlock(task);
> +
> + return ns ? &ns->ns : NULL;
> +}
> +
> +static struct ns_common *timens_for_children_get(struct task_struct *task)
> +{
> + struct time_namespace *ns = NULL;
> + struct nsproxy *nsproxy;
> +
> + task_lock(task);
> + nsproxy = task->nsproxy;
> + if (nsproxy) {
> + ns = nsproxy->time_ns_for_children;
> + get_time_ns(ns);
> + }
> + task_unlock(task);
> +
> + return ns ? &ns->ns : NULL;
> +}
> +
> +static void timens_put(struct ns_common *ns)
> +{
> + put_time_ns(to_time_ns(ns));
> +}
> +
> +static int timens_install(struct nsproxy *nsproxy, struct ns_common *new)
> +{
> + struct time_namespace *ns = to_time_ns(new);
> +
> + if (!current_is_single_threaded())
> + return -EUSERS;
> +
> + if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN) ||
> + !ns_capable(current_user_ns(), CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + get_time_ns(ns);
> + get_time_ns(ns);
Why is this a double get?
> + put_time_ns(nsproxy->time_ns);
> + put_time_ns(nsproxy->time_ns_for_children);
> + nsproxy->time_ns = ns;
> + nsproxy->time_ns_for_children = ns;
> + ns->initialized = true;
> + return 0;
> +}
> +
> +int timens_on_fork
[PATCHv6 01/36] ns: Introduce Time Namespace
From: Andrei Vagin
Time Namespace isolates clock values.
The kernel provides access to several clocks CLOCK_REALTIME,
CLOCK_MONOTONIC, CLOCK_BOOTTIME, etc.
CLOCK_REALTIME
System-wide clock that measures real (i.e., wall-clock) time.
CLOCK_MONOTONIC
Clock that cannot be set and represents monotonic time since
some unspecified starting point.
CLOCK_BOOTTIME
Identical to CLOCK_MONOTONIC, except it also includes any time
that the system is suspended.
For many users, the time namespace means the ability to changes date and
time in a container (CLOCK_REALTIME).
But in a context of the checkpoint/restore functionality, monotonic and
bootime clocks become interesting. Both clocks are monotonic with
unspecified staring points. These clocks are widely used to measure time
slices and set timers. After restoring or migrating processes, we have to
guarantee that they never go backward. In an ideal case, the behavior of
these clocks should be the same as for a case when a whole system is
suspended. All this means that we need to be able to set CLOCK_MONOTONIC
and CLOCK_BOOTTIME clocks, what can be done by adding per-namespace
offsets for clocks.
A time namespace is similar to a pid namespace in a way how it is
created: unshare(CLONE_NEWTIME) system call creates a new time namespace,
but doesn't set it to the current process. Then all children of
the process will be born in the new time namespace, or a process can
use the setns() system call to join a namespace.
This scheme allows setting clock offsets for a namespace, before any
processes appear in it.
All available clone flags have been used, so CLONE_NEWTIME uses the
highest bit of CSIGNAL. It means that we can use it with the unshare
system call only. Rith now, this works for us, because time namespace
offsets can be set only when a new time namespace is not populated. In a
future, we will have the clone3 system call [1] which will allow to use
the CSIGNAL mask for clone flags.
[1]: httmps://lkml.kernel.org/r/20190604160944.4058-1-christ...@brauner.io
Link: https://criu.org/Time_namespace
Link: https://lists.openvz.org/pipermail/criu/2018-June/041504.html
Signed-off-by: Andrei Vagin
Co-developed-by: Dmitry Safonov
Signed-off-by: Dmitry Safonov
---
MAINTAINERS| 2 +
fs/proc/namespaces.c | 4 +
include/linux/nsproxy.h| 2 +
include/linux/proc_ns.h| 3 +
include/linux/time_namespace.h | 69 +++
include/linux/user_namespace.h | 1 +
include/uapi/linux/sched.h | 6 +
init/Kconfig | 7 ++
kernel/Makefile| 1 +
kernel/fork.c | 16 ++-
kernel/nsproxy.c | 41 +--
kernel/time_namespace.c| 217 +
12 files changed, 359 insertions(+), 10 deletions(-)
create mode 100644 include/linux/time_namespace.h
create mode 100644 kernel/time_namespace.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 420567d1519a..97b7737f5aba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12898,6 +12898,8 @@ T: git
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
S: Maintained
F: fs/timerfd.c
F: include/linux/timer*
+F: include/linux/time_namespace.h
+F: kernel/time_namespace.c
F: kernel/time/*timer*
POWER MANAGEMENT CORE
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index dd2b35f78b09..8b5c720fe5d7 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -33,6 +33,10 @@ static const struct proc_ns_operations *ns_entries[] = {
#ifdef CONFIG_CGROUPS
&cgroupns_operations,
#endif
+#ifdef CONFIG_TIME_NS
+ &timens_operations,
+ &timens_for_children_operations,
+#endif
};
static const char *proc_ns_get_link(struct dentry *dentry,
diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
index 2ae1b1a4d84d..074f395b9ad2 100644
--- a/include/linux/nsproxy.h
+++ b/include/linux/nsproxy.h
@@ -35,6 +35,8 @@ struct nsproxy {
struct mnt_namespace *mnt_ns;
struct pid_namespace *pid_ns_for_children;
struct net *net_ns;
+ struct time_namespace *time_ns;
+ struct time_namespace *time_ns_for_children;
struct cgroup_namespace *cgroup_ns;
};
extern struct nsproxy init_nsproxy;
diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
index d31cb6215905..d312e6281e69 100644
--- a/include/linux/proc_ns.h
+++ b/include/linux/proc_ns.h
@@ -32,6 +32,8 @@ extern const struct proc_ns_operations
pidns_for_children_operations;
extern const struct proc_ns_operations userns_operations;
extern const struct proc_ns_operations mntns_operations;
extern const struct proc_ns_operations cgroupns_operations;
+extern const struct proc_ns_operations timens_operations;
+extern const struct proc_ns_operations timens_for_children_operations;
/*
* We always define these enumerators
@@ -43,6 +45,7 @@ enum {
PROC_USER_INIT_INO = 0xEFF
