is it me or is gdb in illumos 22.11 hopelessly borked?

whenever I debug a file it thinks that one of the headers is the C source file

and it gets the path wrong as well

if I do "set substitute-path borked-bogus-path/header.h source.c" then it is usable

which illumos? oi or omnios or something else?:)

OpenIndiana 22.11 I think

run pkg update -nv and see if there is update for gdb.

going straight to pkg update

Do you think that line 107 here github.com/illumos/illumos-gate/blo…sr/src/lib/libc/port/gen/memalign.c is really intentional?

it keeps on multiplying by 2 until the alignment is greater than MINSIZE+WORDSIZE (6 words or 48 bytes)

So an alignment if 12 gets multiplied up to 96 but an alignment of 20 only gets multiplied up to 80

glibc does something like

while (0 != (alignment & (alignment - 1))) alignment++;

to bump up the alignment to the next power of 2

and gdb is now behaving itself

ou, so it was failing with memalign ?

no it just seems very strange

maybe I'm misreading the code

Trying to fix handling of the various aligned alloc functions in Valgrind

did you check libumem?

:-) no

can't do everything

the memalign() man page does state that the alignment must be a power of two, so alignments of 12 or 20 would be invalid

though the check it does for _misaligned() doesn't properly catch that

should probably be calling ISP2() instead

that sounds more consistent with other platforms (which aren't particularly consistent)

it'd be good to get a bug filed, about the memalign thing above.

if paulf is still here, or someone else is up to summarize it.

paulf: I have used gdb in openindiana at times, I don't know if it's hopelessly broken but it's shockingly old.

maybe that's something toasterson and the OI people could fix pretty easily

gdb was OK after pkg update

memalign might be nice but it's a deprecated interface

posix_memalign is better

C also has aligned alloc but the alignment depends on the implementation so unportable

richlowe: We are not thaaaat old on gsb. we are at 12.1 we could update to 13.1 though.

ah, it must have been bumped since I updated

Yeah there is a 2023 Copyright in there. Must have been shortly before 13 released in January

just to be sure, kmem_cache_alloc(cache, KM_PUSHPAGE) can return NULL?

Good question. I thought I knew the answer, but changed my opinion recently. Let me double check to be sure.

it definitely returns NULL if kmem_slab_alloc() fails.

so, yes it can.

now, can it return garbage....

tsoome: It is not supposed to return NULL, ever. NULL is supposed to only be allowed when the KM_NOSLEEP bit is set in flags.

tsoome: That breaks many assumptions in ZFS...

ZFS cannot handle kmem_cache_alloc() returning NULL unless it specifies KM_NOSLEEP.

I thought it was like that all over the illumos kernel.

ya, well, I have abd with abd_size = 0xe000, abd_chunk_size = 0x1000 (that should make 62 chunks)

> 0xfffffe2d0ce11970::print abd_t abd_u.abd_scatter.abd_chunks[3]

abd_u.abd_scatter.abd_chunks[3] = 0xd0f313429003041

not a pointer

> 0xfffffe2d0ce11970::print abd_t abd_u.abd_scatter.abd_chunks[4]

abd_u.abd_scatter.abd_chunks[4] = 0

so, the natural question is, was the chunk array corrupted, or allocations did fail

tsoome: Did you look at the subsequent array values?

5 and 6 seem valid, 7 is not a pointer again

10 is again not a pointer

tsoome: yes, it can return garbage and need cleanup it before using

ryao: ZFS is using another implementation on Linux and BSD - they have wrappers to own

tsoome: I have never seen your issue until you told me about it, but to be honest, I have been saying that quite often to people recently.

> abd_chunk_cache::print kmem_cache_t cache_alloc_fail

cache_alloc_fail = 0

but that does only mean, we did not reach the code to increment it...

igork: I am aware that we did not port the SLAB allocator from illumos, but the assumptions in the ZFS code are the same. We inherited these assumptions when it was ported from OpenSolaris in the first place. Even the kmflags bits are the same.

ryao: kmem* present only on illumos and was ported to others

I checked the headers for both OpenZFS and illumos-gate before saying that you are supposed to need to pass KM_NOSLEEP for it to return NULL. That was the understanding of it that was used to develop ZFS.

and was ported as saving on interfaces and logic

My understanding is that this idea predates both the Linux port and myself as a contributor.

ryao: you try to speak about kmem_alloc(), but not about kmem_cache_alloc() - they are different

ryao: i'm working with OpenZFS ports to dilos a long time :) kmem* and threads are different between platforms

igork: If my spot check is correct, you appear to be right. Using the same semantics for kmem_cache_alloc() would seem to be a mistake made when porting things.

tsoome: I guess the answer is to switch to kmem_alloc() there.

ryao: it is not mistake, probably some platform specific things can be different

ryao: no need to switch, we can use kmem_cache_* for abd on illumos and i did it on dilos

but it was a long race with updates from OpenZFS with many panics :)

as reference you can try take a look freebsd updates for abd

cache_dump.kd_alloc_fails = 0xa840a05b

so this one definitely has seen allocation failures

igork: The code expects kmem_alloc(..., KM_PUSHPAGE) semantics there. Also, if the illumos kmem_cache_alloc() is really passing non-pointers, then I am not sure how it could be used correctly...

kmem_alloc(..., KM_PUSHPAGE) have no KM_PUSHPAGE

igork: Then it becomes KM_SLEEP.

ryao: illumos version was ported to another platforms with others dependences to platform specific

