Hi everyone, I was looking / poking around the filedesc handling in kern_descrip.c and found the locking a bit.. weird, especially the fd_getfile function is touching protected members of the filedesc w/o taking any lock. This has been already hit previously in [1] and fixed by reordering the malloc / free operations in rev 1.138 of kern_descrip.c ([2]) At the time it was proposed to actually lock fd_getfile which entailed fixing all the callers. I tried to go this way but it proved difficult as sometimes the locking is done one level higher (e.g namei() being called with or without the filedesc lock held). Introducing a _locked version of the file was also not feasible for the same reasons (and anyway it was a bit ugly).
So I went down the rabbit hole and tried changing the semantics of the whole filedesc manipulations in order to make this clearer. I've looked at what others do, fbsd is doing quite the same the thing as this diff does (ie function are responsible for their own locking, and file private function need to be called with the lock held). DFBSD is using a spinlock but essentially took the same approach. The lock are still exposed because some codepath are fiddling with members of the struct without going through a dedicated API (like the UIPC code path). Next step imho would be to get rid of the FIF_LARVAL state. I know the diff is a bit big and scary because it touches sensitive paths... but I wanted to have an opinion on it before going further. It has been running here for a few days, on a workstation, I also ran the regress tests w/o any regressions. Mathieu. [1] https://marc.info/?l=openbsd-bugs&m=148459545413976&w=2 [2] http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/kern/kern_descrip.c.diff?r1=1.137&r2=1.138 diff --git dev/diskmap.c dev/diskmap.c index 4550cffc264..0583cdb642e 100644 --- dev/diskmap.c +++ dev/diskmap.c @@ -55,7 +55,6 @@ diskmapioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p) { struct dk_diskmap *dm; struct nameidata ndp; - struct filedesc *fdp; struct file *fp = NULL; struct vnode *vp = NULL, *ovp; char *devname; @@ -82,9 +81,6 @@ diskmapioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p) if ((error = getvnode(p, fd, &fp)) != 0) goto invalid; - fdp = p->p_fd; - fdplock(fdp); - NDINIT(&ndp, 0, 0, UIO_SYSSPACE, devname, p); ndp.ni_pledge = PLEDGE_RPATH; if ((error = vn_open(&ndp, fp->f_flag, 0)) != 0) @@ -116,7 +112,6 @@ diskmapioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, struct proc *p) VOP_UNLOCK(vp, p); FRELE(fp, p); - fdpunlock(fdp); free(devname, M_DEVBUF, PATH_MAX); return 0; @@ -127,8 +122,6 @@ bad: if (fp) FRELE(fp, p); - fdpunlock(fdp); - invalid: free(devname, M_DEVBUF, PATH_MAX); diff --git kern/exec_script.c kern/exec_script.c index 0b9ae86e224..fdc9215953e 100644 --- kern/exec_script.c +++ kern/exec_script.c @@ -168,9 +168,7 @@ check_shell: panic("exec_script_makecmds: epp already has a fd"); #endif - fdplock(p->p_fd); error = falloc(p, 0, &fp, &epp->ep_fd); - fdpunlock(p->p_fd); if (error) goto fail; @@ -255,9 +253,7 @@ fail: /* kill the opened file descriptor, else close the file */ if (epp->ep_flags & EXEC_HASFD) { epp->ep_flags &= ~EXEC_HASFD; - fdplock(p->p_fd); (void) fdrelease(p, epp->ep_fd); - fdpunlock(p->p_fd); } else vn_close(scriptvp, FREAD, p->p_ucred, p); diff --git kern/kern_descrip.c kern/kern_descrip.c index 623015f75fd..2c056682e3d 100644 --- kern/kern_descrip.c +++ kern/kern_descrip.c @@ -64,6 +64,10 @@ #include <sys/pipe.h> +#define FDP_ASSERT_WLOCK(fdp) rw_assert_wrlock(&(fdp)->fd_lock) +#define FDP_ASSERT_RLOCK(fdp) rw_assert_rdlock(&(fdp)->fd_lock) +#define FDP_ASSERT_LOCK(fdp) rw_assert_anylock(&(fdp)->fd_lock) + /* * Descriptor management. */ @@ -73,6 +77,7 @@ int numfiles; /* actual number of open files */ static __inline void fd_used(struct filedesc *, int); static __inline void fd_unused(struct filedesc *, int); static __inline int find_next_zero(u_int *, int, u_int); +static int fdrelease_locked(struct proc *, int); int finishdup(struct proc *, struct file *, int, int, register_t *, int); int find_last_set(struct filedesc *, int); int dodup3(struct proc *, int, int, int, register_t *); @@ -121,6 +126,7 @@ find_next_zero (u_int *bitmap, int want, u_int bits) return (off << NDENTRYSHIFT) + ffs(~sub) - 1; } +/* filedesc needs to be locked (any lock) */ int find_last_set(struct filedesc *fd, int last) { @@ -128,6 +134,8 @@ find_last_set(struct filedesc *fd, int last) struct file **ofiles = fd->fd_ofiles; u_int *bitmap = fd->fd_lomap; + FDP_ASSERT_LOCK(fd); + off = (last - 1) >> NDENTRYSHIFT; while (off >= 0 && !bitmap[off]) @@ -144,11 +152,14 @@ find_last_set(struct filedesc *fd, int last) return i; } +/* filedesc needs to be write locked. */ static __inline void fd_used(struct filedesc *fdp, int fd) { u_int off = fd >> NDENTRYSHIFT; + FDP_ASSERT_WLOCK(fdp); + fdp->fd_lomap[off] |= 1 << (fd & NDENTRYMASK); if (fdp->fd_lomap[off] == ~0) fdp->fd_himap[off >> NDENTRYSHIFT] |= 1 << (off & NDENTRYMASK); @@ -158,11 +169,14 @@ fd_used(struct filedesc *fdp, int fd) fdp->fd_openfd++; } +/* filedesc needs to be write locked. */ static __inline void fd_unused(struct filedesc *fdp, int fd) { u_int off = fd >> NDENTRYSHIFT; + FDP_ASSERT_WLOCK(fdp); + if (fd < fdp->fd_freefile) fdp->fd_freefile = fd; @@ -184,11 +198,13 @@ fd_getfile(struct filedesc *fdp, int fd) { struct file *fp; - if ((u_int)fd >= fdp->fd_nfiles || (fp = fdp->fd_ofiles[fd]) == NULL) - return (NULL); - - if (!FILE_IS_USABLE(fp)) - return (NULL); + FDP_RLOCK(fdp); + if ((u_int)fd >= fdp->fd_nfiles || + (fp = fdp->fd_ofiles[fd]) == NULL || + !FILE_IS_USABLE(fp)){ + fp = NULL; + } + FDP_RUNLOCK(fdp); return (fp); } @@ -231,20 +247,19 @@ restart: if ((fp = fd_getfile(fdp, old)) == NULL) return (EBADF); FREF(fp); - fdplock(fdp); if ((error = fdalloc(p, 0, &new)) != 0) { FRELE(fp, p); if (error == ENOSPC) { fdexpand(p); - fdpunlock(fdp); goto restart; } goto out; } + FDP_WLOCK(fdp); error = finishdup(p, fp, old, new, retval, 0); + FDP_WUNLOCK(fdp); out: - fdpunlock(fdp); return (error); } @@ -302,28 +317,29 @@ restart: return (0); } FREF(fp); - fdplock(fdp); if (new >= fdp->fd_nfiles) { if ((error = fdalloc(p, new, &i)) != 0) { FRELE(fp, p); if (error == ENOSPC) { fdexpand(p); - fdpunlock(fdp); goto restart; } goto out; } if (new != i) panic("dup2: fdalloc"); + FDP_WLOCK(fdp); fd_unused(fdp, new); + FDP_WUNLOCK(fdp); } /* finishdup() does FRELE */ + FDP_WLOCK(fdp); error = finishdup(p, fp, old, new, retval, 1); if (!error && flags & O_CLOEXEC) fdp->fd_ofileflags[new] |= UF_EXCLOSE; + FDP_WUNLOCK(fdp); out: - fdpunlock(fdp); return (error); } @@ -364,23 +380,22 @@ restart: error = EINVAL; break; } - fdplock(fdp); if ((error = fdalloc(p, newmin, &i)) != 0) { FRELE(fp, p); if (error == ENOSPC) { fdexpand(p); - fdpunlock(fdp); goto restart; } } else { /* finishdup will FRELE for us. */ + FDP_WLOCK(fdp); error = finishdup(p, fp, fd, i, retval, 0); if (!error && SCARG(uap, cmd) == F_DUPFD_CLOEXEC) fdp->fd_ofileflags[i] |= UF_EXCLOSE; + FDP_WUNLOCK(fdp); } - fdpunlock(fdp); return (error); case F_GETFD: @@ -388,12 +403,12 @@ restart: break; case F_SETFD: - fdplock(fdp); + FDP_WLOCK(fdp); if ((long)SCARG(uap, arg) & 1) fdp->fd_ofileflags[fd] |= UF_EXCLOSE; else fdp->fd_ofileflags[fd] &= ~UF_EXCLOSE; - fdpunlock(fdp); + FDP_WUNLOCK(fdp); break; case F_GETFL: @@ -597,7 +612,7 @@ finishdup(struct proc *p, struct file *fp, int old, int new, struct file *oldfp; struct filedesc *fdp = p->p_fd; - fdpassertlocked(fdp); + FDP_ASSERT_WLOCK(fdp); if (fp->f_count == LONG_MAX-2) { FRELE(fp, p); return (EDEADLK); @@ -631,19 +646,33 @@ finishdup(struct proc *p, struct file *fp, int old, int new, void fdremove(struct filedesc *fdp, int fd) { - fdpassertlocked(fdp); + FDP_WLOCK(fdp); fdp->fd_ofiles[fd] = NULL; fdp->fd_ofileflags[fd] = 0; fd_unused(fdp, fd); + FDP_WUNLOCK(fdp); } int fdrelease(struct proc *p, int fd) { struct filedesc *fdp = p->p_fd; + int ret; + + FDP_WLOCK(fdp); + ret = fdrelease_locked(p, fd); + FDP_WUNLOCK(fdp); + + return ret; +} + +static int +fdrelease_locked(struct proc *p, int fd) +{ + struct filedesc *fdp = p->p_fd; struct file **fpp, *fp; - fdpassertlocked(fdp); + FDP_ASSERT_WLOCK(fdp); /* * Don't fd_getfile here. We want to closef LARVAL files and closef @@ -675,9 +704,7 @@ sys_close(struct proc *p, void *v, register_t *retval) if (fd_getfile(fdp, fd) == NULL) return (EBADF); - fdplock(fdp); error = fdrelease(p, fd); - fdpunlock(fdp); return (error); } @@ -780,6 +807,7 @@ fdalloc(struct proc *p, int want, int *result) * of want or fd_freefile. If that fails, consider * expanding the ofile array. */ + FDP_WLOCK(fdp); restart: lim = min((int)p->p_rlimit[RLIMIT_NOFILE].rlim_cur, maxfiles); last = min(fdp->fd_nfiles, lim); @@ -809,15 +837,23 @@ restart: fdp->fd_ofileflags[i] = 0; if (ISSET(p->p_p->ps_flags, PS_PLEDGE)) fdp->fd_ofileflags[i] |= UF_PLEDGED; + FDP_WUNLOCK(fdp); return (0); } } - if (fdp->fd_nfiles >= lim) + + if (fdp->fd_nfiles >= lim) { + FDP_WUNLOCK(fdp); return (EMFILE); + } + FDP_WUNLOCK(fdp); return (ENOSPC); } +/* + * Allocate a file descriptor for the process. + */ void fdexpand(struct proc *p) { @@ -828,8 +864,10 @@ fdexpand(struct proc *p) char *newofileflags; u_int *newhimap, *newlomap; - fdpassertlocked(fdp); + /* This could be a pushed down, mixing read and the write lock but + * to keep the diff minimal we go the full way */ + FDP_WLOCK(fdp); /* * No space in current array. */ @@ -889,6 +927,8 @@ fdexpand(struct proc *p) fdp->fd_ofiles = newofile; fdp->fd_ofileflags = newofileflags; fdp->fd_nfiles = nfiles; + + FDP_WUNLOCK(fdp); } /* @@ -899,12 +939,12 @@ int falloc(struct proc *p, int flags, struct file **resultfp, int *resultfd) { struct file *fp, *fq; + struct filedesc *fdp = p->p_fd; int error, i; KASSERT(resultfp != NULL); KASSERT(resultfd != NULL); - fdpassertlocked(p->p_fd); restart: if ((error = fdalloc(p, 0, &i)) != 0) { if (error == ENOSPC) { @@ -913,8 +953,10 @@ restart: } return (error); } + + FDP_WLOCK(fdp); if (numfiles >= maxfiles) { - fd_unused(p->p_fd, i); + fd_unused(fdp, i); tablefull("file"); return (ENFILE); } @@ -927,19 +969,20 @@ restart: numfiles++; fp = pool_get(&file_pool, PR_WAITOK|PR_ZERO); fp->f_iflags = FIF_LARVAL; - if ((fq = p->p_fd->fd_ofiles[0]) != NULL) { + if ((fq = fdp->fd_ofiles[0]) != NULL) { LIST_INSERT_AFTER(fq, fp, f_list); } else { LIST_INSERT_HEAD(&filehead, fp, f_list); } - p->p_fd->fd_ofiles[i] = fp; - p->p_fd->fd_ofileflags[i] |= (flags & UF_EXCLOSE); + fdp->fd_ofiles[i] = fp; + fdp->fd_ofileflags[i] |= (flags & UF_EXCLOSE); fp->f_count = 1; fp->f_cred = p->p_ucred; crhold(fp->f_cred); *resultfp = fp; *resultfd = i; FREF(fp); + FDP_WUNLOCK(fdp); return (0); } @@ -990,7 +1033,7 @@ fdcopy(struct process *pr) struct file **fpp; int i; - fdplock(fdp); + FDP_RLOCK(fdp); newfdp = pool_get(&fdesc_pool, PR_WAITOK); memcpy(newfdp, fdp, sizeof(struct filedesc)); if (newfdp->fd_cdir) @@ -1039,9 +1082,9 @@ fdcopy(struct process *pr) memcpy(newfdp->fd_ofileflags, fdp->fd_ofileflags, i * sizeof(char)); memcpy(newfdp->fd_himap, fdp->fd_himap, NDHISLOTS(i) * sizeof(u_int)); memcpy(newfdp->fd_lomap, fdp->fd_lomap, NDLOSLOTS(i) * sizeof(u_int)); - fdpunlock(fdp); + FDP_RUNLOCK(fdp); - fdplock(newfdp); + FDP_WLOCK(newfdp); fpp = newfdp->fd_ofiles; for (i = 0; i <= newfdp->fd_lastfile; i++, fpp++) if (*fpp != NULL) { @@ -1054,8 +1097,12 @@ fdcopy(struct process *pr) * their internal consistency, so close them here. */ if ((*fpp)->f_count == LONG_MAX-2 || - (*fpp)->f_type == DTYPE_KQUEUE) + (*fpp)->f_type == DTYPE_KQUEUE) { + /* XXX: Introduce a local fdremove_locked ? */ + FDP_WUNLOCK(newfdp); fdremove(newfdp, i); + FDP_WLOCK(newfdp); + } else (*fpp)->f_count++; } @@ -1068,7 +1115,7 @@ fdcopy(struct process *pr) newfdp->fd_knhashmask = 0; } - fdpunlock(newfdp); + FDP_WUNLOCK(newfdp); return (newfdp); } @@ -1082,6 +1129,7 @@ fdfree(struct proc *p) struct file **fpp, *fp; int i; + FDP_WLOCK(fdp); if (--fdp->fd_refcnt > 0) return; fpp = fdp->fd_ofiles; @@ -1094,6 +1142,8 @@ fdfree(struct proc *p) } } p->p_fd = NULL; + FDP_WUNLOCK(fdp); + if (fdp->fd_nfiles > NDFILE) free(fdp->fd_ofiles, M_FILEDESC, fdp->fd_nfiles * OFILESIZE); if (NDHISLOTS(fdp->fd_nfiles) > NDHISLOTS(NDFILE)) { @@ -1270,7 +1320,6 @@ dupfdopen(struct proc *p, int indx, int mode) int dupfd = p->p_dupfd; struct file *wfp; - fdpassertlocked(fdp); /* * Assume that the filename was user-specified; applications do @@ -1302,6 +1351,7 @@ dupfdopen(struct proc *p, int indx, int mode) if (wfp->f_count == LONG_MAX-2) return (EDEADLK); + FDP_WLOCK(fdp); fdp->fd_ofiles[indx] = wfp; fdp->fd_ofileflags[indx] = (fdp->fd_ofileflags[indx] & UF_EXCLOSE) | (fdp->fd_ofileflags[dupfd] & ~UF_EXCLOSE); @@ -1309,6 +1359,7 @@ dupfdopen(struct proc *p, int indx, int mode) fdp->fd_ofileflags[indx] |= UF_PLEDGED; wfp->f_count++; fd_used(fdp, indx); + FDP_WUNLOCK(fdp); return (0); } @@ -1321,13 +1372,13 @@ fdcloseexec(struct proc *p) struct filedesc *fdp = p->p_fd; int fd; - fdplock(fdp); + FDP_WLOCK(fdp); for (fd = 0; fd <= fdp->fd_lastfile; fd++) { fdp->fd_ofileflags[fd] &= ~UF_PLEDGED; if (fdp->fd_ofileflags[fd] & UF_EXCLOSE) - (void) fdrelease(p, fd); + fdrelease_locked(p, fd); } - fdpunlock(fdp); + FDP_WUNLOCK(fdp); } int @@ -1338,17 +1389,17 @@ sys_closefrom(struct proc *p, void *v, register_t *retval) u_int startfd, i; startfd = SCARG(uap, fd); - fdplock(fdp); + FDP_WLOCK(fdp); if (startfd > fdp->fd_lastfile) { - fdpunlock(fdp); + FDP_WUNLOCK(fdp); return (EBADF); } for (i = startfd; i <= fdp->fd_lastfile; i++) - fdrelease(p, i); + fdrelease_locked(p, i); - fdpunlock(fdp); + FDP_WUNLOCK(fdp); return (0); } diff --git kern/kern_event.c kern/kern_event.c index b13feef56a4..5e6eb391d9d 100644 --- kern/kern_event.c +++ kern/kern_event.c @@ -441,9 +441,7 @@ sys_kqueue(struct proc *p, void *v, register_t *retval) struct file *fp; int fd, error; - fdplock(fdp); error = falloc(p, 0, &fp, &fd); - fdpunlock(fdp); if (error) return (error); fp->f_flag = FREAD | FWRITE; diff --git kern/kern_exec.c kern/kern_exec.c index 3c65a3a9a44..303477396c8 100644 --- kern/kern_exec.c +++ kern/kern_exec.c @@ -550,7 +550,6 @@ sys_execve(struct proc *p, void *v, register_t *retval) * stdin, stdout, and stderr. */ error = 0; - fdplock(p->p_fd); for (i = 0; i < 3; i++) { struct file *fp = NULL; @@ -598,7 +597,6 @@ sys_execve(struct proc *p, void *v, register_t *retval) FILE_SET_MATURE(fp, p); } } - fdpunlock(p->p_fd); if (error) goto exec_abort; } else @@ -699,9 +697,7 @@ bad: /* kill any opened file descriptor, if necessary */ if (pack.ep_flags & EXEC_HASFD) { pack.ep_flags &= ~EXEC_HASFD; - fdplock(p->p_fd); (void) fdrelease(p, pack.ep_fd); - fdpunlock(p->p_fd); } if (pack.ep_interp != NULL) pool_put(&namei_pool, pack.ep_interp); diff --git kern/sys_generic.c kern/sys_generic.c index 8afc08f8d17..864518a499e 100644 --- kern/sys_generic.c +++ kern/sys_generic.c @@ -419,12 +419,12 @@ sys_ioctl(struct proc *p, void *v, register_t *retval) switch (com) { case FIONCLEX: case FIOCLEX: - fdplock(fdp); + FDP_WLOCK(fdp); if (com == FIONCLEX) fdp->fd_ofileflags[SCARG(uap, fd)] &= ~UF_EXCLOSE; else fdp->fd_ofileflags[SCARG(uap, fd)] |= UF_EXCLOSE; - fdpunlock(fdp); + FDP_WUNLOCK(fdp); return (0); } diff --git kern/sys_pipe.c kern/sys_pipe.c index 57c45d74335..067e9fa1a0d 100644 --- kern/sys_pipe.c +++ kern/sys_pipe.c @@ -146,11 +146,9 @@ dopipe(struct proc *p, int *ufds, int flags) if (error != 0) goto free1; - fdplock(fdp); - error = falloc(p, cloexec, &rf, &fds[0]); if (error != 0) - goto free2; + goto free1; rf->f_flag = FREAD | FWRITE | (flags & FNONBLOCK); rf->f_type = DTYPE_PIPE; rf->f_data = rpipe; @@ -158,7 +156,7 @@ dopipe(struct proc *p, int *ufds, int flags) error = falloc(p, cloexec, &wf, &fds[1]); if (error != 0) - goto free3; + goto free2; wf->f_flag = FREAD | FWRITE | (flags & FNONBLOCK); wf->f_type = DTYPE_PIPE; wf->f_data = wpipe; @@ -179,15 +177,12 @@ dopipe(struct proc *p, int *ufds, int flags) else if (KTRPOINT(p, KTR_STRUCT)) ktrfds(p, fds, 2); #endif - fdpunlock(fdp); return (error); -free3: +free2: fdremove(fdp, fds[0]); closef(rf, p); rpipe = NULL; -free2: - fdpunlock(fdp); free1: pipeclose(wpipe); pipeclose(rpipe); diff --git kern/tty_pty.c kern/tty_pty.c index 20dea27e4ed..7c5af67865b 100644 --- kern/tty_pty.c +++ kern/tty_pty.c @@ -1053,16 +1053,13 @@ ptmioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p) switch (cmd) { case PTMGET: - fdplock(fdp); /* Grab two filedescriptors. */ if ((error = falloc(p, 0, &cfp, &cindx)) != 0) { - fdpunlock(fdp); break; } if ((error = falloc(p, 0, &sfp, &sindx)) != 0) { fdremove(fdp, cindx); closef(cfp, p); - fdpunlock(fdp); break; } @@ -1155,7 +1152,6 @@ retry: FILE_SET_MATURE(cfp, p); FILE_SET_MATURE(sfp, p); - fdpunlock(fdp); break; default: error = EINVAL; @@ -1167,6 +1163,5 @@ bad: closef(cfp, p); fdremove(fdp, sindx); closef(sfp, p); - fdpunlock(fdp); return (error); } diff --git kern/uipc_syscalls.c kern/uipc_syscalls.c index 34030d79977..dca2d33a4a5 100644 --- kern/uipc_syscalls.c +++ kern/uipc_syscalls.c @@ -93,9 +93,7 @@ sys_socket(struct proc *p, void *v, register_t *retval) if (error) return (error); - fdplock(fdp); error = falloc(p, (type & SOCK_CLOEXEC) ? UF_EXCLOSE : 0, &fp, &fd); - fdpunlock(fdp); if (error != 0) goto out; @@ -105,10 +103,8 @@ sys_socket(struct proc *p, void *v, register_t *retval) error = socreate(SCARG(uap, domain), &so, type & ~(SOCK_CLOEXEC | SOCK_NONBLOCK | SOCK_DNS), SCARG(uap, protocol)); if (error) { - fdplock(fdp); fdremove(fdp, fd); closef(fp, p); - fdpunlock(fdp); } else { if (type & SOCK_NONBLOCK) so->so_state |= SS_NBIO; @@ -278,9 +274,7 @@ doaccept(struct proc *p, int sock, struct sockaddr *name, socklen_t *anamelen, headfp = fp; - fdplock(fdp); error = falloc(p, (flags & SOCK_CLOEXEC) ? UF_EXCLOSE : 0, &fp, &tmpfd); - fdpunlock(fdp); if (error) { FRELE(headfp, p); return (error); @@ -350,10 +344,8 @@ out: sounlock(s); m_freem(nam); if (error) { - fdplock(fdp); fdremove(fdp, tmpfd); closef(fp, p); - fdpunlock(fdp); } FRELE(headfp, p); return (error); @@ -475,15 +467,14 @@ sys_socketpair(struct proc *p, void *v, register_t *retval) if (error != 0) goto free2; } - fdplock(fdp); if ((error = falloc(p, cloexec, &fp1, &sv[0])) != 0) - goto free3; + goto free2; fp1->f_flag = fflag; fp1->f_type = DTYPE_SOCKET; fp1->f_ops = &socketops; fp1->f_data = so1; if ((error = falloc(p, cloexec, &fp2, &sv[1])) != 0) - goto free4; + goto free3; fp2->f_flag = fflag; fp2->f_type = DTYPE_SOCKET; fp2->f_ops = &socketops; @@ -502,18 +493,15 @@ sys_socketpair(struct proc *p, void *v, register_t *retval) } FILE_SET_MATURE(fp1, p); FILE_SET_MATURE(fp2, p); - fdpunlock(fdp); return (0); } fdremove(fdp, sv[1]); closef(fp2, p); so2 = NULL; -free4: +free3: fdremove(fdp, sv[0]); closef(fp1, p); so1 = NULL; -free3: - fdpunlock(fdp); free2: if (so2 != NULL) (void)soclose(so2); diff --git kern/uipc_usrreq.c kern/uipc_usrreq.c index 787287477fe..7bde375fa4b 100644 --- kern/uipc_usrreq.c +++ kern/uipc_usrreq.c @@ -701,7 +701,6 @@ unp_externalize(struct mbuf *rights, socklen_t controllen, int flags) fdp = mallocarray(nfds, sizeof(int), M_TEMP, M_WAITOK); restart: - fdplock(p->p_fd); if (error != 0) { if (nfds > 0) { rp = ((struct fdpass *)CMSG_DATA(cm)); @@ -734,7 +733,6 @@ restart: */ error = EMSGSIZE; } - fdpunlock(p->p_fd); goto restart; } @@ -743,12 +741,14 @@ restart: * fdalloc() works properly.. We finalize it all * in the loop below. */ + FDP_WLOCK(p->p_fd); p->p_fd->fd_ofiles[fdp[i]] = rp->fp; p->p_fd->fd_ofileflags[fdp[i]] = (rp->flags & UF_PLEDGED); rp++; if (flags & MSG_CMSG_CLOEXEC) p->p_fd->fd_ofileflags[fdp[i]] |= UF_EXCLOSE; + FDP_WUNLOCK(p->p_fd); } /* @@ -774,7 +774,6 @@ restart: cm->cmsg_len = CMSG_LEN(nfds * sizeof(int)); rights->m_len = CMSG_LEN(nfds * sizeof(int)); out: - fdpunlock(p->p_fd); if (fdp) free(fdp, M_TEMP, nfds * sizeof(int)); return (error); diff --git kern/vfs_syscalls.c kern/vfs_syscalls.c index b41c4a24b96..5a6f0b604f3 100644 --- kern/vfs_syscalls.c +++ kern/vfs_syscalls.c @@ -883,8 +883,6 @@ doopenat(struct proc *p, int fd, const char *path, int oflags, mode_t mode, return (error); } - fdplock(fdp); - if ((error = falloc(p, (oflags & O_CLOEXEC) ? UF_EXCLOSE : 0, &fp, &indx)) != 0) goto out; @@ -896,6 +894,7 @@ doopenat(struct proc *p, int fd, const char *path, int oflags, mode_t mode, if (oflags & O_CREAT) ni_pledge |= PLEDGE_CPATH; + /* XXX: Needs locking, in the future ? */ cmode = ((mode &~ fdp->fd_cmask) & ALLPERMS) &~ S_ISTXT; if ((p->p_p->ps_flags & PS_PLEDGE)) cmode &= ACCESSPERMS; @@ -973,7 +972,6 @@ doopenat(struct proc *p, int fd, const char *path, int oflags, mode_t mode, *retval = indx; FILE_SET_MATURE(fp, p); out: - fdpunlock(fdp); return (error); } @@ -1027,7 +1025,6 @@ sys_fhopen(struct proc *p, void *v, register_t *retval) syscallarg(const fhandle_t *) fhp; syscallarg(int) flags; } */ *uap = v; - struct filedesc *fdp = p->p_fd; struct file *fp; struct vnode *vp = NULL; struct mount *mp; @@ -1050,7 +1047,6 @@ sys_fhopen(struct proc *p, void *v, register_t *retval) if ((flags & O_CREAT)) return (EINVAL); - fdplock(fdp); if ((error = falloc(p, (flags & O_CLOEXEC) ? UF_EXCLOSE : 0, &fp, &indx)) != 0) { fp = NULL; @@ -1134,17 +1130,15 @@ sys_fhopen(struct proc *p, void *v, register_t *retval) *retval = indx; FILE_SET_MATURE(fp, p); - fdpunlock(fdp); return (0); bad: if (fp) { - fdremove(fdp, indx); + fdremove(p->p_fd, indx); closef(fp, p); if (vp != NULL) vput(vp); } - fdpunlock(fdp); return (error); } diff --git sys/filedesc.h sys/filedesc.h index 8fd0dc38efc..7dfcd694aba 100644 --- sys/filedesc.h +++ sys/filedesc.h @@ -134,12 +134,15 @@ int fdrelease(struct proc *p, int); void fdremove(struct filedesc *, int); void fdcloseexec(struct proc *); struct file *fd_getfile(struct filedesc *, int); +struct file *fd_getfile_locked(struct filedesc *, int); struct file *fd_getfile_mode(struct filedesc *, int, int); int closef(struct file *, struct proc *); int getsock(struct proc *, int, struct file **); -#define fdplock(fdp) do { NET_ASSERT_UNLOCKED(); rw_enter_write(&(fdp)->fd_lock); } while (0) -#define fdpunlock(fdp) rw_exit_write(&(fdp)->fd_lock) -#define fdpassertlocked(fdp) rw_assert_wrlock(&(fdp)->fd_lock) +#define FDP_WLOCK(fdp) do { NET_ASSERT_UNLOCKED(); rw_enter_write(&(fdp)->fd_lock); } while (0) +#define FDP_WUNLOCK(fdp) rw_exit_write(&(fdp)->fd_lock) +#define FDP_RLOCK(fdp) do { rw_enter_read(&(fdp)->fd_lock); } while (0) +#define FDP_RUNLOCK(fdp) rw_exit_read(&(fdp)->fd_lock) + #endif
