specialfd: Extend interface to support many fds
AbandonedPublic
Actions

Authored by jfree on Feb 10 2023, 7:32 PM.

Details

Reviewers

val_packett.cool
markj
brooks
kib

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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

jfree created this revision.Feb 10 2023, 7:32 PM

Herald added a subscriber: imp. · View Herald TranscriptFeb 10 2023, 7:32 PM

jfree requested review of this revision.Feb 10 2023, 7:32 PM

markj added inline comments.Feb 10 2023, 7:56 PM

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 added inline comments.Feb 10 2023, 8:05 PM

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 added inline comments.Feb 10 2023, 8:21 PM

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 added inline comments.Feb 10 2023, 8:33 PM

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.

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()).

markj accepted this revision.Feb 10 2023, 9:22 PM

This revision is now accepted and ready to land.Feb 10 2023, 9:22 PM

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?

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.

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.