From: Mark Kettenis Subject: Re: make stacktrace_save_utrace() nicer to btrace(8) To: Alexandr Nedvedicky Cc: tech@openbsd.org Date: Sun, 03 Aug 2025 11:36:20 +0200 > Date: Sat, 2 Aug 2025 23:59:51 +0200 > From: Alexandr Nedvedicky > > Hello, > > this diff resurrects idea from diff I've sent few weeks ago [1]. > kettenis@ was not excited about that change back then. > > I've sent another version earlier [2] today which was still kind > of disappointing to kettenis@. This time we had a chance to > discuss those changes at the table. > > The outcome was that the change [3] which avoids the crash > went in [4]. The remaining part which I'd like to get it in > is presented here. > > the change builds an assumption into stacktrace_save_utrace() > function that if particular stack frame contains an invalid > address to next frame (,f_frame is bogus address), then > the return address kept in frame is also invalid. > > stop reading here, if your are not interested in details. > > the reason why I don't want to pass potentially invalid > instruction address to btrace(8) is the cost the btrace > process needs to pay to figure out the address does not > point to instruction in traced process. > > btrace process tries to resolve address to symbol. if > it finds out there is no matching symbol for presented address, > it uses ioctl(2) on dt(4) to query kernel which .so object > the instruction belongs to. If instruction belongs to so .so > in traced process, the kernel opens a file descriptor in > btrace file descriptor table and associates a vnode which > refers to .so with it. It then passes the file descriptor > number back to btrace process, so btrace can use mmap(2) > and libelf to load symbol table. > > Works nicely as long as btrace process is presented with > valid addresses. However if traced process happens not > to use frame pointers correctly then stacktrace_save_utrace() > function may misinterpret frame and return invalid address > to btrace. btrace then does ioctl(2) just to find out > the address is invalid. > > after a brief discussion kettenis@ agrees current simple > approach proposed here seems to be good enough for now. > the alternative option would be to implement a negative > caching in btrace. the btrace could remember addresses > (page ranges) the dt(5) failed to resolve to symbol tables. > This is something that can be done later. > > diff follows. Thanks Sasha, I still have my doubts about whether the kernel should enforce this policy, but I think enforcing the policy consistently and avoiding taking vmmap lock in this code is better. So ok kettenis@ for the diff. I just have a suggestion for changing the text of the comment. > > [1] https://marc.info/?l=openbsd-tech&m=174938917209440&w=2 > > [2] https://marc.info/?l=openbsd-tech&m=175414134428800&w=2 > > [3] https://marc.info/?l=openbsd-tech&m=175415293201212&w=2 > > [4] https://marc.info/?l=openbsd-cvs&m=175416247805522&w=2 > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/arch/amd64/amd64/db_trace.c b/sys/arch/amd64/amd64/db_trace.c > index 5279024e04d..c4499042036 100644 > --- a/sys/arch/amd64/amd64/db_trace.c > +++ b/sys/arch/amd64/amd64/db_trace.c > @@ -319,8 +319,19 @@ stacktrace_save_utrace(struct stacktrace *st) > st->st_pc[st->st_count++] = lastframe->f_retaddr; > > while (frame != NULL && st->st_count < STACKTRACE_MAX) { > - if (copyin(frame, &f, sizeof(f)) != 0) > + if (copyin(frame, &f, sizeof(f)) != 0) { > + /* > + * We assume the return address from previous frame is > + * invalid, because the frame refers to invalid > + * address. We don't want to provide invalid address > + * to btrace(8) in userland. > + */ /* * If the frame pointer read from the previous frame * is invalid, assume the return address we read * from that frame is invalid as well. */ > + if (st->st_count == 0) > + st->st_pc[0] = 0; > + else > + st->st_count--; > break; > + } > st->st_pc[st->st_count++] = f.f_retaddr; > frame = f.f_frame; > } > diff --git a/sys/arch/i386/i386/db_trace.c b/sys/arch/i386/i386/db_trace.c > index 140ba9eca33..086afb11a5a 100644 > --- a/sys/arch/i386/i386/db_trace.c > +++ b/sys/arch/i386/i386/db_trace.c > @@ -318,8 +318,19 @@ stacktrace_save_utrace(struct stacktrace *st) > st->st_pc[st->st_count++] = lastframe->f_retaddr; > > while (frame != NULL && st->st_count < STACKTRACE_MAX) { > - if (copyin(frame, &f, sizeof(f)) != 0) > + if (copyin(frame, &f, sizeof(f)) != 0) { > + /* > + * We assume the return address from previous frame is > + * invalid, because the frame refers to invalid > + * address. We don't want to provide invalid address > + * to btrace(8) in userland. > + */ > + if (st->st_count == 0) > + st->st_pc[0] = 0; > + else > + st->st_count--; > break; > + } > st->st_pc[st->st_count++] = f.f_retaddr; > frame = f.f_frame; > } > >