svn commit: r367927 - in head: sys/kern tests/sys/kern

[Kyle Evans]

On Sun, Nov 22, 2020 at 10:36 AM Kyle Evans <kevans at freebsd.org> wrote:
>
> On Sun, Nov 22, 2020 at 9:54 AM Guy Yur <guyyur at gmail.com> wrote:
> >
> > On 22/11/20 7:00 am, Robert Wing wrote:
> > > Author: rew
> > > Date: Sun Nov 22 05:00:28 2020
> > > New Revision: 367927
> > > URL: https://svnweb.freebsd.org/changeset/base/367927
> > >
> > > Log:
> > >    fd: free old file descriptor tables when not shared
> > >
> > >    During the life of a process, new file descriptor tables may be allocated. When
> > >    a new table is allocated, the old table is placed in a free list and held onto
> > >    until all processes referencing them exit.
> > >
> > >    When a new file descriptor table is allocated, the old file descriptor table
> > >    can be freed when the current process has a single-thread and the file
> > >    descriptor table is not being shared with any other processes.
> > >
> > >    Reviewed by:    kevans
> > >    Approved by:    kevans (mentor)
> > >    Differential Revision:  https://reviews.freebsd.org/D18617
> > >
> > > Added:
> > >    head/tests/sys/kern/fdgrowtable_test.c   (contents, props changed)
> > > Modified:
> > >    head/sys/kern/kern_descrip.c
> > >    head/tests/sys/kern/Makefile
> >
> > Hi,
> >
> > I am getting a kernel panic with this commit when building
> > devel/gmake port and it runs dup2 test in configure script.
> >
> > panic: fc_ioctls != NULL, but fc_nioctls=-16162
> > ...
> > #10 0xffffffff80655c72 in vpanic (fmt=<optimized out>, ap=<optimized out>)
> >      at /usr/src/sys/kern/kern_shutdown.c:907
> > #11 0xffffffff80655a03 in panic (
> >      fmt=0xffffffff80eb2b78 <cnputs_mtx> "헝\200\377\377\377\377")
> >      at /usr/src/sys/kern/kern_shutdown.c:843
> > #12 0xffffffff805fff9a in filecaps_copy_prep (src=<optimized out>)
> >      at /usr/src/sys/kern/kern_descrip.c:1629
> > #13 kern_dup (td=<optimized out>, mode=<optimized out>, flags=0,
> >      old=<optimized out>, new=256) at /usr/src/sys/kern/kern_descrip.c:970
> > #14 0xffffffff8094a5de in syscallenter (td=<optimized out>)
> >      at /usr/src/sys/amd64/amd64/../../kern/subr_syscall.c:189
> > #15 amd64_syscall (td=0xfffffe00513f8500, traced=0)
> >      at /usr/src/sys/amd64/amd64/trap.c:1156
> >
> >
> > Simplified test program that causes panic:
> > #include <unistd.h>
> > #include <limits.h>
> >
> > int main ()
> > {
> >    int bad_fd = INT_MAX;
> >    dup2 (1, 1);
> >    close (0);
> >    dup2 (0, 0);
> >    dup2 (2, bad_fd);
> >    dup2 (2, -1);
> >    dup2 (2, 255);
> >    dup2 (2, 256);
> >    return 0;
> > }
> >
>
> Whoops. =\
>
> It looks like kern_dup grows the file table but assumes that it can
> continue using oldfe that it fetched from the now-freed table. I
> suspect we just need to refetch oldfde after the grow operation, and
> it might be a good idea (under INVARIANTS) to grab the fp from oldfde
> before we grow the table and assert that the new entry we fetch is the
> same underlying file.
>

I can confirm that the below fixes it and no other growth spots keep
pointers into the old table around, I'll give it a little bit for any
objections to be raised then commit.

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index b5680bd79e1..1a3a5cafbea 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -936,6 +936,12 @@ kern_dup(struct thread *td, u_int mode, int
flags, int old, int new)
                break;
        case FDDUP_FIXED:
                if (new >= fdp->fd_nfiles) {
+#ifdef INVARIANTS
+                       struct file *ofp;
+
+                       ofp = oldfde->fde_file;
+#endif
+
                        /*
                         * The resource limits are here instead of e.g.
                         * fdalloc(), because the file descriptor table may be
@@ -955,6 +961,11 @@ kern_dup(struct thread *td, u_int mode, int
flags, int old, int new)
                        }
 #endif
                        fdgrowtable_exp(fdp, new + 1);
+                       /* Refetch; old entry may be invalid. */
+                       oldfde = &fdp->fd_ofiles[old];
+                       KASSERT(ofp == oldfde->fde_file,
+                           ("fdt_ofiles shift from growth observed at fd %d",
+                           old));
                }
                if (!fdisused(fdp, new))
                        fdused(fdp, new);