Date: Thu, 12 Jul 2001 03:28:51 -0500 From: Alfred Perlstein <bright@sneakerz.org> To: Seigo Tanimura <tanimura@r.dl.itc.u-tokyo.ac.jp> 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> 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 %2B0900 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>
next in thread | previous in thread | raw e-mail | index | archive | help
* Seigo Tanimura <tanimura@r.dl.itc.u-tokyo.ac.jp> [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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010712032851.I2449>