On 16/04/18(Mon) 12:33, Martin Pieuchot wrote:
> Diff below is a rewrite/cleanup of fdcopy() to avoid using memcpy().
>
> The problem with the memcpys is that they introduce a window where
> some 'struct file *' are duplicated without being refcounted. We
> should instead use the following common idiom:
>
> FREF(fp);
> fdp->fd_ofiles[i] = fp;
> fdp->fd_ofileflags[i] = flags;
> fd_used(fdp, i);
>
> I'm also reusing fdinit() to simplify the function.
Updated diff that also copies fd_cmask and corrects an off-by-one in
a comparison, both errors pointed by visa@.
ok?
Index: kern/kern_descrip.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.151
diff -u -p -r1.151 kern_descrip.c
--- kern/kern_descrip.c 18 Apr 2018 09:59:09 -0000 1.151
+++ kern/kern_descrip.c 18 Apr 2018 10:04:13 -0000
@@ -994,18 +994,19 @@ struct filedesc *
fdcopy(struct process *pr)
{
struct filedesc *newfdp, *fdp = pr->ps_fd;
- struct file **fpp;
int i;
+ newfdp = fdinit();
+
fdplock(fdp);
- newfdp = pool_get(&fdesc_pool, PR_WAITOK);
- memcpy(newfdp, fdp, sizeof(struct filedesc));
- if (newfdp->fd_cdir)
- vref(newfdp->fd_cdir);
- if (newfdp->fd_rdir)
- vref(newfdp->fd_rdir);
- newfdp->fd_refcnt = 1;
- rw_init(&newfdp->fd_lock, "fdlock");
+ if (fdp->fd_cdir) {
+ vref(fdp->fd_cdir);
+ newfdp->fd_cdir = fdp->fd_cdir;
+ }
+ if (fdp->fd_rdir) {
+ vref(fdp->fd_rdir);
+ newfdp->fd_rdir = fdp->fd_rdir;
+ }
/*
* If the number of open files fits in the internal arrays
@@ -1013,45 +1014,34 @@ fdcopy(struct process *pr)
* additional memory for the number of descriptors currently
* in use.
*/
- if (newfdp->fd_lastfile < NDFILE) {
- newfdp->fd_ofiles = ((struct filedesc0 *) newfdp)->fd_dfiles;
- newfdp->fd_ofileflags =
- ((struct filedesc0 *) newfdp)->fd_dfileflags;
- i = NDFILE;
- } else {
+ if (fdp->fd_lastfile > NDFILE) {
/*
* Compute the smallest multiple of NDEXTENT needed
* for the file descriptors currently in use,
* allowing the table to shrink.
*/
- i = newfdp->fd_nfiles;
- while (i >= 2 * NDEXTENT && i > newfdp->fd_lastfile * 2)
+ i = fdp->fd_nfiles;
+ while (i >= 2 * NDEXTENT && i > fdp->fd_lastfile * 2)
i /= 2;
- newfdp->fd_ofiles = mallocarray(i, OFILESIZE, M_FILEDESC,
M_WAITOK);
+ newfdp->fd_ofiles = mallocarray(i, OFILESIZE, M_FILEDESC,
+ M_WAITOK);
newfdp->fd_ofileflags = (char *) &newfdp->fd_ofiles[i];
+ newfdp->fd_nfiles = i;
}
- if (NDHISLOTS(i) <= NDHISLOTS(NDFILE)) {
- newfdp->fd_himap =
- ((struct filedesc0 *) newfdp)->fd_dhimap;
- newfdp->fd_lomap =
- ((struct filedesc0 *) newfdp)->fd_dlomap;
- } else {
+ if (NDHISLOTS(newfdp->fd_nfiles) > NDHISLOTS(NDFILE)) {
newfdp->fd_himap = mallocarray(NDHISLOTS(i), sizeof(u_int),
M_FILEDESC, M_WAITOK);
newfdp->fd_lomap = mallocarray(NDLOSLOTS(i), sizeof(u_int),
M_FILEDESC, M_WAITOK);
}
- newfdp->fd_nfiles = i;
- memcpy(newfdp->fd_ofiles, fdp->fd_ofiles, i * sizeof(struct file *));
- 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);
+ newfdp->fd_freefile = fdp->fd_freefile;
+ newfdp->fd_flags = fdp->fd_flags;
+ newfdp->fd_cmask = fdp->fd_cmask;
+
+ for (i = 0; i <= fdp->fd_lastfile; i++) {
+ struct file *fp = fdp->fd_ofiles[i];
- fdplock(newfdp);
- fpp = newfdp->fd_ofiles;
- for (i = 0; i <= newfdp->fd_lastfile; i++, fpp++)
- if (*fpp != NULL) {
+ if (fp != NULL) {
/*
* XXX Gruesome hack. If count gets too high, fail
* to copy an fd, since fdcopy()'s callers do not
@@ -1060,22 +1050,18 @@ fdcopy(struct process *pr)
* tied to the process that opened them to enforce
* their internal consistency, so close them here.
*/
- if ((*fpp)->f_count == LONG_MAX-2 ||
- (*fpp)->f_type == DTYPE_KQUEUE)
- fdremove(newfdp, i);
- else
- (*fpp)->f_count++;
- }
+ if (fp->f_count == LONG_MAX-2 ||
+ fp->f_type == DTYPE_KQUEUE)
+ continue;
- /* finish cleaning up kq bits */
- if (newfdp->fd_knlistsize != -1) {
- newfdp->fd_knlist = NULL;
- newfdp->fd_knlistsize = -1;
- newfdp->fd_knhash = NULL;
- newfdp->fd_knhashmask = 0;
+ FREF(fp);
+ newfdp->fd_ofiles[i] = fp;
+ newfdp->fd_ofileflags[i] = fdp->fd_ofileflags[i];
+ fd_used(newfdp, i);
+ }
}
+ fdpunlock(fdp);
- fdpunlock(newfdp);
return (newfdp);
}
@@ -1101,7 +1087,7 @@ fdfree(struct proc *p)
}
}
p->p_fd = NULL;
- if (fdp->fd_nfiles > NDFILE)
+ if (fdp->fd_nfiles >= NDFILE)
free(fdp->fd_ofiles, M_FILEDESC, fdp->fd_nfiles * OFILESIZE);
if (NDHISLOTS(fdp->fd_nfiles) > NDHISLOTS(NDFILE)) {
free(fdp->fd_himap, M_FILEDESC,
