Messages in this thread | | | Subject | Re: CONFIG_ORC_UNWINDER=y breaks get_wchan()? | From | Qi Zheng <> | Date | Wed, 22 Sep 2021 11:30:01 +0800 |
| |
On 9/22/21 8:15 AM, Josh Poimboeuf wrote: > On Tue, Sep 21, 2021 at 12:32:49PM -0700, Vito Caputo wrote: >> Is this an oversight of the ORC_UNWINDER implementation? It's >> arguably a regression to completely break wchans for tools like `ps -o >> wchan` and `top`, or my window manager and its separate monitoring >> utility. Presumably there are other tools out there sampling wchans >> for monitoring as well, there's also an internal use of get_chan() in >> kernel/sched/fair.c for sleep profiling. >> >> I've occasionally seen when monitoring at a high sample rate (60hz) on >> something churny like a parallel kernel or systemd build, there's a >> spurious non-zero sample coming out of /proc/[pid]/wchan containing a >> hexadecimal address like 0xffffa9ebc181bcf8. This all smells broken, >> is get_wchan() occasionally spitting out random junk here kallsyms >> can't resolve, because get_chan() is completely ignorant of >> ORC_UNWINDER's effects? > > Hi Vito, > > Thanks for reporting this. Does this patch fix your issue? > > https://lkml.kernel.org/r/20210831083625.59554-1-zhengqi.arch@bytedance.com > > Though, considering wchan has been silently broken for four years, I do > wonder what the impact would be if we were to just continue to show "0" > (and change frame pointers to do the same).
Agree, Or remove get_wchan() directly.
> > The kernel is much more cautious than it used to be about exposing this > type of thing. Can you elaborate on your use case? > > If we do keep it, we might want to require CAP_SYS_ADMIN anyway, for > similar reasons as > > f8a00cef1720 ("proc: restrict kernel stack dumps to root") > > ... since presumably proc_pid_wchan()'s use of '%ps' can result in an > actual address getting printed if the unwind gets confused, thanks to > __sprint_symbol()'s backup option if kallsyms_lookup_buildid() doesn't > find a name. > > Though, instead of requiring CAP_SYS_ADMIN, maybe we can just fix > __sprint_symbol() to not expose addresses? > > Or is there some other reason for needing CAP_SYS_ADMIN? Jann? >
| |