[PATCH] timens: show clock symbolic names in /proc/pid/timens_offsets

[PATCH] timens: show clock symbolic names in /proc/pid/timens_offsets

[Andrei Vagin]

Michael Kerrisk suggested to replace numeric clock IDs on symbolic
names.

Now the content of these files looks like this:
$ cat /proc/5362/timens_offsets
monotonic	   9504000	         0
boottime	   3456000	         0

For setting offsets, both representations of clocks can be used.

As for compatibility, it is acceptable to change things as long as
userspace doesn't care. The format of timens_offsets files is very
new and there are no userspace tools that rely on this format.

But three projects crun, util-linux and criu rely on the interface of
setting time offsets and this is why we need to continue supporting the
clock IDs in this case.

Fixes: 04a8682a71be ("fs/proc: Introduce /proc/pid/timens_offsets")
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Suggested-by: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 fs/proc/base.c          | 14 +++++++++++++-
 kernel/time/namespace.c | 15 ++++++++++++++-
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6042b646ab27..572898dd16a0 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1573,6 +1573,7 @@ static ssize_t timens_offsets_write(struct file *file, const char __user *buf,
 	noffsets = 0;
 	for (pos = kbuf; pos; pos = next_line) {
 		struct proc_timens_offset *off = &offsets[noffsets];
+		char clock[10];
 		int err;
 
 		/* Find the end of line and ensure we don't look past it */
@@ -1584,10 +1585,21 @@ static ssize_t timens_offsets_write(struct file *file, const char __user *buf,
 				next_line = NULL;
 		}
 
-		err = sscanf(pos, "%u %lld %lu", &off->clockid,
+		err = sscanf(pos, "%9s %lld %lu", clock,
 				&off->val.tv_sec, &off->val.tv_nsec);
 		if (err != 3 || off->val.tv_nsec >= NSEC_PER_SEC)
 			goto out;
+
+		clock[sizeof(clock) - 1] = 0;
+		if (strcmp(clock, "monotonic") == 0 ||
+		    strcmp(clock, __stringify(CLOCK_MONOTONIC)) == 0)
+			off->clockid = CLOCK_MONOTONIC;
+		else if (strcmp(clock, "boottime") == 0 ||
+			 strcmp(clock, __stringify(CLOCK_BOOTTIME)) == 0)
+			off->clockid = CLOCK_BOOTTIME;
+		else
+			goto out;
+
 		noffsets++;
 		if (noffsets == ARRAY_SIZE(offsets)) {
 			if (next_line)
diff --git a/kernel/time/namespace.c b/kernel/time/namespace.c
index e6ba064ce773..8127d2647064 100644
--- a/kernel/time/namespace.c
+++ b/kernel/time/namespace.c
@@ -338,7 +338,20 @@ static struct user_namespace *timens_owner(struct ns_common *ns)
 
 static void show_offset(struct seq_file *m, int clockid, struct timespec64 *ts)
 {
-	seq_printf(m, "%d %lld %ld\n", clockid, ts->tv_sec, ts->tv_nsec);
+	char *clock;
+
+	switch (clockid) {
+	case CLOCK_BOOTTIME:
+		clock = "boottime";
+		break;
+	case CLOCK_MONOTONIC:
+		clock = "monotonic";
+		break;
+	default:
+		clock = "unknown";
+		break;
+	}
+	seq_printf(m, "%s\t%10lld\t%10ld\n", clock, ts->tv_sec, ts->tv_nsec);
 }
 
 void proc_timens_show_offsets(struct task_struct *p, struct seq_file *m)
-- 
2.24.1

Re: [PATCH] timens: show clock symbolic names in /proc/pid/timens_offsets

[Michael Kerrisk (man-pages)]

Hello Andrei,

On 4/11/20 8:52 AM, Andrei Vagin wrote:
> Michael Kerrisk suggested to replace numeric clock IDs on symbolic
> names.
> 
> Now the content of these files looks like this:
> $ cat /proc/5362/timens_offsets
> monotonic	   9504000	         0
> boottime	   3456000	         0
> 
> For setting offsets, both representations of clocks can be used.
> 
> As for compatibility, it is acceptable to change things as long as
> userspace doesn't care. The format of timens_offsets files is very
> new and there are no userspace tools that rely on this format.
> 
> But three projects crun, util-linux and criu rely on the interface of
> setting time offsets and this is why we need to continue supporting the
> clock IDs in this case.

Thanks very much for this patch, which I've tested. So:

Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
Tested-by: Michael Kerrisk <mtk.manpages@gmail.com>

But, I do have one small suggestion below.

> Fixes: 04a8682a71be ("fs/proc: Introduce /proc/pid/timens_offsets")
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Dmitry Safonov <0x7f454c46@gmail.com>
> Suggested-by: Michael Kerrisk <mtk.manpages@gmail.com>
> Signed-off-by: Andrei Vagin <avagin@gmail.com>
> ---
>  fs/proc/base.c          | 14 +++++++++++++-
>  kernel/time/namespace.c | 15 ++++++++++++++-
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6042b646ab27..572898dd16a0 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1573,6 +1573,7 @@ static ssize_t timens_offsets_write(struct file *file, const char __user *buf,
>  	noffsets = 0;
>  	for (pos = kbuf; pos; pos = next_line) {
>  		struct proc_timens_offset *off = &offsets[noffsets];
> +		char clock[10];
>  		int err;
>  
>  		/* Find the end of line and ensure we don't look past it */
> @@ -1584,10 +1585,21 @@ static ssize_t timens_offsets_write(struct file *file, const char __user *buf,
>  				next_line = NULL;
>  		}
>  
> -		err = sscanf(pos, "%u %lld %lu", &off->clockid,
> +		err = sscanf(pos, "%9s %lld %lu", clock,
>  				&off->val.tv_sec, &off->val.tv_nsec);
>  		if (err != 3 || off->val.tv_nsec >= NSEC_PER_SEC)
>  			goto out;
> +
> +		clock[sizeof(clock) - 1] = 0;
> +		if (strcmp(clock, "monotonic") == 0 ||
> +		    strcmp(clock, __stringify(CLOCK_MONOTONIC)) == 0)
> +			off->clockid = CLOCK_MONOTONIC;
> +		else if (strcmp(clock, "boottime") == 0 ||
> +			 strcmp(clock, __stringify(CLOCK_BOOTTIME)) == 0)
> +			off->clockid = CLOCK_BOOTTIME;
> +		else
> +			goto out;
> +
>  		noffsets++;
>  		if (noffsets == ARRAY_SIZE(offsets)) {
>  			if (next_line)
> diff --git a/kernel/time/namespace.c b/kernel/time/namespace.c
> index e6ba064ce773..8127d2647064 100644
> --- a/kernel/time/namespace.c
> +++ b/kernel/time/namespace.c
> @@ -338,7 +338,20 @@ static struct user_namespace *timens_owner(struct ns_common *ns)
>  
>  static void show_offset(struct seq_file *m, int clockid, struct timespec64 *ts)
>  {
> -	seq_printf(m, "%d %lld %ld\n", clockid, ts->tv_sec, ts->tv_nsec);
> +	char *clock;
> +
> +	switch (clockid) {
> +	case CLOCK_BOOTTIME:
> +		clock = "boottime";
> +		break;
> +	case CLOCK_MONOTONIC:
> +		clock = "monotonic";
> +		break;
> +	default:
> +		clock = "unknown";
> +		break;
> +	}

As things stand, there is to my eye an excessive amount of white space
in the output produced by this line:

> +	seq_printf(m, "%s\t%10lld\t%10ld\n", clock, ts->tv_sec, ts->tv_nsec);

Can I suggest instead something like:

	seq_printf(m, "%-16s %10lld %9ld\n", clock, ts->tv_sec, ts->tv_nsec);

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] timens: show clock symbolic names in /proc/pid/timens_offsets

[Michael Kerrisk (man-pages)]

> As things stand, there is to my eye an excessive amount of white space
> in the output produced by this line:
> 
>> +	seq_printf(m, "%s\t%10lld\t%10ld\n", clock, ts->tv_sec, ts->tv_nsec);
> 
> Can I suggest instead something like:
> 
> 	seq_printf(m, "%-16s %10lld %9ld\n", clock, ts->tv_sec, ts->tv_nsec);

Actually, how about even s/-16s/-10s/ for that first field.

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

[PATCH v2] timens: show clock symbolic names in /proc/pid/timens_offsets

[Andrei Vagin]

Michael Kerrisk suggested to replace numeric clock IDs on symbolic
names.

Now the content of these files looks like this:
$ cat /proc/774/timens_offsets
monotonic      864000         0
boottime      1728000         0

For setting offsets, both representations of clocks can be used.

As for compatibility, it is acceptable to change things as long as
userspace doesn't care. The format of timens_offsets files is very
new and there are no userspace tools that rely on this format.

But three projects crun, util-linux and criu rely on the interface of
setting time offsets and this is why we need to continue supporting the
clock IDs in this case.

Fixes: 04a8682a71be ("fs/proc: Introduce /proc/pid/timens_offsets")
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>                                                                                                                                            
Tested-by: Michael Kerrisk <mtk.manpages@gmail.com> 
Suggested-by: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Andrei Vagin <avagin@gmail.com>
---

v2: use the more compact format of timens_offsets files.

 fs/proc/base.c          | 14 +++++++++++++-
 kernel/time/namespace.c | 15 ++++++++++++++-
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6042b646ab27..572898dd16a0 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1573,6 +1573,7 @@ static ssize_t timens_offsets_write(struct file *file, const char __user *buf,
 	noffsets = 0;
 	for (pos = kbuf; pos; pos = next_line) {
 		struct proc_timens_offset *off = &offsets[noffsets];
+		char clock[10];
 		int err;
 
 		/* Find the end of line and ensure we don't look past it */
@@ -1584,10 +1585,21 @@ static ssize_t timens_offsets_write(struct file *file, const char __user *buf,
 				next_line = NULL;
 		}
 
-		err = sscanf(pos, "%u %lld %lu", &off->clockid,
+		err = sscanf(pos, "%9s %lld %lu", clock,
 				&off->val.tv_sec, &off->val.tv_nsec);
 		if (err != 3 || off->val.tv_nsec >= NSEC_PER_SEC)
 			goto out;
+
+		clock[sizeof(clock) - 1] = 0;
+		if (strcmp(clock, "monotonic") == 0 ||
+		    strcmp(clock, __stringify(CLOCK_MONOTONIC)) == 0)
+			off->clockid = CLOCK_MONOTONIC;
+		else if (strcmp(clock, "boottime") == 0 ||
+			 strcmp(clock, __stringify(CLOCK_BOOTTIME)) == 0)
+			off->clockid = CLOCK_BOOTTIME;
+		else
+			goto out;
+
 		noffsets++;
 		if (noffsets == ARRAY_SIZE(offsets)) {
 			if (next_line)
diff --git a/kernel/time/namespace.c b/kernel/time/namespace.c
index e6ba064ce773..757581d64246 100644
--- a/kernel/time/namespace.c
+++ b/kernel/time/namespace.c
@@ -338,7 +338,20 @@ static struct user_namespace *timens_owner(struct ns_common *ns)
 
 static void show_offset(struct seq_file *m, int clockid, struct timespec64 *ts)
 {
-	seq_printf(m, "%d %lld %ld\n", clockid, ts->tv_sec, ts->tv_nsec);
+	char *clock;
+
+	switch (clockid) {
+	case CLOCK_BOOTTIME:
+		clock = "boottime";
+		break;
+	case CLOCK_MONOTONIC:
+		clock = "monotonic";
+		break;
+	default:
+		clock = "unknown";
+		break;
+	}
+	seq_printf(m, "%-10s %10lld %9ld\n", clock, ts->tv_sec, ts->tv_nsec);
 }
 
 void proc_timens_show_offsets(struct task_struct *p, struct seq_file *m)
-- 
2.24.1

Re: [PATCH v2] timens: show clock symbolic names in /proc/pid/timens_offsets

[Michael Kerrisk (man-pages)]

Hi Andrei,

On Sat, 11 Apr 2020 at 17:40, Andrei Vagin <avagin@gmail.com> wrote:
>
> Michael Kerrisk suggested to replace numeric clock IDs on symbolic
> names.
>
> Now the content of these files looks like this:
> $ cat /proc/774/timens_offsets
> monotonic      864000         0
> boottime      1728000         0

Thanks.

Assuming no-one has objections to the patch, please do mark for stable@.

Cheers,

Michael

Re: [PATCH v2] timens: show clock symbolic names in /proc/pid/timens_offsets

[Andrew Morton]

On Sun, 12 Apr 2020 07:51:47 +0200 "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> wrote:

> Hi Andrei,
> 
> On Sat, 11 Apr 2020 at 17:40, Andrei Vagin <avagin@gmail.com> wrote:
> >
> > Michael Kerrisk suggested to replace numeric clock IDs on symbolic
> > names.
> >
> > Now the content of these files looks like this:
> > $ cat /proc/774/timens_offsets
> > monotonic      864000         0
> > boottime      1728000         0
> 
> Thanks.
> 
> Assuming no-one has objections to the patch, please do mark for stable@.
> 

`grep -r timens_offsets Documentation' comes up blank.  Is
/proc/pid/timens_offsets documented anywhere?  If not, it should be! 
And this patch should update that documentation.

I assume the time namespace feature itself is documented under clone(2)?

Re: [PATCH v2] timens: show clock symbolic names in /proc/pid/timens_offsets

[Andrei Vagin]

On Mon, Apr 13, 2020 at 3:47 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Sun, 12 Apr 2020 07:51:47 +0200 "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> wrote:
>
> > Hi Andrei,
> >
> > On Sat, 11 Apr 2020 at 17:40, Andrei Vagin <avagin@gmail.com> wrote:
> > >
> > > Michael Kerrisk suggested to replace numeric clock IDs on symbolic
> > > names.
> > >
> > > Now the content of these files looks like this:
> > > $ cat /proc/774/timens_offsets
> > > monotonic      864000         0
> > > boottime      1728000         0
> >
> > Thanks.
> >
> > Assuming no-one has objections to the patch, please do mark for stable@.
> >
>
> `grep -r timens_offsets Documentation' comes up blank.  Is
> /proc/pid/timens_offsets documented anywhere?  If not, it should be!
> And this patch should update that documentation.
>
> I assume the time namespace feature itself is documented under clone(2)?

Thanks to Michael, we have the man page for time namespaces:
https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man7/time_namespaces.7

And it will be updated according with this change.

Thanks,
Andrei

Re: [PATCH v2] timens: show clock symbolic names in /proc/pid/timens_offsets

[Michael Kerrisk (man-pages)]

On Tue, 14 Apr 2020 at 00:47, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Sun, 12 Apr 2020 07:51:47 +0200 "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> wrote:
>
> > Hi Andrei,
> >
> > On Sat, 11 Apr 2020 at 17:40, Andrei Vagin <avagin@gmail.com> wrote:
> > >
> > > Michael Kerrisk suggested to replace numeric clock IDs on symbolic
> > > names.
> > >
> > > Now the content of these files looks like this:
> > > $ cat /proc/774/timens_offsets
> > > monotonic      864000         0
> > > boottime      1728000         0
> >
> > Thanks.
> >
> > Assuming no-one has objections to the patch, please do mark for stable@.
> >
>
> `grep -r timens_offsets Documentation' comes up blank.  Is
> /proc/pid/timens_offsets documented anywhere?  If not, it should be!
> And this patch should update that documentation.
>
> I assume the time namespace feature itself is documented under clone(2)?

We're good, so far. There's time_namespaces(7) [1] and documentation
of CLONE_NEWTIME in unshare(2) [2].

CLONE_NEWTIME support for clone3() is still a work in progress [3].

Thanks,

Michael

[1] http://man7.org/linux/man-pages/man7/time_namespaces.7.html
[2] http://man7.org/linux/man-pages/man2/unshare.2.html
[3] https://lore.kernel.org/lkml/20200317083043.226593-1-areber@redhat.com/


--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH v2] timens: show clock symbolic names in /proc/pid/timens_offsets

[Andrei Vagin]

On Sat, Apr 11, 2020 at 08:40:31AM -0700, Andrei Vagin wrote:
> Michael Kerrisk suggested to replace numeric clock IDs on symbolic
> names.
> 
> Now the content of these files looks like this:
> $ cat /proc/774/timens_offsets
> monotonic      864000         0
> boottime      1728000         0
> 
> For setting offsets, both representations of clocks can be used.
> 
> As for compatibility, it is acceptable to change things as long as
> userspace doesn't care. The format of timens_offsets files is very
> new and there are no userspace tools that rely on this format.
> 
> But three projects crun, util-linux and criu rely on the interface of
> setting time offsets and this is why we need to continue supporting the
> clock IDs in this case.
> 
> Fixes: 04a8682a71be ("fs/proc: Introduce /proc/pid/timens_offsets")
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Dmitry Safonov <0x7f454c46@gmail.com>
> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>                                                                                                                                            

Thomas and Andrew, could you merge this patch? I am sorry, I used the
wrong subsystem prefix. Let me know if I need to send the third version
of this patch.

Thanks,
Andrei

Re: [PATCH v2] timens: show clock symbolic names in /proc/pid/timens_offsets

[Michael Kerrisk (man-pages)]

Hi Andrei,

On Thu, 16 Apr 2020 at 08:56, Andrei Vagin <avagin@gmail.com> wrote:
>
> On Sat, Apr 11, 2020 at 08:40:31AM -0700, Andrei Vagin wrote:
> > Michael Kerrisk suggested to replace numeric clock IDs on symbolic
> > names.
> >
> > Now the content of these files looks like this:
> > $ cat /proc/774/timens_offsets
> > monotonic      864000         0
> > boottime      1728000         0
> >
> > For setting offsets, both representations of clocks can be used.
> >
> > As for compatibility, it is acceptable to change things as long as
> > userspace doesn't care. The format of timens_offsets files is very
> > new and there are no userspace tools that rely on this format.
> >
> > But three projects crun, util-linux and criu rely on the interface of
> > setting time offsets and this is why we need to continue supporting the
> > clock IDs in this case.
> >
> > Fixes: 04a8682a71be ("fs/proc: Introduce /proc/pid/timens_offsets")
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > Cc: Dmitry Safonov <0x7f454c46@gmail.com>
> > Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
>
> Thomas and Andrew, could you merge this patch? I am sorry, I used the
> wrong subsystem prefix. Let me know if I need to send the third version
> of this patch.

This patch should also be marked for stable@.

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH v2] timens: show clock symbolic names in /proc/pid/timens_offsets

[Thomas Gleixner]

Andrei Vagin <avagin@gmail.com> writes:
> On Sat, Apr 11, 2020 at 08:40:31AM -0700, Andrei Vagin wrote:
>> Michael Kerrisk suggested to replace numeric clock IDs on symbolic
>> names.
>> 
>> Now the content of these files looks like this:
>> $ cat /proc/774/timens_offsets
>> monotonic      864000         0
>> boottime      1728000         0
>> 
>> For setting offsets, both representations of clocks can be used.
>> 
>> As for compatibility, it is acceptable to change things as long as
>> userspace doesn't care. The format of timens_offsets files is very
>> new and there are no userspace tools that rely on this format.
>> 
>> But three projects crun, util-linux and criu rely on the interface of
>> setting time offsets and this is why we need to continue supporting the
>> clock IDs in this case.
>> 
>> Fixes: 04a8682a71be ("fs/proc: Introduce /proc/pid/timens_offsets")
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Eric W. Biederman <ebiederm@xmission.com>
>> Cc: Dmitry Safonov <0x7f454c46@gmail.com>
>> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>                                                                                                                                            
>
> Thomas and Andrew, could you merge this patch? I am sorry, I used the
> wrong subsystem prefix. Let me know if I need to send the third version
> of this patch.

Picking it up.