On 29/08/2019 14.15, Aleksa Sarai wrote:

On 2019-08-24, Daniel Colascione [off-list ref] wrote:

quoted

Why pad the structure when new functionality (perhaps accommodated via
a larger structure) could be signaled by passing a new flag? Adding
reserved fields to a structure with a size embedded in the ABI makes a
lot of sense --- e.g., pthread_mutex_t can't grow. But this structure
can grow, so the reservation seems needless to me.

Quite a few folks have said that ->reserved is either unnecessary or
too big. I will be changing this, though I am not clear what the best
way of extending the structure is. If anyone has a strong opinion on
this (or an alternative to the ones listed below), please chime in. I
don't have any really strong attachment to this aspect of the API.

There appear to be a few ways we can do it (that all have precedence
with other syscalls):

 1. Use O_* flags to indicate extensions.
 2. A separate "version" field that is incremented when we change.
 3. Add a size_t argument to openat2(2).
 4. Reserve space (as in this patchset).

(My personal preference would be (3), followed closely by (2).)

3, definitely, and instead of having to invent a new scheme for every
new syscall, make that the default pattern by providing a helper

int __copy_abi_struct(void *kernel, size_t ksize, const void __user
*user, size_t usize)
{
	size_t copy = min(ksize, usize);

	if (copy_from_user(kernel, user, copy))
		return -EFAULT;

	if (usize > ksize) {
		/* maybe a separate "return user_is_zero(user + ksize, usize -
ksize);" helper */
		char c;
		user += ksize;
		usize -= ksize;
		while (usize--) {
			if (get_user(c, user++))
				return -EFAULT;
			if (c)
				return -EINVAL;
		}
	} else if (ksize > usize) {
		memset(kernel + usize, 0, ksize - usize);
	}
	return 0;
}
#define copy_abi_struct(kernel, user, usize)	\
	__copy_abi_struct(kernel, sizeof(*kernel), user, usize)

Both (1) and (2) have the problem that the "struct version" is inside
the struct so we'd need to copy_from_user() twice. This isn't the end of
the world, it just feels a bit less clean than is ideal. (3) fixes that
problem, at the cost of making the API slightly more cumbersome to use
directly (though again glibc could wrap that away).

I don't see how 3 is cumbersome to use directly. Userspace code does
struct openat_of_the_day args = {.field1 = x, .field3 = y} and passes
&args, sizeof(args). What does glibc need to do beyond its usual munging
of the userspace ABI registers to the syscall ABI registers?

Rasmus