Date: Thu, 29 Jan 2015 22:58:51 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: freebsd-hackers@freebsd.org, pjd@freebsd.org Subject: filedesc: simplify fget_unlocked & friends Message-ID: <20150129215851.GA20867@dft-labs.eu>
next in thread | raw e-mail | index | archive | help
filedesc: simplify fget_unlocked & friends
Introduce fget_fcntl which performs appropriate checks when needed.
This removes a branch from fget_unlocked.
Introduce fget_mmap dealing with cap_rights_to_vmprot conversion.
This removes a branch from _fget.
Modify fget_unlocked to pass sequence counter to interested callers so
that they can perform their own checks and make sure the result was
obtained from stable data.
diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 31352e9..f7e082b6 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -531,8 +531,8 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
break;
case F_GETFL:
- error = fget_unlocked(fdp, fd,
- cap_rights_init(&rights, CAP_FCNTL), F_GETFL, &fp, NULL);
+ error = fget_fcntl(td, fd,
+ cap_rights_init(&rights, CAP_FCNTL), F_GETFL, &fp);
if (error != 0)
break;
td->td_retval[0] = OFLAGS(fp->f_flag);
@@ -540,8 +540,8 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
break;
case F_SETFL:
- error = fget_unlocked(fdp, fd,
- cap_rights_init(&rights, CAP_FCNTL), F_SETFL, &fp, NULL);
+ error = fget_fcntl(td, fd,
+ cap_rights_init(&rights, CAP_FCNTL), F_SETFL, &fp);
if (error != 0)
break;
do {
@@ -568,8 +568,8 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
break;
case F_GETOWN:
- error = fget_unlocked(fdp, fd,
- cap_rights_init(&rights, CAP_FCNTL), F_GETOWN, &fp, NULL);
+ error = fget_fcntl(td, fd,
+ cap_rights_init(&rights, CAP_FCNTL), F_GETOWN, &fp);
if (error != 0)
break;
error = fo_ioctl(fp, FIOGETOWN, &tmp, td->td_ucred, td);
@@ -579,8 +579,8 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
break;
case F_SETOWN:
- error = fget_unlocked(fdp, fd,
- cap_rights_init(&rights, CAP_FCNTL), F_SETOWN, &fp, NULL);
+ error = fget_fcntl(td, fd,
+ cap_rights_init(&rights, CAP_FCNTL), F_SETOWN, &fp);
if (error != 0)
break;
tmp = arg;
@@ -602,7 +602,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
case F_SETLK:
do_setlk:
cap_rights_init(&rights, CAP_FLOCK);
- error = fget_unlocked(fdp, fd, &rights, 0, &fp, NULL);
+ error = fget_unlocked(fdp, fd, &rights, &fp, NULL);
if (error != 0)
break;
if (fp->f_type != DTYPE_VNODE) {
@@ -691,7 +691,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
* that the closing thread was a bit slower and that the
* advisory lock succeeded before the close.
*/
- error = fget_unlocked(fdp, fd, &rights, 0, &fp2, NULL);
+ error = fget_unlocked(fdp, fd, &rights, &fp2, NULL);
if (error != 0) {
fdrop(fp, td);
break;
@@ -710,7 +710,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
case F_GETLK:
error = fget_unlocked(fdp, fd,
- cap_rights_init(&rights, CAP_FLOCK), 0, &fp, NULL);
+ cap_rights_init(&rights, CAP_FLOCK), &fp, NULL);
if (error != 0)
break;
if (fp->f_type != DTYPE_VNODE) {
@@ -748,7 +748,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, intptr_t arg)
arg = arg ? 128 * 1024: 0;
/* FALLTHROUGH */
case F_READAHEAD:
- error = fget_unlocked(fdp, fd, NULL, 0, &fp, NULL);
+ error = fget_unlocked(fdp, fd, NULL, &fp, NULL);
if (error != 0)
break;
if (fp->f_type != DTYPE_VNODE) {
@@ -2327,10 +2327,10 @@ finit(struct file *fp, u_int flag, short type, void *data, struct fileops *ops)
int
fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
- int needfcntl, struct file **fpp, cap_rights_t *haverightsp)
+ struct file **fpp, seq_t *seqp)
{
#ifdef CAPABILITIES
- struct filedescent fde;
+ struct filedescent *fde;
#endif
struct fdescenttbl *fdt;
struct file *fp;
@@ -2355,28 +2355,23 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
for (;;) {
#ifdef CAPABILITIES
seq = seq_read(fd_seq(fdt, fd));
- fde = fdt->fdt_ofiles[fd];
+ fde = &fdt->fdt_ofiles[fd];
+ haverights = cap_rights_fde(fde);
+ fp = fde->fde_file;
if (!seq_consistent(fd_seq(fdt, fd), seq)) {
cpu_spinwait();
continue;
}
- fp = fde.fde_file;
#else
fp = fdt->fdt_ofiles[fd].fde_file;
#endif
if (fp == NULL)
return (EBADF);
#ifdef CAPABILITIES
- haverights = cap_rights_fde(&fde);
if (needrightsp != NULL) {
error = cap_check(haverights, needrightsp);
if (error != 0)
return (error);
- if (cap_rights_is_set(needrightsp, CAP_FCNTL)) {
- error = cap_fcntl_check_fde(&fde, needfcntl);
- if (error != 0)
- return (error);
- }
}
#endif
retry:
@@ -2406,11 +2401,9 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
fdrop(fp, curthread);
}
*fpp = fp;
- if (haverightsp != NULL) {
+ if (seqp != NULL) {
#ifdef CAPABILITIES
- *haverightsp = *haverights;
-#else
- CAP_ALL(haverightsp);
+ *seqp = seq;
#endif
}
return (0);
@@ -2431,11 +2424,11 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
*/
static __inline int
_fget(struct thread *td, int fd, struct file **fpp, int flags,
- cap_rights_t *needrightsp, u_char *maxprotp)
+ cap_rights_t *needrightsp, seq_t *seqp)
{
struct filedesc *fdp;
struct file *fp;
- cap_rights_t haverights, needrights;
+ cap_rights_t needrights;
int error;
*fpp = NULL;
@@ -2444,9 +2437,7 @@ _fget(struct thread *td, int fd, struct file **fpp, int flags,
needrights = *needrightsp;
else
cap_rights_init(&needrights);
- if (maxprotp != NULL)
- cap_rights_set(&needrights, CAP_MMAP);
- error = fget_unlocked(fdp, fd, &needrights, 0, &fp, &haverights);
+ error = fget_unlocked(fdp, fd, &needrights, &fp, seqp);
if (error != 0)
return (error);
if (fp->f_ops == &badfileops) {
@@ -2454,17 +2445,6 @@ _fget(struct thread *td, int fd, struct file **fpp, int flags,
return (EBADF);
}
-#ifdef CAPABILITIES
- /*
- * If requested, convert capability rights to access flags.
- */
- if (maxprotp != NULL)
- *maxprotp = cap_rights_to_vmprot(&haverights);
-#else /* !CAPABILITIES */
- if (maxprotp != NULL)
- *maxprotp = VM_PROT_ALL;
-#endif /* CAPABILITIES */
-
/*
* FREAD and FWRITE failure return EBADF as per POSIX.
*/
@@ -2506,8 +2486,31 @@ int
fget_mmap(struct thread *td, int fd, cap_rights_t *rightsp, u_char *maxprotp,
struct file **fpp)
{
+ int error;
+#ifndef CAPABILITIES
+ error = _fget(td, fd, fpp, 0, rightsp, NULL);
+ if (maxprotp != NULL)
+ *maxprotp = VM_PROT_ALL;
+#else
+ struct filedesc *fdp = td->td_proc->p_fd;
+ seq_t seq;
+ MPASS(cap_rights_is_set(rightsp, CAP_MMAP));
- return (_fget(td, fd, fpp, 0, rightsp, maxprotp));
+ for (;;) {
+ error = _fget(td, fd, fpp, 0, rightsp, &seq);
+ if (error != 0)
+ return (error);
+ /*
+ * If requested, convert capability rights to access flags.
+ */
+ if (maxprotp != NULL)
+ *maxprotp = cap_rights_to_vmprot(cap_rights(fdp, fd));
+ if (!fd_modified(td->td_proc->p_fd, fd, seq))
+ break;
+ fdrop(*fpp, td);
+ }
+#endif
+ return (error);
}
int
@@ -2524,6 +2527,35 @@ fget_write(struct thread *td, int fd, cap_rights_t *rightsp, struct file **fpp)
return (_fget(td, fd, fpp, FWRITE, rightsp, NULL));
}
+int
+fget_fcntl(struct thread *td, int fd, cap_rights_t *rightsp, int needfcntl,
+ struct file **fpp)
+{
+ struct filedesc *fdp = td->td_proc->p_fd;
+#ifndef CAPABILITIES
+ return (fget_unlocked(fdp, fd, rightsp, fpp, NULL));
+#else
+ int error;
+ seq_t seq;
+
+ MPASS(cap_rights_is_set(rightsp, CAP_FCNTL));
+ for (;;) {
+ error = fget_unlocked(fdp, fd, rightsp, fpp, &seq);
+ if (error != 0)
+ return (error);
+ error = cap_fcntl_check(fdp, fd, needfcntl);
+ if (!fd_modified(fdp, fd, seq))
+ break;
+ fdrop(*fpp, td);
+ }
+ if (error != 0) {
+ fdrop(*fpp, td);
+ *fpp = NULL;
+ }
+ return (error);
+#endif
+}
+
/*
* Like fget() but loads the underlying vnode, or returns an error if the
* descriptor does not represent a vnode. Note that pipes use vnodes but
diff --git a/sys/kern/sys_generic.c b/sys/kern/sys_generic.c
index 8dddc9b..8fc1db9 100644
--- a/sys/kern/sys_generic.c
+++ b/sys/kern/sys_generic.c
@@ -1214,7 +1214,7 @@ getselfd_cap(struct filedesc *fdp, int fd, struct file **fpp)
cap_rights_init(&rights, CAP_EVENT);
- return (fget_unlocked(fdp, fd, &rights, 0, fpp, NULL));
+ return (fget_unlocked(fdp, fd, &rights, fpp, NULL));
}
/*
diff --git a/sys/kern/tty.c b/sys/kern/tty.c
index 373fcc3..d9d0cce 100644
--- a/sys/kern/tty.c
+++ b/sys/kern/tty.c
@@ -1897,7 +1897,7 @@ ttyhook_register(struct tty **rtp, struct proc *p, int fd,
/* Validate the file descriptor. */
fdp = p->p_fd;
error = fget_unlocked(fdp, fd, cap_rights_init(&rights, CAP_TTYHOOK),
- 0, &fp, NULL);
+ &fp, NULL);
if (error != 0)
return (error);
if (fp->f_ops == &badfileops) {
diff --git a/sys/kern/uipc_syscalls.c b/sys/kern/uipc_syscalls.c
index b0d849d..83a7b99 100644
--- a/sys/kern/uipc_syscalls.c
+++ b/sys/kern/uipc_syscalls.c
@@ -156,7 +156,7 @@ getsock_cap(struct filedesc *fdp, int fd, cap_rights_t *rightsp,
struct file *fp;
int error;
- error = fget_unlocked(fdp, fd, rightsp, 0, &fp, NULL);
+ error = fget_unlocked(fdp, fd, rightsp, &fp, NULL);
if (error != 0)
return (error);
if (fp->f_type != DTYPE_SOCKET) {
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 0f24819..14be379 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -4217,7 +4217,7 @@ getvnode(struct filedesc *fdp, int fd, cap_rights_t *rightsp, struct file **fpp)
struct file *fp;
int error;
- error = fget_unlocked(fdp, fd, rightsp, 0, &fp, NULL);
+ error = fget_unlocked(fdp, fd, rightsp, &fp, NULL);
if (error != 0)
return (error);
diff --git a/sys/ofed/include/linux/file.h b/sys/ofed/include/linux/file.h
index b76a408..22a035f 100644
--- a/sys/ofed/include/linux/file.h
+++ b/sys/ofed/include/linux/file.h
@@ -48,7 +48,7 @@ linux_fget(unsigned int fd)
{
struct file *file;
- if (fget_unlocked(curthread->td_proc->p_fd, fd, NULL, 0, &file,
+ if (fget_unlocked(curthread->td_proc->p_fd, fd, NULL, &file,
NULL) != 0) {
return (NULL);
}
@@ -73,7 +73,7 @@ put_unused_fd(unsigned int fd)
{
struct file *file;
- if (fget_unlocked(curthread->td_proc->p_fd, fd, NULL, 0, &file,
+ if (fget_unlocked(curthread->td_proc->p_fd, fd, NULL, &file,
NULL) != 0) {
return;
}
@@ -93,7 +93,7 @@ fd_install(unsigned int fd, struct linux_file *filp)
{
struct file *file;
- if (fget_unlocked(curthread->td_proc->p_fd, fd, NULL, 0, &file,
+ if (fget_unlocked(curthread->td_proc->p_fd, fd, NULL, &file,
NULL) != 0) {
file = NULL;
}
diff --git a/sys/sys/file.h b/sys/sys/file.h
index d102a87..1538851 100644
--- a/sys/sys/file.h
+++ b/sys/sys/file.h
@@ -230,6 +230,8 @@ int fget_read(struct thread *td, int fd, cap_rights_t *rightsp,
struct file **fpp);
int fget_write(struct thread *td, int fd, cap_rights_t *rightsp,
struct file **fpp);
+int fget_fcntl(struct thread *td, int fd, cap_rights_t *rightsp,
+ int needfcntl, struct file **fpp);
int _fdrop(struct file *fp, struct thread *td);
fo_rdwr_t invfo_rdwr;
diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
index 9cef6aa..4eef17b 100644
--- a/sys/sys/filedesc.h
+++ b/sys/sys/filedesc.h
@@ -169,7 +169,7 @@ void mountcheckdirs(struct vnode *olddp, struct vnode *newdp);
/* Return a referenced file from an unlocked descriptor. */
int fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
- int needfcntl, struct file **fpp, cap_rights_t *haverightsp);
+ struct file **fpp, seq_t *seqp);
/* Requires a FILEDESC_{S,X}LOCK held and returns without a ref. */
static __inline struct file *
@@ -184,6 +184,13 @@ fget_locked(struct filedesc *fdp, int fd)
return (fdp->fd_ofiles[fd].fde_file);
}
+static __inline bool
+fd_modified(struct filedesc *fdp, int fd, seq_t seq)
+{
+
+ return (!seq_consistent(fd_seq(fdp->fd_files, fd), seq));
+}
+
#endif /* _KERNEL */
#endif /* !_SYS_FILEDESC_H_ */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150129215851.GA20867>