Re: CVS commit: src
Le 25/02/2020 à 19:18, Maxime Villard a écrit : > Le 23/02/2020 à 23:19, Andrew Doran a écrit : >> On Fri, Feb 21, 2020 at 02:14:31PM +0100, Kamil Rytarowski wrote: >> >>> On 22.12.2019 20:47, Andrew Doran wrote: Module Name: src Committed By: ad Date: Sun Dec 22 19:47:35 UTC 2019 Modified Files: src/external/cddl/osnet/dist/uts/common/fs/zfs: zfs_ctldir.c src/sys/kern: vfs_mount.c vfs_subr.c vfs_syscalls.c src/sys/miscfs/genfs: genfs_vfsops.c src/sys/nfs: nfs_export.c src/sys/sys: mount.h vnode.h vnode_impl.h src/sys/ufs/lfs: ulfs_vfsops.c src/sys/ufs/ufs: ufs_vfsops.c ufs_wapbl.c Log Message: Make mntvnode_lock per-mount, and address false sharing of struct mount. >>> >>> This change broke kUBSan syzbot. >>> >>> The sanitizer is now very noisy as struct mount requires 64 byte alignment. >>> >>> http://netbsd.org/~kamil/kubsan/mount-alignment.txt >> >> I had a look this weekend. This is down to KMEM_SIZE messing up the >> alignment, so is a DIAGNOSTIC thing. The align_offset parameter to >> pool_cache() would be a nice easy way to solve this but it seems someone >> killed that off, so I'll need to give this some more thought. >> >> Andrew > > kmem guarantees alignment on 8 bytes, but not more. Changing the backend > allocators to enforce more alignment still may not result in aligned buffers, > because kmem has the right to modify the buffers for debugging purposes, like > with KMEM_SIZE. > > If you want a buffer aligned to a specific value, don't use kmem, rather a > pool(_cache) with COHERENCY_UNIT in "align". > > If the goal is to have kmem really enforce COHERENCY_UNIT alignment, then this > should be documented and the debugging features should be adapted to respect > that constraint. > > "align_offset" got removed because it increased complexity in subr_pool for > no reason (two users in all of the kernel, one was actually a bug). Can we revert the "__aligned(COHERENCY_UNIT)" for now? There is no particular hurry to fix this bug, however the KUBSAN instance has been down for more than two months because of this, and it needs to be addressed. Similarly, the KASAN instance is currently crashing hard on: https://syzkaller.appspot.com/bug?id=1aa3f789d356bf04644bcef632bf8c2373398ba2 Dozens of thousands of times each day. This has been the case for two weeks, and it too needs to be addressed. Thanks
Re: CVS commit: src/tests/lib/libc/sys
On 07.03.2020 15:53, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Sat Mar 7 14:53:14 UTC 2020 > > Modified Files: > src/tests/lib/libc/sys: t_ptrace_wait.c t_ptrace_wait.h > > Log Message: > Try to fix the build. This is why all those inlines should really be in a > separate file as regular function. The code is too large and hard to manage > this way, and only increases in complexity as time goes by. > > What build configuration was broken? signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On Sat, Mar 07, 2020 at 12:24:21PM +0100, Maxime Villard wrote: > Can we revert the "__aligned(COHERENCY_UNIT)" for now? There is no particular > hurry to fix this bug, however the KUBSAN instance has been down for more than > two months because of this, and it needs to be addressed. That should be quelled now. > Similarly, the KASAN instance is currently crashing hard on: > > https://syzkaller.appspot.com/bug?id=1aa3f789d356bf04644bcef632bf8c2373398ba2 > Dozens of thousands of times each day. This has been the case for two weeks, > and it too needs to be addressed. That's been there since I started looking last year. I guess it's a false positive because the sanitiser probably thinks objects are gone once pool_cache_put() is called, but the actual point of disposal is the pool_cache dtor. Andrew
Re: CVS commit: src/tests/lib/libc/sys
In article <5e528f7a-147a-23e7-46da-6b02d76e5...@gmx.com>, Kamil Rytarowski wrote: >-=-=-=-=-=- >-=-=-=-=-=- > >On 07.03.2020 15:53, Christos Zoulas wrote: >> Module Name: src >> Committed By:christos >> Date:Sat Mar 7 14:53:14 UTC 2020 >> >> Modified Files: >> src/tests/lib/libc/sys: t_ptrace_wait.c t_ptrace_wait.h >> >> Log Message: >> Try to fix the build. This is why all those inlines should really be in a >> separate file as regular function. The code is too large and hard to manage >> this way, and only increases in complexity as time goes by. >> >> > >What build configuration was broken? All of the evbarm ones since t_ptrace_sigchld.c was not including ieefp.h http://releng.netbsd.org/builds/HEAD/202003070040Z/evbarm-earmhfeb.build.failed christos
Re: CVS commit: src/sys/conf
On Sat, Mar 07, 2020 at 19:18:41 -0500, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Sun Mar 8 00:18:41 UTC 2020 > > Modified Files: > src/sys/conf: files > > Log Message: > undo previous since config has been fixed It's still not. Nested ifdefs are not handled correctly. -uwe
Re: CVS commit: src/lib/libc/compiler_rt
Oops, forgot to note that this is only for 68010, and other
systems including m68k are not affected.
Thanks,
rin
On 2020/03/08 15:30, Rin Okuyama wrote:
Module Name:src
Committed By: rin
Date: Sun Mar 8 06:30:06 UTC 2020
Modified Files:
src/lib/libc/compiler_rt: Makefile.inc
Log Message:
Fix broken printf(3) %d output for numbers more than two digits, e.g.,
printf("%d\n", 42) ---> "::" instead of "42"
Our __{,u}modsi3 codes assume that __udivsi3 returns remainder to
%d1 (volatile register). __udivsi3 in libgcc does not, and therefore
mixing them up results in mess.
To generate a diff of this commit:
cvs rdiff -u -r1.36 -r1.37 src/lib/libc/compiler_rt/Makefile.inc
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/lib/libc/compiler_rt/Makefile.inc
diff -u src/lib/libc/compiler_rt/Makefile.inc:1.36
src/lib/libc/compiler_rt/Makefile.inc:1.37
--- src/lib/libc/compiler_rt/Makefile.inc:1.36 Tue Oct 29 16:08:50 2019
+++ src/lib/libc/compiler_rt/Makefile.inc Sun Mar 8 06:30:06 2020
@@ -1,4 +1,4 @@
-# $NetBSD: Makefile.inc,v 1.36 2019/10/29 16:08:50 joerg Exp $
+# $NetBSD: Makefile.inc,v 1.37 2020/03/08 06:30:06 rin Exp $
COMPILER_RT_SRCDIR= ${NETBSDSRCDIR}/sys/external/bsd/compiler_rt/dist
@@ -170,9 +170,16 @@ GENERIC_SRCS+= \
GENERIC_SRCS+= \
divmodsi4.c \
divsi3.c \
- modsi3.c \
udivmodsi4.c \
+
+. if ${LIBC_MACHINE_ARCH} != "m68000"
+# Our __{,u}modsi3 codes assume that __udivsi3 returns remainder to
+# %d1 (volatile register). __udivsi3 in libgcc does not, and therefore
+# mixing them up results in mess.
+GENERIC_SRCS+= \
+ modsi3.c \
umodsi3.c
+. endif
. if ${LIBC_MACHINE_CPU} != "sh3"
# On sh3 __udivsi3 is gcc "millicode" with special calling convention
Re: CVS commit: src/sys/kern
Le 08/03/2020 à 01:31, Andrew Doran a écrit : > Module Name: src > Committed By: ad > Date: Sun Mar 8 00:31:19 UTC 2020 > > Modified Files: > src/sys/kern: subr_kmem.c > > Log Message: > KMEM_SIZE: append the size_t to the allocated buffer, rather than > prepending, so it doesn't screw up the alignment of the buffer. > > Reported-by: syzbot+c024c50570cccac51...@syzkaller.appspotmail.com > > > To generate a diff of this commit: > cvs rdiff -u -r1.78 -r1.79 src/sys/kern/subr_kmem.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. This is wrong. The point of KMEM_SIZE is to store at a reliable location the size of the buffer, in order to then verify that the 'size' argument given to kmem_free() is correct. Here, you are using that size argument to compute the location, which breaks the whole point of the check. Also it probably collides with the KASAN shadow. Please revert this change. As said previously, if cacheline alignment is expected this way, then it should clearly be part of the kmem contract, and documented to be so. Thanks
Re: CVS commit: src
Le 08/03/2020 à 02:33, Andrew Doran a écrit :
> On Sat, Mar 07, 2020 at 12:24:21PM +0100, Maxime Villard wrote:
>
>> Can we revert the "__aligned(COHERENCY_UNIT)" for now? There is no particular
>> hurry to fix this bug, however the KUBSAN instance has been down for more
>> than
>> two months because of this, and it needs to be addressed.
>
> That should be quelled now.
The change is not correct, see my answer in response to the commit.
>> Similarly, the KASAN instance is currently crashing hard on:
>>
>> https://syzkaller.appspot.com/bug?id=1aa3f789d356bf04644bcef632bf8c2373398ba2
>> Dozens of thousands of times each day. This has been the case for two weeks,
>> and it too needs to be addressed.
>
> That's been there since I started looking last year.
Not sure what you mean? The history log indicates that it started on Jan 7th
2020.
> I guess it's a false positive because the sanitiser probably thinks objects
> are gone once pool_cache_put() is called, but the actual point of disposal
> is the pool_cache dtor.
There is no false positive because of that. If a dtor is there, the data is
left as valid:
3124 #ifdef KASAN
3125/* If there is a ctor/dtor, leave the data as valid. */
3126if (__predict_false(pc_has_ctor(pc) || pc_has_dtor(pc))) {
3127return;
3128}
3129 #endif
And in all cases, the instance is compiled with POOL_QUARANTINE, which
"cancels" caches, that is, pool_cache_put directly returns the object to
the pool layer. So the buffer remains KASAN-valid, goes through dtor,
then is made KASAN-invalid, and finally lands in the pool.
Looking at the report, it looks like the use-after-free is on this line
in mutex_oncpu():
421 l = (lwp_t *)MUTEX_OWNER(owner);
422 ci = l->l_cpu;
So mutex_oncpu() is called on a lock somehow (previously?) held by an LWP
that now has been freed. If you look at the different reports, this issue
is triggered in random places. Maybe one of your recent changes leaves the
mutex as owned somehow?
Thanks
