Re: fdcopy() w/o memcpy()
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 - 1.151
+++ kern/kern_descrip.c 18 Apr 2018 14:38:01 -
@@ -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(_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(>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 *) >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
Re: fdcopy() w/o memcpy()
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 - 1.151
+++ kern/kern_descrip.c 18 Apr 2018 10:04:13 -
@@ -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(_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(>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 *) >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
-
fdcopy() w/o memcpy()
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 - 1.150
+++ kern/kern_descrip.c 16 Apr 2018 10:23:44 -
@@ -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(_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(>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 *) >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;
- /*
