Changeset View
Standalone View
sys/sys/timerfd.h
- This file was added.
/*- | |||||||||||||||||
* SPDX-License-Identifier: BSD-2-Clause-FreeBSD | |||||||||||||||||
* | |||||||||||||||||
* Copyright (c) 2023 Jake Freeland <jfree@FreeBSD.org> | |||||||||||||||||
* | |||||||||||||||||
* Redistribution and use in source and binary forms, with or without | |||||||||||||||||
* modification, are permitted provided that the following conditions | |||||||||||||||||
* are met: | |||||||||||||||||
* 1. Redistributions of source code must retain the above copyright | |||||||||||||||||
* notice, this list of conditions and the following disclaimer. | |||||||||||||||||
* 2. Redistributions in binary form must reproduce the above copyright | |||||||||||||||||
* notice, this list of conditions and the following disclaimer in the | |||||||||||||||||
* documentation and/or other materials provided with the distribution. | |||||||||||||||||
* | |||||||||||||||||
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND | |||||||||||||||||
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | |||||||||||||||||
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | |||||||||||||||||
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE | |||||||||||||||||
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | |||||||||||||||||
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS | |||||||||||||||||
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | |||||||||||||||||
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | |||||||||||||||||
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | |||||||||||||||||
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | |||||||||||||||||
* SUCH DAMAGE. | |||||||||||||||||
*/ | |||||||||||||||||
#ifndef _SYS_TIMERFD_H_ | |||||||||||||||||
#define _SYS_TIMERFD_H_ | |||||||||||||||||
#include <sys/types.h> | |||||||||||||||||
#include <sys/fcntl.h> | |||||||||||||||||
#include <sys/malloc.h> | |||||||||||||||||
#include <sys/timespec.h> | |||||||||||||||||
typedef uint64_t timerfd_t; | |||||||||||||||||
#define TFD_NONBLOCK O_NONBLOCK | |||||||||||||||||
#define TFD_CLOEXEC O_CLOEXEC | |||||||||||||||||
#define TFD_TIMER_ABSTIME (1 << 0) | |||||||||||||||||
#define TFD_TIMER_CANCEL_ON_SET (1 << 1) | |||||||||||||||||
#define TFD_SHARED_FCNTL_FLAGS (TFD_NONBLOCK | TFD_CLOEXEC) | |||||||||||||||||
#define TFD_CREATE_FLAGS TFD_SHARED_FCNTL_FLAGS | |||||||||||||||||
#define TFD_SETTIME_FLAGS (TFD_TIMER_ABSTIME | TFD_TIMER_CANCEL_ON_SET) | |||||||||||||||||
#ifdef _KERNEL | |||||||||||||||||
MALLOC_DECLARE(M_TIMERFD); | |||||||||||||||||
int timerfd_create_file(struct thread *td, int clockid, int flags); | |||||||||||||||||
brooks: The changes occur via the pointers in timerfd_settime_args so no need for W. My prior… | |||||||||||||||||
int timerfd_gettime_common(struct thread *td, int fd, struct itimerspec *cts); | |||||||||||||||||
int timerfd_settime_common(struct thread *td, int fd, int flags, | |||||||||||||||||
struct itimerspec *nts, struct itimerspec *ots); | |||||||||||||||||
#else | |||||||||||||||||
Done Inline Actions
brooks: | |||||||||||||||||
__BEGIN_DECLS | |||||||||||||||||
Done Inline Actions
Different command numbers must not be used. Only the size changes between 32 and 64-bit. Keep in mind that 32-bit libc will use TFD_GETTIME and TFD_SETTIME. brooks: Different command numbers must not be used. Only the size changes between 32 and 64-bit. Keep… | |||||||||||||||||
Done Inline Actions
The _IOC_NEWTYPE() obviously isn't the underlying problem, but these changes affect 64-bit devices compiled with COMPAT32 presumably by chopping off the upper 32 bits in the cast to uint32_t. This results in ENOTTY. I don't think an ifdef i386 would be appropriate. I am unsure how to proceed. jfree: > Different command numbers must not be used. Only the size changes between 32 and 64-bit. Keep… | |||||||||||||||||
Done Inline ActionsIgnore the comment above. I figured out the real issue... I adjusted TFD_SETTIME to be read-only as you suggested, but this didn't make much sense to me. We want the kernel to copyin the same userspace settime_args struct that is passed in via the ioctl() so writing is necessary. Also, we want to pass that modified copy back to userspace in the case of old_time being non-NULL so read is also necessary. TFD_SETTIME must be _IOWR if we don't want ENOTTY. jfree: Ignore the comment above. I figured out the real issue...
I adjusted `TFD_SETTIME` to be read… | |||||||||||||||||
Done Inline Actions
Gah. I'm sorry. It should in fact be _IOW. I keep getting them backwards. The kernel needs to read the struct, but does not need to write to it. brooks: > I adjusted `TFD_SETTIME` to be read-only as you suggested, but this didn't make much sense to… | |||||||||||||||||
Done Inline ActionsI still get ENOTTY with _IOW. The timerfd functions aren't modifying the userspace addr directly. I believe that the kernel doesn't bother copying the modified data back to the userspace address unless read access is declared. There also appears to be some checking on the data to see if it was modified because I do not receive ENOTTY with _IOWR. jfree: I still get ENOTTY with `_IOW`. The timerfd functions aren't modifying the userspace addr… | |||||||||||||||||
Done Inline ActionsDid you recompile userspace? ENOTTY means you didn't match a command. There is no check that anything changed. The code in sys_ioctl is literally: if (error == 0 && (com & IOC_OUT)) error = copyout(data, uap->data, (u_int)size); brooks: Did you recompile userspace? ENOTTY means you didn't match a command. There is no check that… | |||||||||||||||||
Done Inline ActionsI have to form the habit of sifting through the code when I encounter an issue like this. Yes, you are right. I had forgotten that sys header files were installed through world. I apologize for the silly distraction. jfree: I have to form the habit of sifting through the code when I encounter an issue like this. Yes… | |||||||||||||||||
int timerfd_create(int clockid, int flags); | |||||||||||||||||
int timerfd_settime(int fd, int flags, const struct itimerspec *new_value, | |||||||||||||||||
Done Inline Actions
kib: | |||||||||||||||||
struct itimerspec *old_value); | |||||||||||||||||
int timerfd_gettime(int fd, struct itimerspec *curr_value); | |||||||||||||||||
__END_DECLS | |||||||||||||||||
#endif /* !_KERNEL */ | |||||||||||||||||
#endif /* !_SYS_TIMERFD_H_ */ | |||||||||||||||||
Done Inline ActionsI do not understand this. Why do you need fd in the timerfd_XXX_args structs? You operate on some fd/fp by means of ioctl. Is the Linux API that weird that you can use one timerfd fd to request operation on another? kib: I do not understand this. Why do you need fd in the timerfd_XXX_args structs? You operate on… | |||||||||||||||||
Done Inline Actions
The fd that is passed in through timerfd_XXX_args is the same fd that we're calling ioctl() on. There doesn't appear to be a direct way to get that fd back once we're in timerfd_ioctl(). Either way, this approach is extremely messy and I dislike it, but it's one less call in the syscall table. jfree: > I do not understand this. Why do you need fd in the timerfd_XXX_args structs? You operate… | |||||||||||||||||
Done Inline ActionsWhy do you need fd? Why fp is not enough? kib: Why do you need fd? Why fp is not enough? | |||||||||||||||||
Done Inline ActionsBoth timerfd_args structures contain pointers. You need compat32 translation for them. kib: Both timerfd_args structures contain pointers. You need compat32 translation for them. | |||||||||||||||||
Done Inline Actions
I figured this would be necessary, but I can't find documentation on the procedure anywhere. Could you direct me towards a guide? jfree: > Both timerfd_args structures contain pointers. You need compat32 translation for them.
I… | |||||||||||||||||
Done Inline ActionsThere is no guide. Read examples in the code. kib: There is no guide. Read examples in the code. | |||||||||||||||||
Done Inline Actions
If someone wants to write tests that directly exercise these from userspace then adding (defined(_KERNEL) || defined(WANT_TFD32_IOCTLS) or the like would work, but let's not expose them to userspace without an actual use. brooks: If someone wants to write tests that directly exercise these from userspace then adding `… | |||||||||||||||||
Done Inline Actions
brooks: | |||||||||||||||||
Done Inline ActionsShould these be pointers? jfree: Should these be pointers? | |||||||||||||||||
Done Inline ActionsNo, this is still wrong. You need uint32_t new_value and convert the 32bit val into the pointer (pointers in compat32 world are 32bit). kib: No, this is still wrong. You need `uint32_t new_value` and convert the 32bit val into the… | |||||||||||||||||
Done Inline ActionsYes. I understand now. Thanks. jfree: Yes. I understand now. Thanks. | |||||||||||||||||
Done Inline Actions
brooks: |
The changes occur via the pointers in timerfd_settime_args so no need for W. My prior suggestion was based on a different model.