> Date: Sat, 16 Aug 2025 15:19:26 +0200
> From: Martin Pieuchot <[email protected]>
>
> On 16/08/25(Sat) 14:41, Mark Kettenis wrote:
> > > Date: Fri, 15 Aug 2025 15:53:25 +0200
> > > From: Martin Pieuchot <[email protected]>
> > >
> > > On 15/08/25(Fri) 15:45, Mark Kettenis wrote:
> > > > > Date: Fri, 15 Aug 2025 14:06:06 +0200
> > > > > From: Martin Pieuchot <[email protected]>
> > > > >
> > > > > On 15/08/25(Fri) 13:17, Mark Kettenis wrote:
> > > > > > > Date: Fri, 15 Aug 2025 11:51:18 +0200
> > > > > > > From: Martin Pieuchot <[email protected]>
> > > > > > >
> > > > > > > On 15/08/25(Fri) 09:39, Miod Vallat wrote:
> > > > > > > > > Please don't. Keeping that page read-only is important for
> > > > > > > > > security.
> > > > > > > > > Maybe if nobody cares about the amd64 and i386 pmaps we
> > > > > > > > > should just
> > > > > > > > > delete those architectures?
> > > > > > > >
> > > > > > > > But remember, because the end argument was wrong (sz instead of
> > > > > > > > va +
> > > > > > > > sz), this call did *nothing*.
> > > > > > > >
> > > > > > > > At least the commented out code will be correct now.
> > > > > > >
> > > > > > > Exactly. Since you committed this code Mark it does nothing.
> > > > > > > That's
> > > > > > > what I said in the original report it's dead code.
> > > > > >
> > > > > > The original code (before I "broke" it) did an unmap of the memory.
> > > > > > BTW that means we need to fix the comment.
> > > > >
> > > > > Indeed. I'd be glad if you could do that when enable the
> > > > > uvm_map_protect()
> > > > > call.
> > > >
> > > > ok?
> > >
> > > Would you please integrate the proper refcount & error handling I fixed in
> > > my diff?
> >
> > I'm not sure the refcount and error handling in your diff makes sense.
> > I'm not sure the current refcount and error handling makes sense
> > either.
>
> All I want is correct code. This code has already been copied and it
> took dv@ and I some iteration to get the refcount correct in vmm(4).
> The diff I produced might not be needed but is similar to what we have
> there and can be copied.
>
> > This sigobject is a permanent uvm object that is never going to be
> > freed. It gets created the first time we do an exec. And if for some
> > reason we fail to create it you won't be able to exec anything. Not
> > even init(8)!
>
> Sure, I'm fine with a panic.
>
> > So I think it makes more sense to simply panic if that uvm_map() call
> > fails. Not sure what to do with the uvm_map_protect() call. That
> > call should never fail. And even if it does fail, the kernel will
> > work just fine. So I'm inclined to leave it without an error check.
>
> Can we print a message in such case? I dislike silent failures.
>
> > At that point the reference counting should be really simple. The
> > uao_create() call already gives us a reference. And since we no
> > longer unmap the object we don't need another one. So we can actually
> > drop the uao_reference() call that we have right now.
>
> Right.
>
> > I would rather make those changes in a separate commit though.
>
> Go for it.
So here is the diff I was thinking about. This fixes the reference
count and turns failures into panic. I also removed the "r" variable
since it was pointless and doing so results in more readable code.
The comment gets adjusted again since we no longer explicitly "add a
permanent reference".
ok?
Index: kern/kern_exec.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_exec.c,v
retrieving revision 1.267
diff -u -p -r1.267 kern_exec.c
--- kern/kern_exec.c 23 Aug 2025 16:12:52 -0000 1.267
+++ kern/kern_exec.c 23 Aug 2025 16:51:39 -0000
@@ -860,12 +860,12 @@ exec_sigcode_map(struct process *pr)
/*
* If we don't have a sigobject yet, create one.
*
- * sigobject is an anonymous memory object (just like SYSV shared
- * memory) that we keep a permanent reference to and that we map
- * in all processes that need this sigcode. The creation is simple,
- * we create an object, add a permanent reference to it, map it in
- * kernel space, copy out the sigcode to it and map it PROT_READ
- * such that the coredump code can write it out into core dumps.
+ * sigobject is an anonymous memory object (just like SYSV
+ * shared memory) that we keep a permanent reference to and
+ * that we map in all processes that need this sigcode. The
+ * creation is simple, we create an object, map it in kernel
+ * space, copy out the sigcode to it and map it PROT_READ such
+ * that the coredump code can write it out into core dumps.
* Then we map it with PROT_EXEC into the process just the way
* sys_mmap would map it.
*/
@@ -874,17 +874,13 @@ exec_sigcode_map(struct process *pr)
extern u_char sigfill[];
size_t off, left;
vaddr_t va;
- int r;
- sigobject = uao_create(sz, 0);
- uao_reference(sigobject); /* permanent reference */
+ sigobject = uao_create(sz, 0); /* permanent reference */
- if ((r = uvm_map(kernel_map, &va, round_page(sz), sigobject,
- 0, 0, UVM_MAPFLAG(PROT_READ | PROT_WRITE, PROT_READ |
PROT_WRITE,
- MAP_INHERIT_SHARE, MADV_RANDOM, 0)))) {
- uao_detach(sigobject);
- return (ENOMEM);
- }
+ if (uvm_map(kernel_map, &va, round_page(sz), sigobject, 0, 0,
+ UVM_MAPFLAG(PROT_READ | PROT_WRITE, PROT_READ | PROT_WRITE,
+ MAP_INHERIT_SHARE, MADV_RANDOM, 0)))
+ panic("can't map sigobject");
for (off = 0, left = round_page(sz); left != 0;
off += sigfillsiz) {
@@ -894,8 +890,10 @@ exec_sigcode_map(struct process *pr)
}
memcpy((caddr_t)va, sigcode, sz);
- (void) uvm_map_protect(kernel_map, va, round_page(va + sz),
- PROT_READ, 0, FALSE, FALSE);
+ if (uvm_map_protect(kernel_map, va, round_page(va + sz),
+ PROT_READ, 0, FALSE, FALSE))
+ panic("can't write-protect sigobject");
+
sigcode_va = va;
sigcode_sz = round_page(sz);
}