it is why linux and freebsd have some different *_os.c files with different inmplementations

i know openzfs code :)

hm, cache_dump.kd_alloc_fails should be incremented when constructor fails, but this cache has cache_constructor = 0.... interesting.

well, I'm not sure if we can get garbage from kmem_cache_alloc(), so it still can be some sort of memory corruption.

but returning NULL there is possibility we have to pay attention...

eh, abd_alloc_struct() does abd_t *abd = kmem_alloc(size, KM_PUSHPAGE); so, while attempting to use reserved memory, it too can fail.

igork: In the OpenZFS tree, we have used kmem_cache_alloc() in this way in many places, including platform independent places like ddt_alloc(). :/

tsoome: That was my original understanding of KM_PUSHPAGE, but there is a comment in the code that suggests it is supposed to be otherwise: #define KM_PUSHPAGE 0x0004 /* can block for memory; may use reserve */

err, I am stupid. KM_PUSHPAGE does leave bit 0 unset, so KM_SLEEP is present there as "flag"

as KM_SLEEP is 0

it is good question, if it is tested properly:D

KM_NOSLEEP is 1. I thought the design intent was that it would SLEEP unless told otherwise.

s/SLEEP/be KM_SLEEP/

but kmem_alloc is also using kmem_cache_alloc(). fun.

tsoome: I don't think that line proves what you think it does

it calls into vmem

and passes the flags

I highly doubt KM_SLEEP is broken

and yea ryao, that is the default

no, I do not think the KM_SLEEP is broken.

but as much as I can read, there is possibility, we do get NULL from kmem_cache_alloc even with flags bit 0 unset.

which would mean KM_SLEEP is broken

well.... yes:D

au_get_buff(), kcf_submit_request() and others will be very unhappy if kmem_cache_alloc() returns NULL with KM_SLEEP.

They dereference the returned pointer without a NULL check.

See the second kmem_cache_alloc() if you look at au_get_buff().

tsoome: and if you look at kmem_slab_create() it calls into vmem_alloc(), which honors the VM_SLEEP flag.

src.illumos.org/source/xref/illumos…ts/common/os/kmem.c?r=bbf21555#2549 ?

tsoome: src.illumos.org/source/xref/illumos…/crypto/io/crypto.c?r=bbf21555#1987

It is not my imagination. Plenty of code in the kernel assumes that without KM_NOSLEEP, you cannot get a NULL return value. The fact that it can means there is either a few bugs, or *many* bugs, depending on how you count.

s/is either/are either/

tsoome: ah so that is a different line and I think a different semantic, and yes, I think that means if the constructor fails, then you get a NULL. It still honored the semantic of KM_SLEEP, but if the constructor didn't pass you can't return the object as it could be bad.

tsoome: what is the cache in question?

abd_chunk_cache. that one does not have constructor.

specifically, one abd record has corrupted chunk array.

rzezeski: I assume he is talking about abd_alloc_chunk().

I was scrolled up when I hit enter.

yes

corrupted in sense that some pointers have clearly non-pointer values, some have 0

yea, so in that case it can't even reach the second line you linked, and my read of kmem/vmem if KM/VM_SLEEP is honored just fine in kmem_cache_alloc()

The declaration for abd_alloc_chunk() is not valid C99 if I recall correctly. That is a K&R-ism.

yes, we still have many of those...

tsoome: that could be use after free

hm.

good pint.

point*

Newer compilers are beginning to complain about that sinc ethey are becoming strict. They want you to write (void).

yep. except, we do not yet support that new ones:D

rzezeski: Use after free was my thought until igork said that you can get non-pointers from kmem_cache_alloc(). Then I became very confused.

I am still confused by that.

well, need to get kmem to fill things with patterns and see if this panic will be repeated:D

tsoome: so you have a pointer to the cache object, maybe of a `<object addr>::kgrep |::whatis`

I think that's what I usually do when dealing with UAF (been a while)

that seems to take its time:)

hm, that output does not tell too much to me.

no obvious hits about being freed.

well when I run something like that I'm looking to see if the ojbect is referenced by multiple different threads

and part of debugging this requires knowing how these objects are used, because it might be valid for multiple threads to have a pointer to it

but typically I can find that some thread has a pointer when it shouldn't, I think I also usually use some of the kmem mdb commands related to this as well, but I can't remember off the top of my head

hrm. now this is weird. first pointer in chunklist array is perfectly valid. second one is freed from abd_chunk. the abd itself is note yet freed.

rzezeski: ::kgrep?

as far as finding things that might be pointers to a doodad

nah, I need to dig it with fresh head.

richlowe: yea that's what I showed above, but I think I typically also like to use that with things like the audit log or whatever, though IIRC that requires kmem flag adjustment

tsoome: You said that some of the values were NULL. I just spotted this in abd_alloc_chunk(): ASSERT3P(c, !=, NULL);

tsoome: Unless you built without assertions, the allocator could not have returned NULL there. Use after free seems more likely, although that is its own headache.

ASSERT's need debug kernel, yep.

tsoome: Did you reproduce this without a debug kernel or were you lucky and hit it without looking for it?

its the dump from first panic, got no second case (yet)

the abd in question is supposed to be in arc and to receive data after write completion.

arc_write_ready -> abd_copy_off -> abd_iterate_func2 -> abd_copy_off_cb -> bcopy_ck_size -> GPF :)

tsoome: Does that mean you were just lucky/unlucky and hit this when not trying to find it?

yep.