Date: Mon, 18 Oct 2021 08:17:56 -0600 From: Warner Losh <imp@bsdimp.com> To: Cy Schubert <Cy.Schubert@cschubert.com> Cc: Marcin Wojtas <mw@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-main@freebsd.org Subject: Re: git: 889b56c8cd84 - main - setrlimit: Take stack gap into account. Message-ID: <CANCZdfpTz4EdHdxb9kQmTpDiQP9A1feoD5QKNzZyRQyuo1f-Zg@mail.gmail.com> In-Reply-To: <202110181310.19IDABYd005908@slippy.cwsent.com> References: <202110150823.19F8NEr9047194@gitrepo.freebsd.org> <202110161602.19GG2FYs004292@slippy.cwsent.com> <CANCZdfrCPSoKeKGfuQETGv=9WYLiCBTcZv2XQ-6uC99Q1Jvp5A@mail.gmail.com> <202110181310.19IDABYd005908@slippy.cwsent.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000ce3ee805cea136fb Content-Type: text/plain; charset="UTF-8" On Mon, Oct 18, 2021 at 7:10 AM Cy Schubert <Cy.Schubert@cschubert.com> wrote: > In message > <CANCZdfrCPSoKeKGfuQETGv=9WYLiCBTcZv2XQ-6uC99Q1Jvp5A@mail.gmail.c > om> > , Warner Losh writes: > > On Sat, Oct 16, 2021, 10:02 AM Cy Schubert <Cy.Schubert@cschubert.com> > > wrote: > > > > > In message <202110150823.19F8NEr9047194@gitrepo.freebsd.org>, Marcin > > > Wojtas > > > wri > > > tes: > > > > The branch main has been updated by mw: > > > > > > > > URL: > > > > https://cgit.FreeBSD.org/src/commit/?id=889b56c8cd84c9a9f2d9e3b019c154d6 > > > > f14d9021 > > > > > > > > commit 889b56c8cd84c9a9f2d9e3b019c154d6f14d9021 > > > > Author: Dawid Gorecki <dgr@semihalf.com> > > > > AuthorDate: 2021-10-13 19:01:08 +0000 > > > > Commit: Marcin Wojtas <mw@FreeBSD.org> > > > > CommitDate: 2021-10-15 08:21:47 +0000 > > > > > > > > setrlimit: Take stack gap into account. > > > > > > > > Calling setrlimit with stack gap enabled and with low values of > stack > > > > resource limit often caused the program to abort immediately > after > > > > exiting the syscall. This happened due to the fact that the > resource > > > > limit was calculated assuming that the stack started at > sv_usrstack, > > > > while with stack gap enabled the stack is moved by a random > number > > > > of bytes. > > > > > > > > Save information about stack size in struct vmspace and adjust > the > > > > rlim_cur value. If the rlim_cur and stack gap is bigger than > > > rlim_max, > > > > then the value is truncated to rlim_max. > > > > > > > > PR: 253208 > > > > Reviewed by: kib > > > > Obtained from: Semihalf > > > > Sponsored by: Stormshield > > > > MFC after: 1 month > > > > Differential Revision: https://reviews.freebsd.org/D31516 > > > > --- > > > > sys/kern/imgact_elf.c | 5 +++-- > > > > sys/kern/kern_exec.c | 11 ++++++++--- > > > > sys/kern/kern_resource.c | 3 +++ > > > > sys/sys/imgact_elf.h | 2 +- > > > > sys/sys/sysent.h | 2 +- > > > > sys/vm/vm_map.c | 2 ++ > > > > sys/vm/vm_map.h | 1 + > > > > 7 files changed, 19 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/sys/kern/imgact_elf.c b/sys/kern/imgact_elf.c > > > > index ef1edfcabaf0..898f0f66a532 100644 > > > > --- a/sys/kern/imgact_elf.c > > > > +++ b/sys/kern/imgact_elf.c > > > > @@ -2684,7 +2684,7 @@ __elfN(untrans_prot)(vm_prot_t prot) > > > > return (flags); > > > > } > > > > > > > > -void > > > > +vm_size_t > > > > __elfN(stackgap)(struct image_params *imgp, uintptr_t *stack_base) > > > > { > > > > uintptr_t range, rbase, gap; > > > > @@ -2692,7 +2692,7 @@ __elfN(stackgap)(struct image_params *imgp, > > > uintptr_t * > > > > stack_base) > > > > > > > > pct = __elfN(aslr_stack_gap); > > > > if (pct == 0) > > > > - return; > > > > + return (0); > > > > if (pct > 50) > > > > pct = 50; > > > > range = imgp->eff_stack_sz * pct / 100; > > > > @@ -2700,4 +2700,5 @@ __elfN(stackgap)(struct image_params *imgp, > > > uintptr_t * > > > > stack_base) > > > > gap = rbase % range; > > > > gap &= ~(sizeof(u_long) - 1); > > > > *stack_base -= gap; > > > > + return (gap); > > > > } > > > > diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c > > > > index 50e75fda6cfb..9dceebdd8441 100644 > > > > --- a/sys/kern/kern_exec.c > > > > +++ b/sys/kern/kern_exec.c > > > > @@ -1148,6 +1148,7 @@ exec_new_vmspace(struct image_params *imgp, > struct > > > syse > > > > ntvec *sv) > > > > stack_prot, error, vm_mmap_to_errno(error)); > > > > return (vm_mmap_to_errno(error)); > > > > } > > > > + vmspace->vm_stkgap = 0; > > > > > > > > /* > > > > * vm_ssize and vm_maxsaddr are somewhat antiquated concepts, > but > > > they > > > > @@ -1493,12 +1494,16 @@ exec_args_get_begin_envv(struct image_args > *args) > > > > void > > > > exec_stackgap(struct image_params *imgp, uintptr_t *dp) > > > > { > > > > + struct proc *p = imgp->proc; > > > > + > > > > if (imgp->sysent->sv_stackgap == NULL || > > > > - (imgp->proc->p_fctl0 & (NT_FREEBSD_FCTL_ASLR_DISABLE | > > > > + (p->p_fctl0 & (NT_FREEBSD_FCTL_ASLR_DISABLE | > > > > NT_FREEBSD_FCTL_ASG_DISABLE)) != 0 || > > > > - (imgp->map_flags & MAP_ASLR) == 0) > > > > + (imgp->map_flags & MAP_ASLR) == 0) { > > > > + p->p_vmspace->vm_stkgap = 0; > > > > return; > > > > - imgp->sysent->sv_stackgap(imgp, dp); > > > > + } > > > > + p->p_vmspace->vm_stkgap = imgp->sysent->sv_stackgap(imgp, dp); > > > > } > > > > > > > > /* > > > > diff --git a/sys/kern/kern_resource.c b/sys/kern/kern_resource.c > > > > index 4c62961e1bc4..b556d4fded51 100644 > > > > --- a/sys/kern/kern_resource.c > > > > +++ b/sys/kern/kern_resource.c > > > > @@ -671,6 +671,9 @@ kern_proc_setrlimit(struct thread *td, struct > proc > > > *p, u_ > > > > int which, > > > > if (limp->rlim_max < 0) > > > > limp->rlim_max = RLIM_INFINITY; > > > > > > > > + if (which == RLIMIT_STACK && limp->rlim_cur != RLIM_INFINITY) > > > > + limp->rlim_cur += p->p_vmspace->vm_stkgap; > > > > + > > > > oldssiz.rlim_cur = 0; > > > > newlim = lim_alloc(); > > > > PROC_LOCK(p); > > > > diff --git a/sys/sys/imgact_elf.h b/sys/sys/imgact_elf.h > > > > index 97383c6eeeb8..294f17c87b6f 100644 > > > > --- a/sys/sys/imgact_elf.h > > > > +++ b/sys/sys/imgact_elf.h > > > > @@ -118,7 +118,7 @@ int > __elfN(remove_brand_entry)(Elf_Brandinfo > > > *entry > > > > ); > > > > int __elfN(freebsd_fixup)(uintptr_t *, struct image_params *); > > > > int __elfN(coredump)(struct thread *, struct vnode *, off_t, int); > > > > size_t __elfN(populate_note)(int, void *, void *, size_t, void > > > **); > > > > -void __elfN(stackgap)(struct image_params *, uintptr_t *); > > > > +vm_size_t __elfN(stackgap)(struct image_params *, uintptr_t *); > > > > int __elfN(freebsd_copyout_auxargs)(struct image_params *, > uintptr_t); > > > > void __elfN(puthdr)(struct thread *, void *, size_t, int, size_t, > int); > > > > void __elfN(prepare_notes)(struct thread *, struct note_info_list *, > > > > diff --git a/sys/sys/sysent.h b/sys/sys/sysent.h > > > > index ad50bf56e87d..ea96c87a79af 100644 > > > > --- a/sys/sys/sysent.h > > > > +++ b/sys/sys/sysent.h > > > > @@ -119,7 +119,7 @@ struct sysentvec { > > > > void (*sv_elf_core_prepare_notes)(struct thread *, > > > > struct note_info_list *, size_t *); > > > > int (*sv_imgact_try)(struct image_params *); > > > > - void (*sv_stackgap)(struct image_params *, > uintptr_t *); > > > > + vm_size_t (*sv_stackgap)(struct image_params *, > uintptr_t *); > > > > int (*sv_copyout_auxargs)(struct image_params *, > > > > uintptr_t); > > > > int sv_minsigstksz; /* minimum signal stack size */ > > > > diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c > > > > index 1ac4ccf72f11..87a290b998b9 100644 > > > > --- a/sys/vm/vm_map.c > > > > +++ b/sys/vm/vm_map.c > > > > @@ -343,6 +343,7 @@ vmspace_alloc(vm_offset_t min, vm_offset_t max, > > > pmap_pini > > > > t_t pinit) > > > > vm->vm_taddr = 0; > > > > vm->vm_daddr = 0; > > > > vm->vm_maxsaddr = 0; > > > > + vm->vm_stkgap = 0; > > > > return (vm); > > > > } > > > > > > > > @@ -4265,6 +4266,7 @@ vmspace_fork(struct vmspace *vm1, vm_ooffset_t > > > *fork_ch > > > > arge) > > > > vm2->vm_taddr = vm1->vm_taddr; > > > > vm2->vm_daddr = vm1->vm_daddr; > > > > vm2->vm_maxsaddr = vm1->vm_maxsaddr; > > > > + vm2->vm_stkgap = vm1->vm_stkgap; > > > > vm_map_lock(old_map); > > > > if (old_map->busy) > > > > vm_map_wait_busy(old_map); > > > > diff --git a/sys/vm/vm_map.h b/sys/vm/vm_map.h > > > > index ace205b21b42..873ff62eec4a 100644 > > > > --- a/sys/vm/vm_map.h > > > > +++ b/sys/vm/vm_map.h > > > > @@ -293,6 +293,7 @@ struct vmspace { > > > > caddr_t vm_taddr; /* (c) user virtual address of text */ > > > > caddr_t vm_daddr; /* (c) user virtual address of data */ > > > > caddr_t vm_maxsaddr; /* user VA at max stack growth */ > > > > + vm_size_t vm_stkgap; /* stack gap size in bytes */ > > > > u_int vm_refcnt; /* number of references */ > > > > /* > > > > * Keep the PMAP last, so that CPU-specific variations of that > > > > > > > > > > Is it possible to have a __FreeBSD_version bump for ports? > > > > > > > There was a bump for linuxkpi you should use for this. It was a day or so > > after the stackgap change. > > That's the one I intend to use. I used the prior one at the time but will > update the patch to use this one instead. Probably today sometime. > Might want to document it in the handbook as well. The 'doubling up' changes are harder to reconstruct later... Warner > > -- > Cheers, > Cy Schubert <Cy.Schubert@cschubert.com> > FreeBSD UNIX: <cy@FreeBSD.org> Web: https://FreeBSD.org > NTP: <cy@nwtime.org> Web: https://nwtime.org > > The need of the many outweighs the greed of the few. > > > > --000000000000ce3ee805cea136fb--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpTz4EdHdxb9kQmTpDiQP9A1feoD5QKNzZyRQyuo1f-Zg>