Re: clone3: allow creation of time namespace with offset

On Thu, Mar 19, 2020 at 11:29:55AM +0100, Christian Brauner wrote:

On Thu, Mar 19, 2020 at 09:16:43AM +0100, Arnd Bergmann wrote:

quoted

On Thu, Mar 19, 2020 at 9:11 AM Adrian Reber [off-list ref] wrote:

quoted

With Arnd's idea of only using nanoseconds, timens_offset would then
contain something like this:

struct timens_offset {
        __aligned_s64 monotonic_offset_ns;
        __aligned_s64 boottime_offset_ns;
};

I kind of prefer adding boottime and monotonic directly to struct clone_args

        __aligned_u64 tls;
        __aligned_u64 set_tid;
        __aligned_u64 set_tid_size;
+       __aligned_s64 monotonic_offset_ns;
+       __aligned_s64 boottime_offset_ns;
 };

I would also prefer the second approach using two 64-bit integers
instead of a pointer, as it keeps the interface simpler to implement
and simpler to interpret by other tools.

Why I don't like has two reasons. There's the scenario where we have
added new extensions after the new boottime member and then we introduce
another offset. Then you'd be looking at:

__aligned_u64 tls;
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
+ __aligned_s64 monotonic_offset_ns;
+ __aligned_s64 boottime_offset_ns;
__aligned_s64 something_1
__aligned_s64 anything_2
+ __aligned_s64 sometime_offset_ns

which bothers me just by looking at it. That's in addition to adding two
new members to the struct when most people will never set CLONE_NEWTIME.
We'll also likely have more features in the future that will want to
pass down more info than we want to directly expose in struct
clone_args, e.g. for a long time I have been thinking about adding a
struct for CLONE_NEWUSER that allows you to specify the id mappings you
want the new user namespace to get. We surely don't want to force all
new info into the uppermost struct. So I'm not convinced we should here.

I think here we can start thinking about a netlink-like interface.

struct clone_args {
	....
	u64	attrs_offset;
}

struct clone_attr {
	u16 cla_len;
	u16 cla_type;
}


....

int parse_clone_attributes(struct kernel_clone_args *kargs, struct clone_args *args, size_t args_size)
{
	u64 off = args->attrs_offset;

	while (off < size) {
		struct clone_attr *attr;

		if (off + sizeof(struct clone_attr) uargs_size)
			return -EINVAL;

		attr = (struct clone_attr *) ((void *)args + off);

		if (attr->cla_type > CLONE_ATTR_TYPE_MAX)
			return -ENOSYS;

		kargs->attrs[attr->cla_type] = CLONE_ATTR_DATA(attr);
		off += CLONE_ATTR_LEN(attr);
	}

	return 0;
}

This interface doesn't suffer from problems what you enumerated before:

* clone_args contains only fields which are often used.
* per-feature attributes can be extended in a future without breaking
  backward compatibility.
* unused features don't affect clone3 argument size.
* seccomp-friendly (I am not 100% sure about this)