On 20/06/18(Wed) 17:16, Mark Kettenis wrote:
> > Date: Wed, 20 Jun 2018 15:17:27 +0200
> > From: Martin Pieuchot <m...@openbsd.org>
>
> Joel, the diff below changes the semantics of the DIOCMAP ioctl a bit.
> See the bit about DIOCMAP below. I think this approach is better.
> But maybe I'm missing someting...
>
> Martin, I like fnew(). I can use that in the prime/DRI3 stuff as
> well. I'm still not convinced that inserting the struct file into the
> list late is the right choice. Yes, inserting it early (in fnew())
> means that tools like fstat may see a struct file that is in an
> inconsistent state. But I'm not sure we can guarantee that anyway.
Let's see how we can insert the struct file early once visa@'s locking
is in. Does it sound like a plan?
> That would allow you to atomically replace the pointer in the file
> descriptor table and be done. Actually I think you could already
> simplify the code and skip the "Zap old file" bit. Just do a
>
> fdinsert(fdp, fd, 0, fp)
>
> which will retain the flags.
To be able to do that I have to remove the KASSERT() in fdinsert().
This doesn't make me happy because I like to assert that the given slot
is unused.
I could have used fdremove() instead of fiddling with the array directly
but then I'd have to export & call fd_used(). So for the moment I'd
prefer to keep this code "stupid", hopefully we can design a better API
to manipulate the arrays.
> Now if you really prefer to keep inserting late, could we change
> fdinsert to only insert if FIF_INSERTED isn't set already? That way
> we can use it to insert the same open file into multiple file
> descriptor tables. I can include such a change in my prime/DRI3 diff.
Sure, updated diff below.
Index: dev/diskmap.c
===================================================================
RCS file: /cvs/src/sys/dev/diskmap.c,v
retrieving revision 1.20
diff -u -p -r1.20 diskmap.c
--- dev/diskmap.c 9 May 2018 08:42:02 -0000 1.20
+++ dev/diskmap.c 20 Jun 2018 13:14:06 -0000
@@ -57,9 +57,9 @@ diskmapioctl(dev_t dev, u_long cmd, cadd
struct dk_diskmap *dm;
struct nameidata ndp;
struct filedesc *fdp = p->p_fd;
- struct file *fp = NULL;
- struct vnode *vp = NULL, *ovp;
- char *devname;
+ struct file *fp0 = NULL, *fp = NULL;
+ struct vnode *vp = NULL;
+ char *devname, flags;
int fd, error = EINVAL;
if (cmd != DIOCMAP)
@@ -80,57 +80,59 @@ diskmapioctl(dev_t dev, u_long cmd, cadd
goto invalid;
/* Attempt to open actual device. */
- if ((error = getvnode(p, fd, &fp)) != 0)
+ if ((error = getvnode(p, fd, &fp0)) != 0)
goto invalid;
- KASSERT(fp->f_type == DTYPE_VNODE);
- KASSERT(fp->f_ops == &vnops);
-
- 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)
+ if ((error = vn_open(&ndp, fp0->f_flag, 0)) != 0)
goto bad;
vp = ndp.ni_vp;
+ VOP_UNLOCK(vp);
- /* Close the original vnode. */
- ovp = (struct vnode *)fp->f_data;
- if (fp->f_flag & FWRITE)
- ovp->v_writecount--;
-
- if (ovp->v_writecount == 0) {
- vn_lock(ovp, LK_EXCLUSIVE | LK_RETRY);
- VOP_CLOSE(ovp, fp->f_flag, p->p_ucred, p);
- vput(ovp);
+ fdplock(fdp);
+ /* We lost a race, don't go any further. */
+ if (fdp->fd_ofiles[fd] != fp0) {
+ error = EAGAIN;
+ goto bad;
}
- fp->f_data = (caddr_t)vp;
- fp->f_offset = 0;
- mtx_enter(&fp->f_mtx);
- fp->f_rxfer = 0;
- fp->f_wxfer = 0;
- fp->f_seek = 0;
- fp->f_rbytes = 0;
- fp->f_wbytes = 0;
- mtx_leave(&fp->f_mtx);
-
- VOP_UNLOCK(vp);
+ fp = fnew(p);
+ if (fp == NULL) {
+ error = ENFILE;
+ goto bad;
+ }
- FRELE(fp, p);
+ /* Zap old file. */
+ mtx_enter(&fhdlk);
+ KASSERT(fdp->fd_ofiles[fd] == fp0);
+ flags = fdp->fd_ofileflags[fd];
+ fdp->fd_ofiles[fd] = NULL;
+ fdp->fd_ofileflags[fd] = 0;
+ mtx_leave(&fhdlk);
+
+ /* Insert new file. */
+ fp->f_flag = fp0->f_flag;
+ fp->f_type = DTYPE_VNODE;
+ fp->f_ops = &vnops;
+ fp->f_data = (caddr_t)vp;
+ fdinsert(fdp, fd, flags, fp);
fdpunlock(fdp);
+ FRELE(fp, p);
+
+ closef(fp0, p);
free(devname, M_DEVBUF, PATH_MAX);
return 0;
bad:
- if (vp)
- vput(vp);
- if (fp)
- FRELE(fp, p);
-
fdpunlock(fdp);
+
+ if (vp)
+ vrele(vp);
+ if (fp0)
+ FRELE(fp0, p);
invalid:
free(devname, M_DEVBUF, PATH_MAX);
Index: kern/kern_descrip.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.167
diff -u -p -r1.167 kern_descrip.c
--- kern/kern_descrip.c 20 Jun 2018 10:52:49 -0000 1.167
+++ kern/kern_descrip.c 21 Jun 2018 07:48:20 -0000
@@ -686,15 +686,17 @@ fdinsert(struct filedesc *fdp, int fd, i
fdpassertlocked(fdp);
mtx_enter(&fhdlk);
- if ((fq = fdp->fd_ofiles[0]) != NULL) {
- LIST_INSERT_AFTER(fq, fp, f_list);
- } else {
- LIST_INSERT_HEAD(&filehead, fp, f_list);
+ if ((fp->f_iflags & FIF_INSERTED) == 0) {
+ fp->f_iflags |= FIF_INSERTED;
+ if ((fq = fdp->fd_ofiles[0]) != NULL) {
+ LIST_INSERT_AFTER(fq, fp, f_list);
+ } else {
+ LIST_INSERT_HEAD(&filehead, fp, f_list);
+ }
}
KASSERT(fdp->fd_ofiles[fd] == NULL);
fdp->fd_ofiles[fd] = fp;
fdp->fd_ofileflags[fd] |= (flags & UF_EXCLOSE);
- fp->f_iflags |= FIF_INSERTED;
mtx_leave(&fhdlk);
}
@@ -971,11 +973,29 @@ restart:
}
return (error);
}
- if (numfiles >= maxfiles) {
+
+ fp = fnew(p);
+ if (fp == NULL) {
fd_unused(p->p_fd, i);
- tablefull("file");
return (ENFILE);
}
+
+ *resultfp = fp;
+ *resultfd = i;
+
+ return (0);
+}
+
+struct file *
+fnew(struct proc *p)
+{
+ struct file *fp;
+
+ if (numfiles >= maxfiles) {
+ tablefull("file");
+ return (NULL);
+ }
+
/*
* Allocate a new file descriptor.
* If the process has file descriptor zero open, add to the list
@@ -992,14 +1012,12 @@ restart:
fp->f_count = 1;
fp->f_cred = p->p_ucred;
crhold(fp->f_cred);
- *resultfp = fp;
- *resultfd = i;
mtx_enter(&fhdlk);
fp->f_count++;
mtx_leave(&fhdlk);
- return (0);
+ return (fp);
}
/*
Index: sys/filedesc.h
===================================================================
RCS file: /cvs/src/sys/sys/filedesc.h,v
retrieving revision 1.39
diff -u -p -r1.39 filedesc.h
--- sys/filedesc.h 18 Jun 2018 09:15:05 -0000 1.39
+++ sys/filedesc.h 20 Jun 2018 11:49:07 -0000
@@ -120,6 +120,7 @@ void filedesc_init(void);
int dupfdopen(struct proc *, int, int);
int fdalloc(struct proc *p, int want, int *result);
void fdexpand(struct proc *);
+struct file *fnew(struct proc *_p);
int falloc(struct proc *_p, struct file **_rfp, int *_rfd);
struct filedesc *fdinit(void);
struct filedesc *fdshare(struct process *);
