Re: Please review: lookup changes
> Date: Thu, 5 Mar 2020 22:48:25 + > From: Andrew Doran > > I'd like to merge the changes on the ad-namecache branch, and would > appreciate any code review. The motivation for these changes is, as > you might guess, performance. I put an overview below. I won't be > merging this for a couple of weeks in any case. Thanks for working on this. Lookups are a big point of contention, so it's worth putting effort into fixing that. At the same time, I want to make sure we don't regress into the vfs locking insanity we had ten years ago; dholland@ and especially hannken@ have put a lot of work into cleaning it up and making it understandable and maintainable. Please make sure to wait until hannken@ has had an opportunity weigh in on it. > If the file system exports IMNT_NCLOOKUP, this now tries to "fast > forward" through as many component names as possible by looking > directly in the namecache, without taking vnode locks nor vnode > refs, and stops when the leaf is reached or there is something that > can't be handled using the namecache. In the most optimistic case a > vnode reference is taken only at the root and the leaf. > IMNT_NCLOOKUP is implemented right now only for tmpfs, ffs. What file systems can't support IMNT_NCLOOKUP and why? > Layered file systems can't work that way (names are cached below), It seems there's a tradeoff here: 1. We could cache layered vnodes, at the cost of doubling the size of the namecache. 2. We could _not_ cache layered vnodes, at the cost of having to redo layer_node_create and vcache_get for every lookup -- a process which scales worse than namecache lookups (global vcache_lock). Am I missing anything else about this? Is this tradeoff the only reason that it's still up to the vnode operations to call cache_lookup and cache_enter, rather than something namei does directly -- so that layer_lookup can skip them? > and the permissions check won't work for anything unusual like ACLs, > so there is also IMNT_SHRLOOKUP. If the file system exports > IMNT_SHRLOOKUP, then VOP_LOOKUP() for "read only" lookups (e.g. not > RENAME & friends) is tried first with an LK_SHARED lock on the > directory. For an FS that can do the whole lookup this way (e.g. > tmpfs) all is good. Otherwise the FS is free to return ENOLCK from > VOP_LOOKUP() if something tricky comes up, and the lookup gets > retried with LK_EXCLUSIVE (e.g. ffs when it needs to scan directory > metadata). Except for access to the create/rename/remove hints (which we should do differently anyway [*]), in what circumstances does ufs lookup need an exclusive lock? Why does scanning directory metadata require that? [*] Specifically: We should push the last component lookup into directory operations, and leave it to *fs_rename/remove/ to invoke an internal lookup routine that does the right thing with locks and returns the hint to the caller rather than stashing it in the directory's struct inode. This has been intended for several years, but dholland and I have had a shortage of round tuits. > I also added a LOCKSHARED flag to go with LOCKLEAF (get an LK_SHARED > lock on the leaf instead of LK_EXCLUSIVE). This matches FreeBSD. I worry that after we spent several years getting rid of unnecessary complexity in the incomprehensible namei flags that rendered the subsystem unmaintainable, we may be sliding back toward that. Could we do this with a separate operation that happens after namei instead? NDINIT(, ...); error = namei(); if (error) goto out; namei_lockleaf(); (namei_lockleaf would obviously has to be careful not to lock when it's a `.' lookup and the parent is already locked, Perhaps this won't work -- I haven't thought it through -- but it would be nice to decrease rather than increase the complexity of namei itself.) On a related note: Why do VFS_VGET, VFS_ROOT, and VFS_FHTOVP need a lktype argument? Why can't they just return the vnode unlocked, and leave it to the caller to just call vn_lock? > vfs_syscalls.c: > > Corrected vnode lock types: LK_SHARED ok for VOP_ACCESS(), but > LK_EXCLUSIVE still needed for VOP_GETATTR() due to itimes handling. > That's a shame and it would be nice to use a seqlock or something > there eventually. I don't understand this itimes handling. Why do we ever _set_ any itimes in VOP_GETATTR? My understanding is that we try to defer issuing disk writes to update inode times as long as possible (except in wapbl where it can be done asynchronously and write-combined -- if nevertheless serially across the whole file system -- via the log). That explains why, e.g., ufs has patterns like ip->i_flag |= IN_CHANGE; UFS_WAPBL_UPDATE(vp, NULL, NULL, 0); to indicate that a change just happened and to defer a disk write either to the next wapbl commit
Re: NULL pointer arithmetic issues
On 08.03.2020 19:42, Mouse wrote: >>> The correct fix is not to disable the null-pointer-check option but >>> to remove the broken automatic non-null arguments in GCC. > >> The C standard and current usage (GNU) disagrees here. > > GNU is not some kind of arbiter of "current usage" (whatever _that_ > means). > >> memcpy (along with a number of other functions) must not accept NULL >> arguments and compiler can optimize the code based on these >> assumptions. > > Then such functions - or the language in which they are embedded - is > not suitable for writing kernels. > Theoretical C is not suitable for writing kernels and we need extensions for the freestanding environment. We require at least assembly stubs. > But, is it "must not accept null arguments" or is it "may do anything > they like when presented with null arguments"? Actually both. > But the answer is to fix the problem, not to > twist the code into a pretzel to work around the compiler's refusal to > be suitable for the job. > We use -fno-delete-null-pointer-checks to disable these optimizations. signature.asc Description: OpenPGP digital signature
Re: NULL pointer arithmetic issues
On 08.03.2020 19:30, Taylor R Campbell wrote:
>> Date: Sun, 8 Mar 2020 20:52:29 +0300
>> From: Roman Lebedev
>>
>> so we are allowed to lower that in clang front-end as:
>>
>> int
>> statu(char *a)
>> {
>> __builtin_assume(a != NULL);
>> a += getuid() - geteuid();
>> __builtin_assume(a != NULL);
>
> Allowed, yes.
>
> What I'm wondering is whether this is something Clang will actually do
> -- and whether it is for any serious reason other than to capriciously
> screw people who write software for real machines instead of for the
> pure C abstract machine to the letter of the standard (and if so,
> whether -fno-delete-null-pointer-checks is enough to disable it).
>
> Evidently making that assumption _not_ allowed in C++, so presumably
> it's not important for performance purposes. It's also not important
> for expressive purposes, because I could just as well have written
> assert(a != NULL).
>
>>> I was told by Roman that it was checked during a C committee meeting and
>>> confirmed to be an intentional UB.
>> Correction: Aaron Ballman asked about this in non-public WG14 reflector
>> mailing list, it wasn't a committee meeting, but the point still stands.
>
> What does `intentional' UB mean, exactly? What is the intent behind
> having p + 0 for null p be undefined in C, while the C++ committee saw
> fit to define it?
>
> Is it because there technically once existed C implementations on
> bizarre architectures with wacky arithmetic on pointers like Zeta-C,
> or is it because there are actual useful consequences to inferring
> that p must be nonnull if we evaluate p + 0?
>
> I ask because in principle a conformant implementation could compile
> the NetBSD kernel into a useless blob that does nothing -- we rely on
> all sorts of behaviour relative to a real physical machine that is not
> defined by the letter of the standard, like inline asm, or converting
> integers from the VM system's virtual address allocator into pointers
> to objects. But such an implementation would not be useful.
>
Aaron (as he was mentioned by name), is a voting member in the C++
committee and actively working on harmonizing C and C++ standards. There
is a good chance that C and C++ will be synced here (they definitely
should).
signature.asc
Description: OpenPGP digital signature
PG_BUSY and read locks in the VM system
The recent change to make vmobjlock & friends into RW locks is still a bit useless because while PG_BUSY can be observed or held off while holding a RW_READER lock, nothing can be done if it's set. To fix that I'd like to change it so that the WANTED flag is stored in pg->pqflags and covered by pg->interlock. >From there it's not huge strech to imagine how you might do basic page fault handing for already in-core pages while holding only a read lock. http://www.netbsd.org/~ad/2020/wanted.diff Comments? Thanks, Andrew
Object dirtyness
With the merge of yamt-pagecache we have solid tracking of page dirtyness. Dirtyness tracking of the containing uvm_object/vnode is not so solid by comparison. For example you can call uvm_pagemarkdirty() on a vnode page, but letting ioflush know about that is a separate step. When the page & uvm_object state aren't consistent we get panics. I'd like to change it so that: - the vnode/uvm_object gets put on the syncer worklist as soon as it gains a first dirty page, by having uvm_pagemarkdirty() do it. - VI_WRMAPDIRTY is eliminated. I think it was really only to stop dirty metadata buffers muddying the waters by implying dirty uvm_object, but the radix tree can be checked cheaply & directly now. http://www.netbsd.org/~ad/2020/dirty.diff Comments? Thanks, Andrew
Re: Please review: lookup changes
On Thu, Mar 05, 2020 at 10:48:25PM +, Andrew Doran wrote: > http://www.netbsd.org/~ad/2020/namecache.diff > http://www.netbsd.org/~ad/2020/vfs_lookup.c > http://www.netbsd.org/~ad/2020/vfs_cache.c This was missing one small change: http://www.netbsd.org/~ad/2020/namecache2.diff To compile it also needs this first: cd src/sys/sys USETOOLS=no make namei Andrew
Re: Please review: lookup changes
On Sat, Mar 07, 2020 at 12:14:05AM +0100, Mateusz Guzik wrote: > On 3/5/20, Andrew Doran wrote: > > vfs_cache.c: > > > > I changed the namecache to operate using a per-directory index of > > names rather than a global index, and changed the reclamation > > mechanism for cache entries to ape the VM system's CLOCK algorithm > > rather than using LRU / pseudo-LRU. This made concurrency easier to > > manage, and the nasty locking scheme from 5.0 is gone. > > > > I don't think rb trees (or in fact anything non-hash) is a good idea for > this purpose. See way below. Right, I've admitted to the fact that rbtree is not ideal here. But it is "good enough", and I'm happy for someone to replace it with some other data structure later on. > I doubt aging namecache entries is worth the complexity. Note that said > very likely this code does not really win anything in practice. If there was a 1:1 relationship I would agree but due to links (including dot and dotdot) it's N:M and I don't fancy changing that behaviour too much right now. Definitely something to consider in the future. > > I also changed the namecache to cache permission information for > > directories, so that it can be used to do single component lookups > > fully in-cache for file systems that can work that way. > > > > Low priority, but I think this comes with a bug. kauth() has this support > for "listeners" and so happens in the base system at least there is > nobody hooked for vnode lookup. Key point being that normally the vnode > is passed in shared-locked, but fast path lookup avoids this and > consequently if there is a real-world uesr of the feature somewhere, they > may be silently broken. Good point. Yes that aspect of kauth is a bit of a mess. Plus it adds a lot of CPU overhead. It could do with its own solution for concurrency that's straightforward, easy to maintain and doesn't have too many tentacles that depend on concurrency rules in the various subsystems. > I had a similar problem with the MAC framework and devised a simple flag > which can be checked upfront to disable fast path lookup. > > So this would have a side effect of removing the call in the first place. > genfs_can_access is rather pessimal for checking VEXEC. I wrote a dedicated > routine which only checks for VEXEC and starts with a check for > (mode & 0111) == 0111 upfront. It happens to almost always be true. > > > This is described in the comments in the file. > > > > vfs_getcwd.c: > > > > The design pattern here was to hold vnode locks as long as possible > > and clearly deliniate the handoff from one lock to another. It's a > > nice way of working but not good for MP performance, and not needed > > for correctness. I pushed the locks back as far as possible and try > > to do the lookup in the namecache without taking vnode locks (not > > needed to look at namecache). > > > > vfs_lookup.c: > > > > Like vfs_getcwd.c the vnode locking is pushed back far as possible. > > > > If the file system exports IMNT_NCLOOKUP, this now tries to "fast > > forward" through as many component names as possible by looking > > directly in the namecache, without taking vnode locks nor vnode > > refs, and stops when the leaf is reached or there is something that > > can't be handled using the namecache. In the most optimistic case a > > vnode reference is taken only at the root and the leaf. > > IMNT_NCLOOKUP is implemented right now only for tmpfs, ffs. > > > > Layered file systems can't work that way (names are cached below), > > and the permissions check won't work for anything unusual like ACLs, > > so there is also IMNT_SHRLOOKUP. If the file system exports > > IMNT_SHRLOOKUP, then VOP_LOOKUP() for "read only" lookups (e.g. not > > RENAME & friends) is tried first with an LK_SHARED lock on the > > directory. For an FS that can do the whole lookup this way (e.g. > > tmpfs) all is good. Otherwise the FS is free to return ENOLCK from > > VOP_LOOKUP() if something tricky comes up, and the lookup gets > > retried with LK_EXCLUSIVE (e.g. ffs when it needs to scan directory > > metadata). > > > > I also added a LOCKSHARED flag to go with LOCKLEAF (get an LK_SHARED > > lock on the leaf instead of LK_EXCLUSIVE). This matches FreeBSD. > > > > vfs_syscalls.c: > > > > Corrected vnode lock types: LK_SHARED ok for VOP_ACCESS(), but > > LK_EXCLUSIVE still needed for VOP_GETATTR() due to itimes handling. > > That's a shame and it would be nice to use a seqlock or something > > there eventually. > > > > I'm considering doing something like this myself. Lookup doing this could > avoid even referencing the vnode in the first place in the common case. > > The itimes situation is very broken in my opinion. Filesystems mark vnode > as
Re: NULL pointer arithmetic issues
>> The correct fix is not to disable the null-pointer-check option but >> to remove the broken automatic non-null arguments in GCC. > The C standard and current usage (GNU) disagrees here. GNU is not some kind of arbiter of "current usage" (whatever _that_ means). > memcpy (along with a number of other functions) must not accept NULL > arguments and compiler can optimize the code based on these > assumptions. Then such functions - or the language in which they are embedded - is not suitable for writing kernels. But, is it "must not accept null arguments" or is it "may do anything they like when presented with null arguments"? Given the lack of any way for it to "not accept" arguments, I suspect it is the latter, in which case it is not the language but rather the compiler, the compiler that mis-"optimizes" those cases into broken code, that is not suitable for kernel use. The "automatic non-null arguments" are not apporpriate in a language used for writing kernels. Whether that means changing the basic language, changing compilers, changing compiler flags, changing a file somewhere, I don't know. But the answer is to fix the problem, not to twist the code into a pretzel to work around the compiler's refusal to be suitable for the job. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: NULL pointer arithmetic issues
> Date: Sun, 8 Mar 2020 20:52:29 +0300
> From: Roman Lebedev
>
> so we are allowed to lower that in clang front-end as:
>
> int
> statu(char *a)
> {
> __builtin_assume(a != NULL);
> a += getuid() - geteuid();
> __builtin_assume(a != NULL);
Allowed, yes.
What I'm wondering is whether this is something Clang will actually do
-- and whether it is for any serious reason other than to capriciously
screw people who write software for real machines instead of for the
pure C abstract machine to the letter of the standard (and if so,
whether -fno-delete-null-pointer-checks is enough to disable it).
Evidently making that assumption _not_ allowed in C++, so presumably
it's not important for performance purposes. It's also not important
for expressive purposes, because I could just as well have written
assert(a != NULL).
> > I was told by Roman that it was checked during a C committee meeting and
> > confirmed to be an intentional UB.
> Correction: Aaron Ballman asked about this in non-public WG14 reflector
> mailing list, it wasn't a committee meeting, but the point still stands.
What does `intentional' UB mean, exactly? What is the intent behind
having p + 0 for null p be undefined in C, while the C++ committee saw
fit to define it?
Is it because there technically once existed C implementations on
bizarre architectures with wacky arithmetic on pointers like Zeta-C,
or is it because there are actual useful consequences to inferring
that p must be nonnull if we evaluate p + 0?
I ask because in principle a conformant implementation could compile
the NetBSD kernel into a useless blob that does nothing -- we rely on
all sorts of behaviour relative to a real physical machine that is not
defined by the letter of the standard, like inline asm, or converting
integers from the VM system's virtual address allocator into pointers
to objects. But such an implementation would not be useful.
Re: NULL pointer arithmetic issues
Warning: i'm not subscribed to the lists CC'd here.
On Sun, Mar 8, 2020 at 8:26 PM Kamil Rytarowski wrote:
>
> On 08.03.2020 18:00, Taylor R Campbell wrote:
> > Thanks for doing the research.
> >
> >> Date: Sun, 8 Mar 2020 15:30:02 +0100
> >> From: Kamil Rytarowski
> >>
> >> NULL+0 was added to UBSan proactively as it is as of today not
> >> miscompiled, but it is planned to so in not so distant time. It is a
> >> chicken-egg problem.
> >
> > If you say it is planned, can you please cite the plan?
> >
>
> Adding Roman Lebedev, we discussed about enabling NULL+0 optimization.
> Teaching LLVM about this will allow more aggressive optimization, so
> code like this:
>
> http://netbsd.org/~kamil/null_plus_0-ub.c
>
> will report different results depending on optimization levels.
I believe this point is correct. Given
int
statu(char *a)
{
a += getuid() - geteuid();
return a == NULL;
}
in C, as per all the wording cited below \/, we are allowed to assume
that given `ptr + offset`, regardless of `offset`,
both `ptr` and `ptr + offset` are non-NULL,
so we are allowed to lower that in clang front-end as:
int
statu(char *a)
{
__builtin_assume(a != NULL);
a += getuid() - geteuid();
__builtin_assume(a != NULL);
return a == NULL;
}
And that will naturally fold to
int
statu(char *a)
{
(void)getuid();
(void)geteuid();
return false;
}
.. which isn't what the code initially intended to do.
I.e. it's a dormant miscompile.
> > In C++, adding zero to a null pointer is explicitly allowed and
> > guaranteed to return a null pointer. See, for example, C++11 5.7
> > `Additive operators', clause 7, p. 117: `If the value 0 is added to or
> > subtracted from a pointer value, the result compares equal to the
> > original pointer value.' C++17 clarifies in 8.7 `Additive operators',
> > clause 7, p. 132: `If the value 0 is added to or subtracted from a
> > null pointer value, the result is a null pointer.'
> >
> > So it would be a little surprising to me if compilers -- which tend to
> > focus their efforts on C++ more than C these days -- went out of their
> > way to act on the inference that evaluating p + 0 implies p is nonnull
> > in C.
> >
>
> I underlined the C language in my message. More elaborated answer here:
>
> https://reviews.llvm.org/D67122#inline-612172
>
> I was told by Roman that it was checked during a C committee meeting and
> confirmed to be an intentional UB.
Correction: Aaron Ballman asked about this in non-public WG14 reflector
mailing list, it wasn't a committee meeting, but the point still stands.
> >> There is however a fallback. If we want to use NULL+0, we must use
> >> -fno-delete-null-pointer-checks that avoids miscompilation and raising
> >> UBSan reports. If we want to allow NULL+x we must enable -fwrapv.
> >
> > Adding -fno-delete-null-pointer-checks for clang too sounds sensible
> > to me in general, but please check with joerg first. It remains
> > unclear to me that it's necessary here.
> >
>
> Today it will calm down LLVM UBSan. In future potentially avoid
> miscompilation.
>
> There are also memcpy(NULL,NULL,0)-like cases that need research why
> they happen in the first place.
Roman
Re: NULL pointer arithmetic issues
On 08.03.2020 18:12, Joerg Sonnenberger wrote: > On Sun, Mar 08, 2020 at 03:33:57PM +0100, Kamil Rytarowski wrote: >> There was also a request to make a proof that memcpy(NULL,NULL,0) is UB >> and can be miscompiled. >> >> Here is a reproducer: >> >> http://netbsd.org/~kamil/memcpy-ub.c >> >> 131 kamil@rugged /tmp $ gcc -O0 memcpy.c >> >> 132 kamil@rugged /tmp $ ./a.out >> >> 1 >> >> 133 kamil@rugged /tmp $ gcc -O2 memcpy.c >> 134 kamil@rugged /tmp $ ./a.out >> 0 >> >> A fallback for freestanding environment is to use >> -fno-delete-null-pointer-check. > > The correct fix is not to disable the null-pointer-check option but to > remove the broken automatic non-null arguments in GCC. > > Joerg > The C standard and current usage (GNU) disagrees here. memcpy (along with a number of other functions) must not accept NULL arguments and compiler can optimize the code based on these assumptions. -fno-delete-null-pointer-check is a fallback disabling this optimizations. signature.asc Description: OpenPGP digital signature
Re: NULL pointer arithmetic issues
On 08.03.2020 18:11, Joerg Sonnenberger wrote: > On Sun, Mar 08, 2020 at 03:30:02PM +0100, Kamil Rytarowski wrote: >> NULL+x is now miscompiled by Clang/LLVM after this commit: >> >> https://reviews.llvm.org/rL369789 >> >> This broke various programs like: >> >> "Performing base + offset pointer arithmetic is only allowed when base >> itself is not nullptr. In other words, the compiler is assumed to allow >> that base + offset is always non-null, which an upcoming compiler >> release will do in this case. The result is that CommandStream.cpp, >> which calls this in a loop until the result is nullptr, will never >> terminate (until it runs junk data and crashes)." > > As you said, using a non-zero offset. Noone here argued that using > non-zero offsets is or should be valid since that would obviously create > a pointer outside the zero-sized object. > > Joerg > We catch NULL + x at least here: Undefined Behavior in t_subr_prf.c:179:9, pointer expression with base 0 overflowed to 0x14 Undefined Behavior in t_subr_prf.c:179:9, pointer expression with base 0 overflowed to 0xa signature.asc Description: OpenPGP digital signature
Re: NULL pointer arithmetic issues
On 08.03.2020 18:00, Taylor R Campbell wrote: > Thanks for doing the research. > >> Date: Sun, 8 Mar 2020 15:30:02 +0100 >> From: Kamil Rytarowski >> >> NULL+0 was added to UBSan proactively as it is as of today not >> miscompiled, but it is planned to so in not so distant time. It is a >> chicken-egg problem. > > If you say it is planned, can you please cite the plan? > Adding Roman Levedev, we discussed about enabling NULL+0 optimization. Teaching LLVM about this will allow more aggressive optimization, so code like this: http://netbsd.org/~kamil/null_plus_0-ub.c will report different results depending on optimization levels. > In C++, adding zero to a null pointer is explicitly allowed and > guaranteed to return a null pointer. See, for example, C++11 5.7 > `Additive operators', clause 7, p. 117: `If the value 0 is added to or > subtracted from a pointer value, the result compares equal to the > original pointer value.' C++17 clarifies in 8.7 `Additive operators', > clause 7, p. 132: `If the value 0 is added to or subtracted from a > null pointer value, the result is a null pointer.' > > So it would be a little surprising to me if compilers -- which tend to > focus their efforts on C++ more than C these days -- went out of their > way to act on the inference that evaluating p + 0 implies p is nonnull > in C. > I underlined the C language in my message. More elaborated answer here: https://reviews.llvm.org/D67122#inline-612172 I was told by Roman that it was checked during a C committee meeting and confirmed to be an intentional UB. >> There is however a fallback. If we want to use NULL+0, we must use >> -fno-delete-null-pointer-checks that avoids miscompilation and raising >> UBSan reports. If we want to allow NULL+x we must enable -fwrapv. > > Adding -fno-delete-null-pointer-checks for clang too sounds sensible > to me in general, but please check with joerg first. It remains > unclear to me that it's necessary here. > Today it will calm down LLVM UBSan. In future potentially avoid miscompilation. There are also memcpy(NULL,NULL,0)-like cases that need research why they happen in the first place. signature.asc Description: OpenPGP digital signature
Re: NULL pointer arithmetic issues
On Sun, Mar 08, 2020 at 03:33:57PM +0100, Kamil Rytarowski wrote: > There was also a request to make a proof that memcpy(NULL,NULL,0) is UB > and can be miscompiled. > > Here is a reproducer: > > http://netbsd.org/~kamil/memcpy-ub.c > > 131 kamil@rugged /tmp $ gcc -O0 memcpy.c > > 132 kamil@rugged /tmp $ ./a.out > > 1 > > 133 kamil@rugged /tmp $ gcc -O2 memcpy.c > 134 kamil@rugged /tmp $ ./a.out > 0 > > A fallback for freestanding environment is to use > -fno-delete-null-pointer-check. The correct fix is not to disable the null-pointer-check option but to remove the broken automatic non-null arguments in GCC. Joerg
Re: NULL pointer arithmetic issues
On Sun, Mar 08, 2020 at 03:30:02PM +0100, Kamil Rytarowski wrote: > NULL+x is now miscompiled by Clang/LLVM after this commit: > > https://reviews.llvm.org/rL369789 > > This broke various programs like: > > "Performing base + offset pointer arithmetic is only allowed when base > itself is not nullptr. In other words, the compiler is assumed to allow > that base + offset is always non-null, which an upcoming compiler > release will do in this case. The result is that CommandStream.cpp, > which calls this in a loop until the result is nullptr, will never > terminate (until it runs junk data and crashes)." As you said, using a non-zero offset. Noone here argued that using non-zero offsets is or should be valid since that would obviously create a pointer outside the zero-sized object. Joerg
Re: NULL pointer arithmetic issues
Thanks for doing the research. > Date: Sun, 8 Mar 2020 15:30:02 +0100 > From: Kamil Rytarowski > > NULL+0 was added to UBSan proactively as it is as of today not > miscompiled, but it is planned to so in not so distant time. It is a > chicken-egg problem. If you say it is planned, can you please cite the plan? In C++, adding zero to a null pointer is explicitly allowed and guaranteed to return a null pointer. See, for example, C++11 5.7 `Additive operators', clause 7, p. 117: `If the value 0 is added to or subtracted from a pointer value, the result compares equal to the original pointer value.' C++17 clarifies in 8.7 `Additive operators', clause 7, p. 132: `If the value 0 is added to or subtracted from a null pointer value, the result is a null pointer.' So it would be a little surprising to me if compilers -- which tend to focus their efforts on C++ more than C these days -- went out of their way to act on the inference that evaluating p + 0 implies p is nonnull in C. > There is however a fallback. If we want to use NULL+0, we must use > -fno-delete-null-pointer-checks that avoids miscompilation and raising > UBSan reports. If we want to allow NULL+x we must enable -fwrapv. Adding -fno-delete-null-pointer-checks for clang too sounds sensible to me in general, but please check with joerg first. It remains unclear to me that it's necessary here.
Re: NULL pointer arithmetic issues
On 22.02.2020 17:25, Kamil Rytarowski wrote:
> When running the ATF tests under MKLIBCSANITIZER [1], there are many
> NULL pointer arithmetic issues .
>
> http://netbsd.org/~kamil/mksanitizer-reports/ubsan-2020-02-22-null-pointer.txt
>
> These issues are in macros like:
> - IN_ADDRHASH_READER_FOREACH()
> - IN_ADDRLIST_WRITER_INSERT_TAIL()
> - IFADDR_READER_FOREACH()
> - etc
>
> These macros wrap internally pserialize-safe linked lists.
>
> What's the proper approach to address this issue?
>
> These reports are responsible for around half of all kinds of the
> remaining Undefined Behavior unique issues when executing ATF tests.
>
>
> [1] ./build.sh -N0 -U -V MAKECONF=/dev/null -V HAVE_LLVM=yes -V MKGCC=no
> -V MKLLVM=yes -V MKLIBCSANITIZER=yes -j8 -u -O /public/netbsd-llvm
> distribution
>
>
>
NULL + 0 and NULL + x are both Undefined Behavior (as confirmed by a C
committee member, it was discussed and confirmed to be intentional).
NULL+x is now miscompiled by Clang/LLVM after this commit:
https://reviews.llvm.org/rL369789
This broke various programs like:
"Performing base + offset pointer arithmetic is only allowed when base
itself is not nullptr. In other words, the compiler is assumed to allow
that base + offset is always non-null, which an upcoming compiler
release will do in this case. The result is that CommandStream.cpp,
which calls this in a loop until the result is nullptr, will never
terminate (until it runs junk data and crashes)."
https://github.com/google/filament/pull/1566
LLVM middle-end uses those guarantees for transformations.
This pushed the LLVM devs to implement this NULL+x and NULL+0 check in
UBSan:
https://reviews.llvm.org/D67122
NULL+0 was added to UBSan proactively as it is as of today not
miscompiled, but it is planned to so in not so distant time. It is a
chicken-egg problem.
There is however a fallback. If we want to use NULL+0, we must use
-fno-delete-null-pointer-checks that avoids miscompilation and raising
UBSan reports. If we want to allow NULL+x we must enable -fwrapv.
This means that we can avoid pserialize reports by enabling these CFLAGS
for clang as well.
22 # We are compiling the kernel code with
no-delete-null-pointer-checks,
23 # and compiling without it, causes issues at least on sh3 by adding
24 # aborts after kern_assert on NULL pointer checks.
25 CFLAGS+=${${ACTIVE_CC} == "gcc":?
-fno-delete-null-pointer-checks :}
26
https://nxr.netbsd.org/xref/src/sys/rump/Makefile.rump#25
I'm going to test it and switch -fno-delete-null-pointer-check on
Clang/LLVM.
For the historical note, a reproducer miscompiling NULL + x:
http://netbsd.org/~kamil/null_plus_x-ub.c
136 kamil@chieftec /tmp $ /usr/local/bin/clang -O0 null_plus_x-ub.c
137 kamil@chieftec /tmp $ ./a.out
1
138 kamil@chieftec /tmp $ /usr/local/bin/clang -O2 null_plus_x-ub.c
139 kamil@chieftec /tmp $ ./a.out
0
151 kamil@chieftec /tmp $ /usr/local/bin/clang -O3 -fwrapv null_plus_x-ub.c
152 kamil@chieftec /tmp $ ./a.out
1
NULL+0 is planned to follow up.
signature.asc
Description: OpenPGP digital signature
Re: Current status of "support jails-like features"?
On 08.03.2020 03:55, メーリングリストNetBSD wrote: > Hello, > > I'm interested in "support jails-like features" [1] among The NetBSD > projects. > I investigated several current container systems [2], but couldn't find > one providing kernel-level virtualization. > I would like to know the current state of this project. Are there any > NetBSD developers working on this project? > There is initial work in this domain. Replied in a private mail. > [1] http://wiki.netbsd.org/projects/project/kernel_components/ > [2] https://github.com/NetBSDfr/sailor > https://github.com/jmmv/sandboxctl > > --- > wataru kasai signature.asc Description: OpenPGP digital signature
