| Message ID | 20231110110348.1815612-5-benjamin@sipsolutions.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | General cleanups and fixes from SECCOMP patchset | expand |
On Fri, Nov 10, 2023 at 12:03 PM <benjamin@sipsolutions.net> wrote: > > From: Benjamin Berg <benjamin@sipsolutions.net> > > The threads allocated inside the kernel have only a single page of > stack. Unfortunately, the vfprintf function in standard glibc may use > too much stack-space, overflowing it. Another option is giving the helper threads more memory, we don't have that many. Did you explore this option already?
Hi, On Thu, 2024-01-04 at 23:37 +0100, Richard Weinberger wrote: > On Fri, Nov 10, 2023 at 12:03 PM <benjamin@sipsolutions.net> wrote: > > > > From: Benjamin Berg <benjamin@sipsolutions.net> > > > > The threads allocated inside the kernel have only a single page of > > stack. Unfortunately, the vfprintf function in standard glibc may use > > too much stack-space, overflowing it. > > Another option is giving the helper threads more memory, we don't have > that many. > Did you explore this option already? I had not really considered that. One thing though is that userspace_tramp calls os_info while working with the stub stack. So that also means setting STUB_DATA_PAGES to 2. But, I suspect we may want to do that anyway eventually to fit the ever increasing mcontext size for all the AVX512 registers and such. As I said, I think that is fine to do. Benjamin
On Fri, 2024-01-05 at 09:12 +0100, Benjamin Berg wrote: > > Another option is giving the helper threads more memory, we don't have > > that many. > > Did you explore this option already? > > I had not really considered that. > > One thing though is that userspace_tramp calls os_info while working > with the stub stack. So that also means setting STUB_DATA_PAGES to 2. > But, I suspect we may want to do that anyway eventually to fit the ever > increasing mcontext size for all the AVX512 registers and such. That'll probably happen eventually regardless ... > As I said, I think that is fine to do. But I'm not sure it's a good solution to give more stack space to the threads and think/hope that glibc won't make more assumptions about the amount of space it can use ... who knows if it uses alloca() inside somewhere, for example? After all, userspace stacks are multiple megabytes by default? johannes
----- Ursprüngliche Mail ----- > Von: "Johannes Berg" <johannes@sipsolutions.net> > An: "Benjamin Berg" <benjamin@sipsolutions.net>, "Richard Weinberger" <richard.weinberger@gmail.com> > CC: "linux-um" <linux-um@lists.infradead.org> > Gesendet: Freitag, 5. Januar 2024 09:56:11 > Betreff: Re: [PATCH v3 04/11] um: Don't use vfprintf() for os_info() > On Fri, 2024-01-05 at 09:12 +0100, Benjamin Berg wrote: >> > Another option is giving the helper threads more memory, we don't have >> > that many. >> > Did you explore this option already? >> >> I had not really considered that. >> >> One thing though is that userspace_tramp calls os_info while working >> with the stub stack. So that also means setting STUB_DATA_PAGES to 2. >> But, I suspect we may want to do that anyway eventually to fit the ever >> increasing mcontext size for all the AVX512 registers and such. > > That'll probably happen eventually regardless ... > >> As I said, I think that is fine to do. > > But I'm not sure it's a good solution to give more stack space to the > threads and think/hope that glibc won't make more assumptions about the > amount of space it can use ... who knows if it uses alloca() inside > somewhere, for example? After all, userspace stacks are multiple > megabytes by default? For thread stacks things are a bit different. glibc is in general more heap than stack greedy, it uses malloc() almost everywhere. For pthreads the minimal stack size on x86 is 16k (_SC_THREAD_STACK_MIN). Donating four pages to each helper thread seems okay to me. And if I understand _SC_THREAD_STACK_MIN correctly, this is the minimal stack size the glibc library can deal with. Thanks, //richard
diff --git a/arch/um/os-Linux/util.c b/arch/um/os-Linux/util.c index fc0f2a9dee5a..1dca4ffbd572 100644 --- a/arch/um/os-Linux/util.c +++ b/arch/um/os-Linux/util.c @@ -173,23 +173,38 @@ __uml_setup("quiet", quiet_cmd_param, "quiet\n" " Turns off information messages during boot.\n\n"); +/* + * The os_info/os_warn functions will be called by helper threads. These + * have a very limited stack size and using the libc formatting functions + * may overflow the stack. + * So pull in the kernel vscnprintf and use that instead with a fixed + * on-stack buffer. + */ +int vscnprintf(char *buf, size_t size, const char *fmt, va_list args); + void os_info(const char *fmt, ...) { + char buf[256]; va_list list; + int len; if (quiet_info) return; va_start(list, fmt); - vfprintf(stderr, fmt, list); + len = vscnprintf(buf, sizeof(buf), fmt, list); + fwrite(buf, len, 1, stderr); va_end(list); } void os_warn(const char *fmt, ...) { + char buf[256]; va_list list; + int len; va_start(list, fmt); - vfprintf(stderr, fmt, list); + len = vscnprintf(buf, sizeof(buf), fmt, list); + fwrite(buf, len, 1, stderr); va_end(list); }