Changeset View
Standalone View
sys/kern/sys_generic.c
Show First 20 Lines • Show All 101 Lines • ▼ Show 20 Lines | |||||||||||||||
#endif | #endif | ||||||||||||||
/* | /* | ||||||||||||||
* Assert that the return value of read(2) and write(2) syscalls fits | * Assert that the return value of read(2) and write(2) syscalls fits | ||||||||||||||
* into a register. If not, an architecture will need to provide the | * into a register. If not, an architecture will need to provide the | ||||||||||||||
* usermode wrappers to reconstruct the result. | * usermode wrappers to reconstruct the result. | ||||||||||||||
*/ | */ | ||||||||||||||
CTASSERT(sizeof(register_t) >= sizeof(size_t)); | CTASSERT(sizeof(register_t) >= sizeof(size_t)); | ||||||||||||||
delphij: I think it's worth commenting here that the reason we are CTASSERT'ing the size was because we… | |||||||||||||||
static MALLOC_DEFINE(M_IOCTLOPS, "ioctlops", "ioctl data buffer"); | static MALLOC_DEFINE(M_IOCTLOPS, "ioctlops", "ioctl data buffer"); | ||||||||||||||
static MALLOC_DEFINE(M_SELECT, "select", "select() buffer"); | static MALLOC_DEFINE(M_SELECT, "select", "select() buffer"); | ||||||||||||||
MALLOC_DEFINE(M_IOV, "iov", "large iov's"); | MALLOC_DEFINE(M_IOV, "iov", "large iov's"); | ||||||||||||||
static int pollout(struct thread *, struct pollfd *, struct pollfd *, | static int pollout(struct thread *, struct pollfd *, struct pollfd *, | ||||||||||||||
u_int); | u_int); | ||||||||||||||
static int pollscan(struct thread *, struct pollfd *, u_int); | static int pollscan(struct thread *, struct pollfd *, u_int); | ||||||||||||||
static int pollrescan(struct thread *); | static int pollrescan(struct thread *); | ||||||||||||||
▲ Show 20 Lines • Show All 738 Lines • ▼ Show 20 Lines | kern_posix_fallocate(struct thread *td, int fd, off_t offset, off_t len) | ||||||||||||||
error = fo_fallocate(fp, offset, len, td); | error = fo_fallocate(fp, offset, len, td); | ||||||||||||||
out: | out: | ||||||||||||||
fdrop(fp, td); | fdrop(fp, td); | ||||||||||||||
return (error); | return (error); | ||||||||||||||
} | } | ||||||||||||||
int | int | ||||||||||||||
sys_fspacectl(struct thread *td, struct fspacectl_args *uap) | |||||||||||||||
{ | |||||||||||||||
struct spacectl_range rqsr, rmsr; | |||||||||||||||
int error, cerror; | |||||||||||||||
error = copyin(uap->rqsr, &rqsr, sizeof(rqsr)); | |||||||||||||||
Done Inline Actions
Just returning without the temporary is more common. I noticed the 32-bit version calls kern_posix_error(). Which is intended? brooks: Just returning without the temporary is more common. I noticed the 32-bit version calls… | |||||||||||||||
Done Inline Actionsthe kern_posix_error() is a mistake. I would remove that. khng: the kern_posix_error() is a mistake. I would remove that. | |||||||||||||||
if (error != 0) | |||||||||||||||
return (error); | |||||||||||||||
error = kern_fspacectl(td, uap->fd, uap->cmd, &rqsr, uap->flags, | |||||||||||||||
&rmsr); | |||||||||||||||
if (error == 0 && uap->rmsr != NULL) { | |||||||||||||||
Done Inline ActionsI wonder if it make sense to change the fspacectl(2) interface while not too late and split in and out spacectl_range. Basically keep original spacectl_range as is, and copy out optional processed length: fspacectl(int fd, int cmd, const struct spacectl_range *sr, int flags, off_t *length); kib: I wonder if it make sense to change the fspacectl(2) interface while not too late and split in… | |||||||||||||||
Done Inline ActionsDoes it sound better to have copyout spacectl_range instead of copyout length, so that callers may save an fseek(DATA) call to guess where to restart? khng: Does it sound better to have copyout spacectl_range instead of copyout length, so that callers… | |||||||||||||||
Not Done Inline ActionsWhy do you copyout result only in case of an error? This is esp. not useful for ERESTART/EINTR etc case where you cleared the error. kib: Why do you copyout result only in case of an error? This is esp. not useful for ERESTART/EINTR… | |||||||||||||||
cerror = copyout(&rmsr, uap->rmsr, sizeof(rmsr)); | |||||||||||||||
if (cerror != 0) | |||||||||||||||
Done Inline ActionsNo, I think error is more important than cerror. In other words, it should be if (error == 0) error = cerror; kib: No, I think error is more important than cerror.
In other words, it should be
```
if (error… | |||||||||||||||
error = cerror; | |||||||||||||||
} | |||||||||||||||
return (error); | |||||||||||||||
} | |||||||||||||||
int | |||||||||||||||
kern_fspacectl(struct thread *td, int fd, int cmd, | |||||||||||||||
const struct spacectl_range *rqsr, int flags, struct spacectl_range *rmsrp) | |||||||||||||||
{ | |||||||||||||||
struct file *fp; | |||||||||||||||
struct spacectl_range rmsr; | |||||||||||||||
int error; | |||||||||||||||
AUDIT_ARG_FD(fd); | |||||||||||||||
AUDIT_ARG_CMD(cmd); | |||||||||||||||
Not Done Inline ActionsExtra () on this and previous lines. kib: Extra () on this and previous lines. | |||||||||||||||
AUDIT_ARG_FFLAGS(flags); | |||||||||||||||
Not Done Inline Actions(flags & ~XXX) != 0 kib: (flags & ~XXX) != 0 | |||||||||||||||
Done Inline ActionsDon't you need to check that offset+len do not overflow? kib: Don't you need to check that offset+len do not overflow? | |||||||||||||||
if (cmd != SPACECTL_DEALLOC || | |||||||||||||||
rqsr->r_offset < 0 || rqsr->r_len <= 0 || | |||||||||||||||
rqsr->r_offset > OFF_MAX - rqsr->r_len || | |||||||||||||||
(flags & ~SPACECTL_F_SUPPORTED) != 0) | |||||||||||||||
return (EINVAL); | |||||||||||||||
error = fget_write(td, fd, &cap_pwrite_rights, &fp); | |||||||||||||||
Done Inline ActionsWhy don't you use fget_write()? I am not even sure why you check for CAP_PWRITE instead of CAP_WRITE, the later seems to be more suitable. kib: Why don't you use fget_write()? I am not even sure why you check for CAP_PWRITE instead of… | |||||||||||||||
Done Inline ActionsBut if we can in theory operate in any positions in a file, we should use CAP_PWRITE? khng: But if we can in theory operate in any positions in a file, we should use CAP_PWRITE? | |||||||||||||||
if (error != 0) | |||||||||||||||
return (error); | |||||||||||||||
AUDIT_ARG_FILE(td->td_proc, fp); | |||||||||||||||
if ((fp->f_ops->fo_flags & DFLAG_SEEKABLE) == 0) { | |||||||||||||||
error = ESPIPE; | |||||||||||||||
goto out; | |||||||||||||||
} | |||||||||||||||
if ((fp->f_flag & FWRITE) == 0) { | |||||||||||||||
error = EBADF; | |||||||||||||||
goto out; | |||||||||||||||
} | |||||||||||||||
rmsr = *rqsr; | |||||||||||||||
error = fo_fspacectl(fp, cmd, &rmsr.r_offset, &rmsr.r_len, flags, | |||||||||||||||
td->td_ucred, td); | |||||||||||||||
/* fspacectl is not restarted after signals if the file is modified. */ | |||||||||||||||
Done Inline ActionsIs this intentional? If so, could you please add a comment explaining why we are ignoring ERESTART/EINTR/EWOULDBLOCK? (I think this is different from short read/write and these shouldn't be ignored?) delphij: Is this intentional? If so, could you please add a comment explaining why we are ignoring… | |||||||||||||||
Done Inline ActionsIt was intentional, but yes they shouldn't be ignored as short read/write. khng: It was intentional, but yes they shouldn't be ignored as short read/write. | |||||||||||||||
Done Inline ActionsI believe that was already discussed, and now you returned back the same errant behavior. Unix convention is that error return is only acceptable if the file content was not changed at all. If syscall did modified the file, regardless was it completed WRT requested op, or just a partial execution, userspace must know exactly what was done. kib: I believe that was already discussed, and now you returned back the same errant behavior. Unix… | |||||||||||||||
if (rmsr.r_len != rqsr->r_len && (error == ERESTART || | |||||||||||||||
error == EINTR || error == EWOULDBLOCK || error == ENOSPC)) | |||||||||||||||
error = 0; | |||||||||||||||
if (error == 0 && rmsrp != NULL) | |||||||||||||||
*rmsrp = rmsr; | |||||||||||||||
out: | |||||||||||||||
Done Inline ActionsI still insist on copying out rmsr always, not only on error == 0. For instance, if errno == ENOSPC, you still might have modified parts of the file, and it is important for usermode to know. kib: I still insist on copying out rmsr always, not only on error == 0. For instance, if errno ==… | |||||||||||||||
fdrop(fp, td); | |||||||||||||||
return (error); | |||||||||||||||
} | |||||||||||||||
int | |||||||||||||||
kern_specialfd(struct thread *td, int type, void *arg) | kern_specialfd(struct thread *td, int type, void *arg) | ||||||||||||||
{ | { | ||||||||||||||
struct file *fp; | struct file *fp; | ||||||||||||||
struct specialfd_eventfd *ae; | struct specialfd_eventfd *ae; | ||||||||||||||
int error, fd, fflags; | int error, fd, fflags; | ||||||||||||||
fflags = 0; | fflags = 0; | ||||||||||||||
error = falloc_noinstall(td, &fp); | error = falloc_noinstall(td, &fp); | ||||||||||||||
▲ Show 20 Lines • Show All 1,137 Lines • Show Last 20 Lines |
I think it's worth commenting here that the reason we are CTASSERT'ing the size was because we wanted to make sure that for both 32-bit and 64-bit variants, the struct have the same layout (and thus the compat32 call can just use the same struct); having it in the same block of asserting size_t was confusing for me in the first glance.