10:14:52 is it me or is gdb in illumos 22.11 hopelessly borked? 10:15:38 whenever I debug a file it thinks that one of the headers is the C source file 10:15:51 and it gets the path wrong as well 10:17:41 if I do "set substitute-path borked-bogus-path/header.h source.c" then it is usable 11:23:19 which illumos? oi or omnios or something else?:) 11:44:33 OpenIndiana 22.11 I think 11:48:02 run pkg update -nv and see if there is update for gdb. 11:49:48 going straight to pkg update 13:19:14 Do you think that line 107 here https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libc/port/gen/memalign.c is really intentional? 13:21:00 it keeps on multiplying by 2 until the alignment is greater than MINSIZE+WORDSIZE (6 words or 48 bytes) 13:21:33 So an alignment if 12 gets multiplied up to 96 but an alignment of 20 only gets multiplied up to 80 13:23:16 glibc does something like 13:23:17 while (0 != (alignment & (alignment - 1))) alignment++; 13:23:36 to bump up the alignment to the next power of 2 13:24:53 and gdb is now behaving itself 13:25:16 ou, so it was failing with memalign ? 13:25:33 no it just seems very strange 13:25:41 maybe I'm misreading the code 13:26:06 https://bugs.kde.org/show_bug.cgi?id=466104 13:26:36 Trying to fix handling of the various aligned alloc functions in Valgrind 13:26:53 did you check libumem? 13:27:20 :-) no 13:27:34 can't do everything 15:27:55 the memalign() man page does state that the alignment must be a power of two, so alignments of 12 or 20 would be invalid 15:29:22 though the check it does for _misaligned() doesn't properly catch that 16:01:14 should probably be calling ISP2() instead 16:14:24 that sounds more consistent with other platforms (which aren't particularly consistent) 20:52:12 it'd be good to get a bug filed, about the memalign thing above. 20:52:27 if paulf is still here, or someone else is up to summarize it. 20:53:00 paulf: I have used gdb in openindiana at times, I don't know if it's hopelessly broken but it's shockingly old. 20:53:45 maybe that's something toasterson and the OI people could fix pretty easily 21:04:14 gdb was OK after pkg update 21:04:37 memalign might be nice but it's a deprecated interface 21:04:45 posix_memalign is better 21:05:16 C also has aligned alloc but the alignment depends on the implementation so unportable 21:15:10 richlowe: We are not thaaaat old on gsb. we are at 12.1 we could update to 13.1 though. 21:18:06 ah, it must have been bumped since I updated 21:19:19 Yeah there is a 2023 Copyright in there. Must have been shortly before 13 released in January 21:42:46 just to be sure, kmem_cache_alloc(cache, KM_PUSHPAGE) can return NULL? 21:45:15 Good question. I thought I knew the answer, but changed my opinion recently. Let me double check to be sure. 21:46:04 it definitely returns NULL if kmem_slab_alloc() fails. 21:46:23 so, yes it can. 21:46:35 now, can it return garbage.... 21:46:44 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. 21:47:16 https://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/os/kmem.c?r=bbf21555#2522 21:48:40 tsoome: That breaks many assumptions in ZFS... 21:49:26 ZFS cannot handle kmem_cache_alloc() returning NULL unless it specifies KM_NOSLEEP. 21:49:34 I thought it was like that all over the illumos kernel. 21:50:45 ya, well, I have abd with abd_size = 0xe000, abd_chunk_size = 0x1000 (that should make 62 chunks) 21:51:01 > 0xfffffe2d0ce11970::print abd_t abd_u.abd_scatter.abd_chunks[3] 21:51:07 abd_u.abd_scatter.abd_chunks[3] = 0xd0f313429003041 21:51:12 not a pointer 21:51:24 > 0xfffffe2d0ce11970::print abd_t abd_u.abd_scatter.abd_chunks[4] 21:51:30 abd_u.abd_scatter.abd_chunks[4] = 0 21:52:21 so, the natural question is, was the chunk array corrupted, or allocations did fail 21:55:34 tsoome: Did you look at the subsequent array values? 21:56:23 5 and 6 seem valid, 7 is not a pointer again 21:56:51 10 is again not a pointer 21:56:57 tsoome: yes, it can return garbage and need cleanup it before using 21:58:06 ryao: ZFS is using another implementation on Linux and BSD - they have wrappers to own 21:58:09 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. 21:58:38 > abd_chunk_cache::print kmem_cache_t cache_alloc_fail 21:58:38 cache_alloc_fail = 0 21:58:58 but that does only mean, we did not reach the code to increment it... 21:59:05 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. 21:59:48 ryao: kmem* present only on illumos and was ported to others 22:00:20 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. 22:00:34 and was ported as saving on interfaces and logic 22:01:12 My understanding is that this idea predates both the Linux port and myself as a contributor. 22:01:29 ryao: you try to speak about kmem_alloc(), but not about kmem_cache_alloc() - they are different 22:03:13 ryao: i'm working with OpenZFS ports to dilos a long time :) kmem* and threads are different between platforms 22:03:16 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. 22:04:28 tsoome: I guess the answer is to switch to kmem_alloc() there. 22:04:58 ryao: it is not mistake, probably some platform specific things can be different 22:05:54 ryao: no need to switch, we can use kmem_cache_* for abd on illumos and i did it on dilos 22:06:20 but it was a long race with updates from OpenZFS with many panics :) 22:07:08 as reference you can try take a look freebsd updates for abd 22:07:09 cache_dump.kd_alloc_fails = 0xa840a05b 22:07:30 so this one definitely has seen allocation failures 22:07:43 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... 22:07:59 kmem_alloc(..., KM_PUSHPAGE) have no KM_PUSHPAGE 22:08:17 igork: Then it becomes KM_SLEEP. 22:09:14 ryao: illumos version was ported to another platforms with others dependences to platform specific 22:10:22 it is why linux and freebsd have some different *_os.c files with different inmplementations 22:10:46 i know openzfs code :) 22:10:55 hm, cache_dump.kd_alloc_fails should be incremented when constructor fails, but this cache has cache_constructor = 0.... interesting. 22:15:19 well, I'm not sure if we can get garbage from kmem_cache_alloc(), so it still can be some sort of memory corruption. 22:16:47 but returning NULL there is possibility we have to pay attention... 22:25:17 eh, abd_alloc_struct() does abd_t *abd = kmem_alloc(size, KM_PUSHPAGE); so, while attempting to use reserved memory, it too can fail. 22:25:50 igork: In the OpenZFS tree, we have used kmem_cache_alloc() in this way in many places, including platform independent places like ddt_alloc(). :/ 22:26:46 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 */ 22:27:24 err, I am stupid. KM_PUSHPAGE does leave bit 0 unset, so KM_SLEEP is present there as "flag" 22:27:52 as KM_SLEEP is 0 22:28:42 it is good question, if it is tested properly:D 22:29:08 KM_NOSLEEP is 1. I thought the design intent was that it would SLEEP unless told otherwise. 22:29:27 s/SLEEP/be KM_SLEEP/ 22:34:24 but kmem_alloc is also using kmem_cache_alloc(). fun. 22:38:25 tsoome: I don't think that line proves what you think it does 22:38:32 it calls into vmem 22:38:36 and passes the flags 22:38:55 I highly doubt KM_SLEEP is broken 22:39:06 and yea ryao, that is the default 22:39:39 no, I do not think the KM_SLEEP is broken. 22:40:40 but as much as I can read, there is possibility, we do get NULL from kmem_cache_alloc even with flags bit 0 unset. 22:41:59 which would mean KM_SLEEP is broken 22:42:29 well.... yes:D 22:43:13 au_get_buff(), kcf_submit_request() and others will be very unhappy if kmem_cache_alloc() returns NULL with KM_SLEEP. 22:43:39 They dereference the returned pointer without a NULL check. 22:43:53 See the second kmem_cache_alloc() if you look at au_get_buff(). 22:45:41 tsoome: and if you look at kmem_slab_create() it calls into vmem_alloc(), which honors the VM_SLEEP flag. 22:47:39 https://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/os/kmem.c?r=bbf21555#2549 ? 22:48:31 tsoome: https://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/crypto/io/crypto.c?r=bbf21555#1987 22:49:29 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. 22:49:58 s/is either/are either/ 22:53:32 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. 22:55:55 tsoome: what is the cache in question? 22:56:54 abd_chunk_cache. that one does not have constructor. 22:57:32 specifically, one abd record has corrupted chunk array. 22:57:42 rzezeski: I assume he is talking about abd_alloc_chunk(). 22:57:55 I was scrolled up when I hit enter. 22:57:55 yes 22:58:47 corrupted in sense that some pointers have clearly non-pointer values, some have 0 22:59:01 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() 22:59:12 The declaration for abd_alloc_chunk() is not valid C99 if I recall correctly. That is a K&R-ism. 22:59:31 yes, we still have many of those... 22:59:32 tsoome: that could be use after free 22:59:47 hm. 22:59:51 good pint. 22:59:55 point* 22:59:59 Newer compilers are beginning to complain about that sinc ethey are becoming strict. They want you to write (void). 23:00:32 yep. except, we do not yet support that new ones:D 23:01:00 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. 23:01:08 I am still confused by that. 23:02:23 well, need to get kmem to fill things with patterns and see if this panic will be repeated:D 23:03:31 tsoome: so you have a pointer to the cache object, maybe of a `::kgrep |::whatis` 23:03:43 I think that's what I usually do when dealing with UAF (been a while) 23:06:53 that seems to take its time:) 23:13:12 hm, that output does not tell too much to me. 23:13:33 no obvious hits about being freed. 23:14:32 well when I run something like that I'm looking to see if the ojbect is referenced by multiple different threads 23:15:05 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 23:15:47 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 23:20:58 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. 23:28:14 rzezeski: ::kgrep? 23:28:34 as far as finding things that might be pointers to a doodad 23:30:02 nah, I need to dig it with fresh head. 23:34:28 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 23:41:42 tsoome: You said that some of the values were NULL. I just spotted this in abd_alloc_chunk(): ASSERT3P(c, !=, NULL); 23:42:12 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. 23:42:39 ASSERT's need debug kernel, yep. 23:43:12 tsoome: Did you reproduce this without a debug kernel or were you lucky and hit it without looking for it? 23:44:00 its the dump from first panic, got no second case (yet) 23:45:43 the abd in question is supposed to be in arc and to receive data after write completion. 23:47:19 arc_write_ready -> abd_copy_off -> abd_iterate_func2 -> abd_copy_off_cb -> bcopy_ck_size -> GPF :) 23:47:28 tsoome: Does that mean you were just lucky/unlucky and hit this when not trying to find it? 23:47:47 yep.