From owner-svn-src-all@freebsd.org Sun Nov 22 17:25:14 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 1280046E734; Sun, 22 Nov 2020 17:25:14 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CfHGx6GjMz3kZX; Sun, 22 Nov 2020 17:25:13 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from mail-qk1-f173.google.com (mail-qk1-f173.google.com [209.85.222.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) (Authenticated sender: kevans) by smtp.freebsd.org (Postfix) with ESMTPSA id BD4587030; Sun, 22 Nov 2020 17:25:13 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: by mail-qk1-f173.google.com with SMTP id u4so14279247qkk.10; Sun, 22 Nov 2020 09:25:13 -0800 (PST) X-Gm-Message-State: AOAM531MomXISRBMA59PW5HHXT30578LqYu0uo4g+3vnPQ2O9eKXcv0b A6DDq5qv7sAQTNahLSo4lDR9LEk9+pgVmMeWnxE= X-Google-Smtp-Source: ABdhPJzCLoJCxRtgaOBO+9xMX37EUZS9pv/9jduxZbRaNe52wbi7Ll6vVfYuJlsOAatDVXVfnBBZOGmgu+HVZ6ZvzS8= X-Received: by 2002:a37:9f82:: with SMTP id i124mr26486629qke.493.1606065913252; Sun, 22 Nov 2020 09:25:13 -0800 (PST) MIME-Version: 1.0 References: <202011220500.0AM50Tml047660@repo.freebsd.org> <75a09790-79d8-f539-e118-acf32b6bc65e@gmail.com> In-Reply-To: From: Kyle Evans Date: Sun, 22 Nov 2020 11:25:02 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r367927 - in head: sys/kern tests/sys/kern To: Guy Yur Cc: Robert Wing , src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 22 Nov 2020 17:25:14 -0000 On Sun, Nov 22, 2020 at 10:36 AM Kyle Evans wrote: > > On Sun, Nov 22, 2020 at 9:54 AM Guy Yur 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 al= located. When > > > a new table is allocated, the old table is placed in a free list a= nd held onto > > > until all processes referencing them exit. > > > > > > When a new file descriptor table is allocated, the old file descri= ptor 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 !=3D NULL, but fc_nioctls=3D-16162 > > ... > > #10 0xffffffff80655c72 in vpanic (fmt=3D, ap=3D) > > at /usr/src/sys/kern/kern_shutdown.c:907 > > #11 0xffffffff80655a03 in panic ( > > fmt=3D0xffffffff80eb2b78 "=ED=97=9D\200\377\377\377\3= 77") > > at /usr/src/sys/kern/kern_shutdown.c:843 > > #12 0xffffffff805fff9a in filecaps_copy_prep (src=3D) > > at /usr/src/sys/kern/kern_descrip.c:1629 > > #13 kern_dup (td=3D, mode=3D, flags=3D0, > > old=3D, new=3D256) at /usr/src/sys/kern/kern_descri= p.c:970 > > #14 0xffffffff8094a5de in syscallenter (td=3D) > > at /usr/src/sys/amd64/amd64/../../kern/subr_syscall.c:189 > > #15 amd64_syscall (td=3D0xfffffe00513f8500, traced=3D0) > > at /usr/src/sys/amd64/amd64/trap.c:1156 > > > > > > Simplified test program that causes panic: > > #include > > #include > > > > int main () > > { > > int bad_fd =3D 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. =3D\ > > 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 >=3D fdp->fd_nfiles) { +#ifdef INVARIANTS + struct file *ofp; + + ofp =3D 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 =3D &fdp->fd_ofiles[old]; + KASSERT(ofp =3D=3D oldfde->fde_file, + ("fdt_ofiles shift from growth observed at fd %= d", + old)); } if (!fdisused(fdp, new)) fdused(fdp, new);