| Message ID | 20220418132217.1573072-2-zhe.he@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hardened usercopy and stacktrace improvement | expand |
On Mon, Apr 18, 2022 at 09:22:10PM +0800, He Zhe wrote: > Currently stack_trace_consume_fn can only have pc of each frame of the > stack. Copying-beyond-the-frame-detection also needs fp of current and > previous frame. Other detection algorithm in the future may need more > information of the frame. > > We define a frame_info to include them all. > > > Signed-off-by: He Zhe <zhe.he@windriver.com> > --- > include/linux/stacktrace.h | 9 ++++++++- > kernel/stacktrace.c | 10 +++++----- > 2 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h > index 97455880ac41..5a61bfafe6f0 100644 > --- a/include/linux/stacktrace.h > +++ b/include/linux/stacktrace.h > @@ -10,15 +10,22 @@ struct pt_regs; > > #ifdef CONFIG_ARCH_STACKWALK > > +struct frame_info { > + unsigned long pc; > + unsigned long fp; > + unsigned long prev_fp; > +}; I don't think this should be exposed through a generic interface; the `fp` and `prev_fp` values are only meaningful with arch-specific knowledge, and they're *very* easy to misuse (e.g. when transitioning from one stack to another). There's also a bunch of other information one may or may not want, depending on what you're trying to achieve. I am happy to have an arch-specific internal unwinder where we can access this information, and *maybe* it makes sense to have a generic API that passes some opaque token, but I don't think we should make the structure generic. Thanks, Mark. > + > /** > * stack_trace_consume_fn - Callback for arch_stack_walk() > * @cookie: Caller supplied pointer handed back by arch_stack_walk() > * @addr: The stack entry address to consume > + * @fi: The frame information to consume > * > * Return: True, if the entry was consumed or skipped > * False, if there is no space left to store > */ > -typedef bool (*stack_trace_consume_fn)(void *cookie, unsigned long addr); > +typedef bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi); > /** > * arch_stack_walk - Architecture specific function to walk the stack > * @consume_entry: Callback which is invoked by the architecture code for > diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c > index 9ed5ce989415..2d0a2812e92b 100644 > --- a/kernel/stacktrace.c > +++ b/kernel/stacktrace.c > @@ -79,7 +79,7 @@ struct stacktrace_cookie { > unsigned int len; > }; > > -static bool stack_trace_consume_entry(void *cookie, unsigned long addr) > +static bool stack_trace_consume_entry(void *cookie, struct frame_info *fi) > { > struct stacktrace_cookie *c = cookie; > > @@ -90,15 +90,15 @@ static bool stack_trace_consume_entry(void *cookie, unsigned long addr) > c->skip--; > return true; > } > - c->store[c->len++] = addr; > + c->store[c->len++] = fi->pc; > return c->len < c->size; > } > > -static bool stack_trace_consume_entry_nosched(void *cookie, unsigned long addr) > +static bool stack_trace_consume_entry_nosched(void *cookie, struct frame_info *fi) > { > - if (in_sched_functions(addr)) > + if (in_sched_functions(fi->pc)) > return true; > - return stack_trace_consume_entry(cookie, addr); > + return stack_trace_consume_entry(cookie, fi); > } > > /** > -- > 2.25.1 >
On 4/19/22 21:09, Mark Rutland wrote: > On Mon, Apr 18, 2022 at 09:22:10PM +0800, He Zhe wrote: >> Currently stack_trace_consume_fn can only have pc of each frame of the >> stack. Copying-beyond-the-frame-detection also needs fp of current and >> previous frame. Other detection algorithm in the future may need more >> information of the frame. >> >> We define a frame_info to include them all. >> >> >> Signed-off-by: He Zhe <zhe.he@windriver.com> >> --- >> include/linux/stacktrace.h | 9 ++++++++- >> kernel/stacktrace.c | 10 +++++----- >> 2 files changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h >> index 97455880ac41..5a61bfafe6f0 100644 >> --- a/include/linux/stacktrace.h >> +++ b/include/linux/stacktrace.h >> @@ -10,15 +10,22 @@ struct pt_regs; >> >> #ifdef CONFIG_ARCH_STACKWALK >> >> +struct frame_info { >> + unsigned long pc; >> + unsigned long fp; >> + unsigned long prev_fp; >> +}; > I don't think this should be exposed through a generic interface; the `fp` and > `prev_fp` values are only meaningful with arch-specific knowledge, and they're > *very* easy to misuse (e.g. when transitioning from one stack to another). > There's also a bunch of other information one may or may not want, depending on > what you're trying to achieve. > > I am happy to have an arch-specific internal unwinder where we can access this > information, and *maybe* it makes sense to have a generic API that passes some > opaque token, but I don't think we should make the structure generic. Thanks for thoughts. I saw unwind_frame and etc was made private earlier and took that as a hint that all further stack walk things should be based on those functions and came up with this. But OK, good to know that arch-specific unwind would be fine, I'll redo this series in that way. Thanks, Zhe > > Thanks, > Mark. > >> + >> /** >> * stack_trace_consume_fn - Callback for arch_stack_walk() >> * @cookie: Caller supplied pointer handed back by arch_stack_walk() >> * @addr: The stack entry address to consume >> + * @fi: The frame information to consume >> * >> * Return: True, if the entry was consumed or skipped >> * False, if there is no space left to store >> */ >> -typedef bool (*stack_trace_consume_fn)(void *cookie, unsigned long addr); >> +typedef bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi); >> /** >> * arch_stack_walk - Architecture specific function to walk the stack >> * @consume_entry: Callback which is invoked by the architecture code for >> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c >> index 9ed5ce989415..2d0a2812e92b 100644 >> --- a/kernel/stacktrace.c >> +++ b/kernel/stacktrace.c >> @@ -79,7 +79,7 @@ struct stacktrace_cookie { >> unsigned int len; >> }; >> >> -static bool stack_trace_consume_entry(void *cookie, unsigned long addr) >> +static bool stack_trace_consume_entry(void *cookie, struct frame_info *fi) >> { >> struct stacktrace_cookie *c = cookie; >> >> @@ -90,15 +90,15 @@ static bool stack_trace_consume_entry(void *cookie, unsigned long addr) >> c->skip--; >> return true; >> } >> - c->store[c->len++] = addr; >> + c->store[c->len++] = fi->pc; >> return c->len < c->size; >> } >> >> -static bool stack_trace_consume_entry_nosched(void *cookie, unsigned long addr) >> +static bool stack_trace_consume_entry_nosched(void *cookie, struct frame_info *fi) >> { >> - if (in_sched_functions(addr)) >> + if (in_sched_functions(fi->pc)) >> return true; >> - return stack_trace_consume_entry(cookie, addr); >> + return stack_trace_consume_entry(cookie, fi); >> } >> >> /** >> -- >> 2.25.1 >>
diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h index 97455880ac41..5a61bfafe6f0 100644 --- a/include/linux/stacktrace.h +++ b/include/linux/stacktrace.h @@ -10,15 +10,22 @@ struct pt_regs; #ifdef CONFIG_ARCH_STACKWALK +struct frame_info { + unsigned long pc; + unsigned long fp; + unsigned long prev_fp; +}; + /** * stack_trace_consume_fn - Callback for arch_stack_walk() * @cookie: Caller supplied pointer handed back by arch_stack_walk() * @addr: The stack entry address to consume + * @fi: The frame information to consume * * Return: True, if the entry was consumed or skipped * False, if there is no space left to store */ -typedef bool (*stack_trace_consume_fn)(void *cookie, unsigned long addr); +typedef bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi); /** * arch_stack_walk - Architecture specific function to walk the stack * @consume_entry: Callback which is invoked by the architecture code for diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c index 9ed5ce989415..2d0a2812e92b 100644 --- a/kernel/stacktrace.c +++ b/kernel/stacktrace.c @@ -79,7 +79,7 @@ struct stacktrace_cookie { unsigned int len; }; -static bool stack_trace_consume_entry(void *cookie, unsigned long addr) +static bool stack_trace_consume_entry(void *cookie, struct frame_info *fi) { struct stacktrace_cookie *c = cookie; @@ -90,15 +90,15 @@ static bool stack_trace_consume_entry(void *cookie, unsigned long addr) c->skip--; return true; } - c->store[c->len++] = addr; + c->store[c->len++] = fi->pc; return c->len < c->size; } -static bool stack_trace_consume_entry_nosched(void *cookie, unsigned long addr) +static bool stack_trace_consume_entry_nosched(void *cookie, struct frame_info *fi) { - if (in_sched_functions(addr)) + if (in_sched_functions(fi->pc)) return true; - return stack_trace_consume_entry(cookie, addr); + return stack_trace_consume_entry(cookie, fi); } /**
Currently stack_trace_consume_fn can only have pc of each frame of the stack. Copying-beyond-the-frame-detection also needs fp of current and previous frame. Other detection algorithm in the future may need more information of the frame. We define a frame_info to include them all. Signed-off-by: He Zhe <zhe.he@windriver.com> --- include/linux/stacktrace.h | 9 ++++++++- kernel/stacktrace.c | 10 +++++----- 2 files changed, 13 insertions(+), 6 deletions(-)