Re: svn commit: r367927 - in head: sys/kern tests/sys/kern
On Sun, Nov 22, 2020 at 11:25 AM Kyle Evans wrote:
>
> 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
> > > > 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 0x80655c72 in vpanic (fmt=, ap=)
> > > at /usr/src/sys/kern/kern_shutdown.c:907
> > > #11 0x80655a03 in panic (
> > > fmt=0x80eb2b78 "헝\200\377\377\377\377")
> > > at /usr/src/sys/kern/kern_shutdown.c:843
> > > #12 0x805fff9a in filecaps_copy_prep (src=)
> > > at /usr/src/sys/kern/kern_descrip.c:1629
> > > #13 kern_dup (td=, mode=, flags=0,
> > > old=, new=256) at /usr/src/sys/kern/kern_descrip.c:970
> > > #14 0x8094a5de in syscallenter (td=)
> > > at /usr/src/sys/amd64/amd64/../../kern/subr_syscall.c:189
> > > #15 amd64_syscall (td=0xfe00513f8500, traced=0)
> > > at /usr/src/sys/amd64/amd64/trap.c:1156
> > >
> > >
> > > Simplified test program that causes panic:
> > > #include
> > > #include
> > >
> > > 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.
>
Bah, sorry, this still isn't right. The other paths may grow the table
via fdalloc(). I'll throw up a review for this shortly.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367927 - in head: sys/kern tests/sys/kern
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
> > > 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 0x80655c72 in vpanic (fmt=, ap=)
> > at /usr/src/sys/kern/kern_shutdown.c:907
> > #11 0x80655a03 in panic (
> > fmt=0x80eb2b78 "헝\200\377\377\377\377")
> > at /usr/src/sys/kern/kern_shutdown.c:843
> > #12 0x805fff9a in filecaps_copy_prep (src=)
> > at /usr/src/sys/kern/kern_descrip.c:1629
> > #13 kern_dup (td=, mode=, flags=0,
> > old=, new=256) at /usr/src/sys/kern/kern_descrip.c:970
> > #14 0x8094a5de in syscallenter (td=)
> > at /usr/src/sys/amd64/amd64/../../kern/subr_syscall.c:189
> > #15 amd64_syscall (td=0xfe00513f8500, traced=0)
> > at /usr/src/sys/amd64/amd64/trap.c:1156
> >
> >
> > Simplified test program that causes panic:
> > #include
> > #include
> >
> > 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);
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367927 - in head: sys/kern tests/sys/kern
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
> > 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 0x80655c72 in vpanic (fmt=, ap=)
> at /usr/src/sys/kern/kern_shutdown.c:907
> #11 0x80655a03 in panic (
> fmt=0x80eb2b78 "헝\200\377\377\377\377")
> at /usr/src/sys/kern/kern_shutdown.c:843
> #12 0x805fff9a in filecaps_copy_prep (src=)
> at /usr/src/sys/kern/kern_descrip.c:1629
> #13 kern_dup (td=, mode=, flags=0,
> old=, new=256) at /usr/src/sys/kern/kern_descrip.c:970
> #14 0x8094a5de in syscallenter (td=)
> at /usr/src/sys/amd64/amd64/../../kern/subr_syscall.c:189
> #15 amd64_syscall (td=0xfe00513f8500, traced=0)
> at /usr/src/sys/amd64/amd64/trap.c:1156
>
>
> Simplified test program that causes panic:
> #include
> #include
>
> 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.
Thanks,
Kyle Evans
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367927 - in head: sys/kern tests/sys/kern
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 0x80655c72 in vpanic (fmt=, ap=)
at /usr/src/sys/kern/kern_shutdown.c:907
#11 0x80655a03 in panic (
fmt=0x80eb2b78 "헝\200\377\377\377\377")
at /usr/src/sys/kern/kern_shutdown.c:843
#12 0x805fff9a in filecaps_copy_prep (src=)
at /usr/src/sys/kern/kern_descrip.c:1629
#13 kern_dup (td=, mode=, flags=0,
old=, new=256) at /usr/src/sys/kern/kern_descrip.c:970
#14 0x8094a5de in syscallenter (td=)
at /usr/src/sys/amd64/amd64/../../kern/subr_syscall.c:189
#15 amd64_syscall (td=0xfe00513f8500, traced=0)
at /usr/src/sys/amd64/amd64/trap.c:1156
Simplified test program that causes panic:
#include
#include
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;
}
Thanks,
Guy Yur
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r367927 - in head: sys/kern tests/sys/kern
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
Modified: head/sys/kern/kern_descrip.c
==
--- head/sys/kern/kern_descrip.cSun Nov 22 04:29:55 2020
(r367926)
+++ head/sys/kern/kern_descrip.cSun Nov 22 05:00:28 2020
(r367927)
@@ -1801,8 +1801,12 @@ fdgrowtable(struct filedesc *fdp, int nfd)
atomic_store_rel_ptr((volatile void *)&fdp->fd_files,
(uintptr_t)ntable);
/*
-* Do not free the old file table, as some threads may still
-* reference entries within it. Instead, place it on a freelist
+* Free the old file table when not shared by other threads or
processes.
+* The old file table is considered to be shared when either are true:
+* - The process has more than one thread.
+* - The file descriptor table has been shared via fdshare().
+*
+* When shared, the old file table will be placed on a freelist
* which will be processed when the struct filedesc is released.
*
* Note that if onfiles == NDFILE, we're dealing with the original
@@ -1810,10 +1814,14 @@ fdgrowtable(struct filedesc *fdp, int nfd)
* which must not be freed.
*/
if (onfiles > NDFILE) {
- ft = (struct freetable *)&otable->fdt_ofiles[onfiles];
- fdp0 = (struct filedesc0 *)fdp;
- ft->ft_table = otable;
- SLIST_INSERT_HEAD(&fdp0->fd_free, ft, ft_next);
+ if (curproc->p_numthreads == 1 && fdp->fd_refcnt == 1)
+ free(otable, M_FILEDESC);
+ else {
+ ft = (struct freetable *)&otable->fdt_ofiles[onfiles];
+ fdp0 = (struct filedesc0 *)fdp;
+ ft->ft_table = otable;
+ SLIST_INSERT_HEAD(&fdp0->fd_free, ft, ft_next);
+ }
}
/*
* The map does not have the same possibility of threads still
Modified: head/tests/sys/kern/Makefile
==
--- head/tests/sys/kern/MakefileSun Nov 22 04:29:55 2020
(r367926)
+++ head/tests/sys/kern/MakefileSun Nov 22 05:00:28 2020
(r367927)
@@ -10,6 +10,7 @@ TESTSDIR= ${TESTSBASE}/sys/kern
#ATF_TESTS_C+= kcov
ATF_TESTS_C+= kern_copyin
ATF_TESTS_C+= kern_descrip_test
+ATF_TESTS_C+= fdgrowtable_test
ATF_TESTS_C+= kill_zombie
ATF_TESTS_C+= ptrace_test
TEST_METADATA.ptrace_test+=timeout="15"
@@ -46,6 +47,7 @@ LIBADD.ptrace_test+= pthread
LIBADD.unix_seqpacket_test+= pthread
LIBADD.kcov+= pthread
LIBADD.sendfile_helper+= pthread
+LIBADD.fdgrowtable_test+= util pthread kvm procstat
NETBSD_ATF_TESTS_C+= lockf_test
NETBSD_ATF_TESTS_C+= mqueue_test
Added: head/tests/sys/kern/fdgrowtable_test.c
==
--- /dev/null 00:00:00 1970 (empty, because file is newly added)
+++ head/tests/sys/kern/fdgrowtable_test.c Sun Nov 22 05:00:28 2020
(r367927)
@@ -0,0 +1,267 @@
+/*-
+ * SPDX-License-Identifier: BSD-2-Clause-FreeBSD
+ *
+ * Copyright (c) 2020 Rob Wing
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS
