fdcopy() w/o memcpy()

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

fdcopy() w/o memcpy()

Martin Pieuchot
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.

ok?

Index: kern/kern_descrip.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.150
diff -u -p -r1.150 kern_descrip.c
--- kern/kern_descrip.c 12 Apr 2018 10:30:18 -0000 1.150
+++ kern/kern_descrip.c 16 Apr 2018 10:23:44 -0000
@@ -989,18 +989,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
@@ -1008,45 +1009,33 @@ 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;
 
- fdplock(newfdp);
- fpp = newfdp->fd_ofiles;
- for (i = 0; i <= newfdp->fd_lastfile; i++, fpp++)
- if (*fpp != NULL) {
+ for (i = 0; i <= fdp->fd_lastfile; i++) {
+ struct file *fp = fdp->fd_ofiles[i];
+
+ if (fp != NULL) {
  /*
  * XXX Gruesome hack. If count gets too high, fail
  * to copy an fd, since fdcopy()'s callers do not
@@ -1055,22 +1044,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);
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: fdcopy() w/o memcpy()

Martin Pieuchot
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,

Reply | Threaded
Open this post in threaded view
|

Re: fdcopy() w/o memcpy()

Martin Pieuchot
On 18/04/18(Wed) 12:06, Martin Pieuchot wrote:

> 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@.

New version with more fixes from visa@:
 - Do not use `i' uninitialized
 - Pass M_ZERO to map allocations

Index: kern/kern_descrip.c
===================================================================
RCS file: 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 14:38:01 -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 | M_ZERO);
  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 {
- 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);
+ if (NDHISLOTS(newfdp->fd_nfiles) > NDHISLOTS(NDFILE)) {
+ newfdp->fd_himap = mallocarray(NDHISLOTS(newfdp->fd_nfiles),
+    sizeof(u_int), M_FILEDESC, M_WAITOK | M_ZERO);
+ newfdp->fd_lomap = mallocarray(NDLOSLOTS(newfdp->fd_nfiles),
+    sizeof(u_int), M_FILEDESC, M_WAITOK | M_ZERO);
+ }
+ newfdp->fd_freefile = fdp->fd_freefile;
+ newfdp->fd_flags = fdp->fd_flags;
+ newfdp->fd_cmask = fdp->fd_cmask;
 
- fdplock(newfdp);
- fpp = newfdp->fd_ofiles;
- for (i = 0; i <= newfdp->fd_lastfile; i++, fpp++)
- if (*fpp != NULL) {
+ for (i = 0; i <= fdp->fd_lastfile; i++) {
+ struct file *fp = fdp->fd_ofiles[i];
+
+ 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);
 }