From owner-freebsd-current Thu Jul 12 1:29:24 2001 Delivered-To: freebsd-current@freebsd.org Received: from sneakerz.org (sneakerz.org [216.33.66.254]) by hub.freebsd.org (Postfix) with ESMTP id B806137B405; Thu, 12 Jul 2001 01:28:59 -0700 (PDT) (envelope-from bright@sneakerz.org) Received: by sneakerz.org (Postfix, from userid 1092) id C9E5D5D01F; Thu, 12 Jul 2001 03:28:51 -0500 (CDT) Date: Thu, 12 Jul 2001 03:28:51 -0500 From: Alfred Perlstein To: Seigo Tanimura Cc: jake@FreeBSD.org, jhb@FreeBSD.org, current@FreeBSD.org Subject: Re: Lock of struct filedesc, file, pgrp, session and sigio Message-ID: <20010712032851.I2449@sneakerz.org> References: <20010602125223.J31257@dragon.nuxi.com> <200106040748.f547mUD53783@rina.r.dl.itc.u-tokyo.ac.jp> <200106181004.f5IA4VD63112@rina.r.dl.itc.u-tokyo.ac.jp> <200107020812.f628CfK44241@rina.r.dl.itc.u-tokyo.ac.jp> <20010707164249.C88962@sneakerz.org> <20010709032044.B1894@sneakerz.org> <200107100845.f6A8jqt99404@rina.r.dl.itc.u-tokyo.ac.jp> <20010710035347.Q1894@sneakerz.org> <200107100903.f6A93et02367@rina.r.dl.itc.u-tokyo.ac.jp> <200107111137.f6BBbQt22812@rina.r.dl.itc.u-tokyo.ac.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2i In-Reply-To: <200107111137.f6BBbQt22812@rina.r.dl.itc.u-tokyo.ac.jp>; from tanimura@r.dl.itc.u-tokyo.ac.jp on Wed, Jul 11, 2001 at 08:37:26PM +0900 Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG * Seigo Tanimura [010711 19:08] wrote: > > The patch and the results of build test are now on the web page. > > The discussion of ktrace(2) problem does not cover the solution of > BSD/OS, so it needs updating. Right now I'm only reviewing the file part and I'm only up to 'sys/i386/ibcs2/ibcs2_misc.c' in the diff. Here's a diff against your version and some comments so far: svr4_sys_putmsg svr4_sys_getmsg should not use FFIND, needs FFIND_HOLD fdesc_lookup probably should FFIND_HOLD to make sure the file doesn't go away. locking filedesc is probably not sufficient ibcs2_ioctl needs to be converted to FFIND_HOLD like linux's ioctl Basically, I'm pretty sure you need to FFIND_HOLD a lot more than you're doing in order to protect against shared file descriptor tables getting corrupted. diff --exclude=CVS -ur ./alpha/osf1/osf1_misc.c /usr/src/sys/alpha/osf1/osf1_misc.c --- ./alpha/osf1/osf1_misc.c Thu Jul 12 08:04:26 2001 +++ /usr/src/sys/alpha/osf1/osf1_misc.c Thu Jul 12 06:21:21 2001 @@ -670,11 +670,12 @@ struct osf1_stat oub; int error; - FFIND(fp, p, SCARG(uap, fd)); + FFIND_HOLD(fp, p, uap->fd); if (fp == NULL) return (EBADF); error = fo_stat(fp, &ub, p); + fdrop(fp, p); cvtstat2osf1(&ub, &oub); if (error == 0) error = copyout((caddr_t)&oub, (caddr_t)SCARG(uap, sb), diff --exclude=CVS -ur ./alpha/osf1/osf1_mount.c /usr/src/sys/alpha/osf1/osf1_mount.c --- ./alpha/osf1/osf1_mount.c Thu Jul 12 08:04:26 2001 +++ /usr/src/sys/alpha/osf1/osf1_mount.c Thu Jul 12 06:20:50 2001 @@ -154,7 +154,7 @@ struct statfs *sp; struct osf1_statfs osfs; - error = getvnode(p->p_fd, SCARG(uap, fd), &fp); + error = getvnode(p->p_fd, uap->fd, &fp); if (error) return (error); mp = ((struct vnode *)fp->f_data)->v_mount; diff --exclude=CVS -ur ./compat/linux/linux_file.c /usr/src/sys/compat/linux/linux_file.c --- ./compat/linux/linux_file.c Thu Jul 12 08:04:27 2001 +++ /usr/src/sys/compat/linux/linux_file.c Thu Jul 12 06:46:27 2001 @@ -139,11 +139,12 @@ SESS_LEADER(p) && !(p->p_flag & P_CONTROLT)) { struct file *fp; - FFIND(fp, p, p->p_retval[0]); + FFIND_HOLD(fp, p, p->p_retval[0]); SESS_UNLOCK(p->p_session); PROC_UNLOCK(p); if (fp->f_type == DTYPE_VNODE) fo_ioctl(fp, TIOCSCTTY, (caddr_t) 0, p); + fdrop(fp, p); } else { SESS_UNLOCK(p->p_session); PROC_UNLOCK(p); @@ -309,15 +310,19 @@ * significant effect for pipes (SIGIO is not delivered for * pipes under Linux-2.2.35 at least). */ - FFIND(fp, p, args->fd); + FFIND_HOLD(fp, p, args->fd); if (fp == NULL) return EBADF; - if (fp->f_type == DTYPE_PIPE) + if (fp->f_type == DTYPE_PIPE) { + fdrop(fp, p); return EINVAL; + } fcntl_args.cmd = F_SETOWN; fcntl_args.arg = args->arg; - return fcntl(p, &fcntl_args); + error = fcntl(p, &fcntl_args); + fdrop(fp, p); + return (error); } return EINVAL; } diff --exclude=CVS -ur ./compat/linux/linux_ioctl.c /usr/src/sys/compat/linux/linux_ioctl.c --- ./compat/linux/linux_ioctl.c Thu Jul 12 08:04:27 2001 +++ /usr/src/sys/compat/linux/linux_ioctl.c Thu Jul 12 07:52:31 2001 @@ -1437,10 +1437,12 @@ printf(ARGS(ioctl, "%d, %04lx, *"), args->fd, args->cmd); #endif - FFIND_LOCK(fp, p, args->fd); - if (fp == NULL || (fp->f_flag & (FREAD|FWRITE)) == 0) { - if (fp != NULL) - FILE_UNLOCK(fp); + FFIND_HOLD(fp, p, args->fd); + if (fp == NULL) + return (EBADF); + FILE_LOCK(fp); + if ((fp->f_flag & (FREAD|FWRITE)) == 0) { + FILE_UNLOCK(fp); return (EBADF); } FILE_UNLOCK(fp); @@ -1451,9 +1453,11 @@ if (cmd >= he->low && cmd <= he->high) { error = (*he->func)(p, args); if (error != ENOIOCTL) + fdrop(fp, p); return (error); } } + fdrop(fp, p); printf("linux: 'ioctl' fd=%d, cmd=0x%x ('%c',%d) not implemented\n", args->fd, (int)(args->cmd & 0xffff), diff --exclude=CVS -ur ./compat/linux/linux_stats.c /usr/src/sys/compat/linux/linux_stats.c --- ./compat/linux/linux_stats.c Thu Jul 12 08:04:27 2001 +++ /usr/src/sys/compat/linux/linux_stats.c Thu Jul 12 06:12:11 2001 @@ -218,11 +218,12 @@ printf(ARGS(newfstat, "%d, *"), args->fd); #endif - FFIND(fp, p, args->fd); + FFIND_HOLD(fp, p, args->fd); if (fp == NULL) return (EBADF); error = fo_stat(fp, &buf, p); + fdrop(fp, p); if (!error) error = newstat_copyout(&buf, args->buf); diff --exclude=CVS -ur ./compat/svr4/svr4_ioctl.c /usr/src/sys/compat/svr4/svr4_ioctl.c --- ./compat/svr4/svr4_ioctl.c Thu Jul 12 08:04:27 2001 +++ /usr/src/sys/compat/svr4/svr4_ioctl.c Thu Jul 12 08:01:13 2001 @@ -87,6 +87,7 @@ u_long cmd; int (*fun) __P((struct file *, struct proc *, register_t *, int, u_long, caddr_t)); + int error; #ifdef DEBUG_SVR4 char dir[4]; char c; @@ -101,10 +102,11 @@ retval = p->p_retval; cmd = SCARG(uap, com); - FFIND_LOCK(fp, p, SCARG(uap, fd)); + FFIND_HOLD(fp, p, SCARG(uap, fd)); if (fp == NULL) return EBADF; + FILE_LOCK(fp); if ((fp->f_flag & (FREAD | FWRITE)) == 0) { FILE_UNLOCK(fp); return EBADF; @@ -163,5 +165,7 @@ DPRINTF((">>> OUT: so_state = 0x%x\n", so->so_state)); } #endif - return (*fun)(fp, p, retval, SCARG(uap, fd), cmd, SCARG(uap, data)); + error = (*fun)(fp, p, retval, SCARG(uap, fd), cmd, SCARG(uap, data)); + fdrop(fp, p); + return (error); } diff --exclude=CVS -ur ./fs/fdescfs/fdesc_vnops.c /usr/src/sys/fs/fdescfs/fdesc_vnops.c --- ./fs/fdescfs/fdesc_vnops.c Thu Jul 12 08:04:29 2001 +++ /usr/src/sys/fs/fdescfs/fdesc_vnops.c Thu Jul 12 06:13:19 2001 @@ -301,12 +301,13 @@ case Fdesc: fd = VTOFDESC(vp)->fd_fd; - FFIND(fp, ap->a_p, fd); + FFIND_HOLD(fp, ap->a_p, fd); if (fp == NULL) return (EBADF); bzero(&stb, sizeof(stb)); error = fo_stat(fp, &stb, ap->a_p); + fdrop(fp, ap->a_p); if (error == 0) { VATTR_NULL(vap); vap->va_type = IFTOVT(stb.st_mode); diff --exclude=CVS -ur ./i386/ibcs2/ibcs2_fcntl.c /usr/src/sys/i386/ibcs2/ibcs2_fcntl.c --- ./i386/ibcs2/ibcs2_fcntl.c Thu Jul 12 08:04:30 2001 +++ /usr/src/sys/i386/ibcs2/ibcs2_fcntl.c Thu Jul 12 08:20:39 2001 @@ -197,13 +197,14 @@ if (!ret && !noctty && SESS_LEADER(p) && !(p->p_flag & P_CONTROLT)) { struct file *fp; - FFIND(fp, p, p->p_retval[0]); + FFIND_HOLD(fp, p, p->p_retval[0]); SESS_UNLOCK(p->p_session); PROC_UNLOCK(p); /* ignore any error, just give it a try */ if (fp->f_type == DTYPE_VNODE) fo_ioctl(fp, TIOCSCTTY, (caddr_t) 0, p); + fdrop(fp, p); } else { SESS_UNLOCK(p->p_session); PROC_UNLOCK(p); diff --exclude=CVS -ur ./sys/file.h /usr/src/sys/sys/file.h --- ./sys/file.h Thu Jul 12 08:04:35 2001 +++ /usr/src/sys/sys/file.h Thu Jul 12 07:47:18 2001 @@ -165,10 +165,7 @@ { int error; - fhold(fp); - error = (*fp->f_ops->fo_read)(fp, uio, cred, flags, p); - fdrop(fp, p); - return (error); + return ((*fp->f_ops->fo_read)(fp, uio, cred, flags, p)); } static __inline int @@ -181,10 +178,7 @@ { int error; - fhold(fp); - error = (*fp->f_ops->fo_write)(fp, uio, cred, flags, p); - fdrop(fp, p); - return (error); + return ((*fp->f_ops->fo_write)(fp, uio, cred, flags, p)); } static __inline int @@ -196,10 +190,7 @@ { int error; - fhold(fp); - error = (*fp->f_ops->fo_ioctl)(fp, com, data, p); - fdrop(fp, p); - return (error); + return ((*fp->f_ops->fo_ioctl)(fp, com, data, p)); } static __inline int @@ -224,9 +215,7 @@ { int error; - fhold(fp); error = (*fp->f_ops->fo_stat)(fp, sb, p); - fdrop(fp, p); return (error); } -- -Alfred Perlstein [alfred@freebsd.org] Ok, who wrote this damn function called '??'? And why do my programs keep crashing in it? To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message