specialfd: Extend interface to support many fds

[jfree]

Summary

The specialfd interface acts as a syscall wrapper for the creation of
Linux special file descriptors (e.g. eventfd, timerfd). Attempt to
simplify the interface by removing switch statements and moving
fd-specific code into respective user_*fd functions.

As more special fds are added, the switch statement would become crowded
and difficult to work with. This approach attempts to make the interface
more modular.

This is the first patch in a series to support native timerfd:
https://reviews.freebsd.org/D38459

Test Plan

Using the test program from the Linux manpage:

./eventfd 1 2 4 7 14
Child writing 1 to efd
Child writing 2 to efd
Child writing 4 to efd
Child writing 7 to efd
Child writing 14 to efd
Child completed write loop
Parent about to read
Parent read 28 (0x1c) from efd

[markj]

sys/kern/sys_generic.c

939

The usual convention for a system call foo is that sys_foo() handles fetching parameters from userspace, and kern_foo() implements the system call using parameters already copied into the kernel's address space. This makes kern_foo() usable from elsewhere within the kernel.

In particular, I don't think your changes to compat/linux are right: they are passing a kernel pointer for arg, and so the copyin() call below will fail since it expects arg to be a user pointer.

So I'd suggest splitting this function into sys_eventfd(struct thread *, const void *) and kern_eventfd(struct thread *, struct specialfd_eventfd *), and have the Linux compat layer call the latter directly.

961

You could go even further and encode this information in a table. Look at procctl_cmds_info[] for an example.

[jfree]

sys/kern/sys_generic.c

939

The usual convention for a system call foo is that sys_foo() handles fetching parameters from userspace, and kern_foo() implements the system call using parameters already copied into the kernel's address space. This makes kern_foo() usable from elsewhere within the kernel.

Good to know. This is a difficult situation then because sys__specialfd is a multi-purpose syscall and unpacking the user args would require the function to have a specialfd_*fd struct for every unique fd that is passed into it. I was trying to mitigate this.

In particular, I don't think your changes to compat/linux are right: they are passing a kernel pointer for arg, and so the copyin() call below will fail since it expects arg to be a user pointer.

I just caught this after submission :) I was thinking of having lkpi just call eventfd_create_file() directly, but your previous comment disrupts that effort.

So I'd suggest splitting this function into sys_eventfd(struct thread *, const void *) and kern_eventfd(struct thread *, struct specialfd_eventfd *), and have the Linux compat layer call the latter directly.

It probably isn't proper to have a sys_eventfd when eventfd really isn't a syscall, no?

[markj]

sys/kern/sys_generic.c

939

I just caught this after submission :) I was thinking of having lkpi just call eventfd_create_file() directly, but your previous comment disrupts that effort.

That's also fine I think. It just "looks wrong" to have a kern_* function which copies in from userspace, so at least I'd rename kern_eventfd() to sys_eventfd().

It probably isn't proper to have a sys_eventfd when eventfd really isn't a syscall, no?

I don't think there's any improper about that really. For most practical purposes it is a system call, it just doesn't have a dedicated entry in the system call table.

[brooks]

sys/kern/sys_generic.c

939

It probably isn't proper to have a sys_eventfd when eventfd really isn't a syscall, no?

I don't think there's any improper about that really. For most practical purposes it is a system call, it just doesn't have a dedicated entry in the system call table.

I'd call it user_eventfd to stay out of the syscall implementation pseudo namespace. That's what we've been calling common code that sits between sys_foo or freebsd32_foo and kern_foo in CheriBSD and to a letter extent in FreeBSD. See user_clock_nanosleep for an example.

[jfree]

• Fix copyin() errors in lkpi by calling`eventfd_create_file()` directly.
• Rename kern_eventfd() to user_eventfd() since the underlying function doesn't adhere to standard kern_foo() conventions (i.e. it includes a copyin()).

[val_packett.cool]

Wait wait wait… if we decided not to put gettime/settime here, to keep it as an interface only for creating various kinds of descriptors, why are we still going to duplicate the falloc_noinstall/finstall/fdrop dance across the individual functions for each descriptor kind?

[jfree]

In D38497#876076, @val_packett.cool wrote:

Wait wait wait… if we decided not to put gettime/settime here, to keep it as an interface only for creating various kinds of descriptors, why are we still going to duplicate the falloc_noinstall/finstall/fdrop dance across the individual functions for each descriptor kind?

I couldn't figure out a good way to generalize the falloc_noinstall/finstall/fdrop calls since the finstall needs fflags that are unique per special fd. I could move falloc_noinstall and fdrop into sys___specialfd(), I believe.

[val_packett.cool]

In D38497#876077, @jfree wrote:

I couldn't figure out a good way to generalize the falloc_noinstall/finstall/fdrop calls since the finstall needs fflags that are unique per special fd. I could move falloc_noinstall and fdrop into sys___specialfd(), I believe.

Ah, right. Then it's not worth the extra file struct pointer argument tbh.

[jfree]

Fix up flag intermingling between falloc() and finit() in eventfd_create_file().

[kib]

All user_ functions in sys/syscallsubr.h should be moved into a dedicated block and ordered.