| Message ID | 1506816410-10230-3-git-send-email-me@tobin.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote: > Use the %pP functionality to explicitly allow kernel > pointers to be logged for stack traces. > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > --- > arch/arm64/kernel/traps.c | 4 ++-- > kernel/printk/printk.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > index 5ea4b85..fe09660 100644 > --- a/arch/arm64/kernel/traps.c > +++ b/arch/arm64/kernel/traps.c > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) > struct stackframe frame; > int skip; > > - pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); > + pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk); Why do we care for pr_debug? > if (!tsk) > tsk = current; > @@ -233,7 +233,7 @@ static int __die(const char *str, int err, struct pt_regs *regs) > > print_modules(); > __show_regs(regs); > - pr_emerg("Process %.*s (pid: %d, stack limit = 0x%p)\n", > + pr_emerg("Process %.*s (pid: %d, stack limit = 0x%pP)\n", > TASK_COMM_LEN, tsk->comm, task_pid_nr(tsk), > end_of_stack(tsk)); > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 512f7c2..af0bc8e 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c It probably makes sense to keep this separate from arch/ changes Will
On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote: > On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote: > > Use the %pP functionality to explicitly allow kernel > > pointers to be logged for stack traces. > > > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > > --- > > arch/arm64/kernel/traps.c | 4 ++-- > > kernel/printk/printk.c | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > index 5ea4b85..fe09660 100644 > > --- a/arch/arm64/kernel/traps.c > > +++ b/arch/arm64/kernel/traps.c > > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) > > struct stackframe frame; > > int skip; > > > > - pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); > > + pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk); > > Why do we care for pr_debug? Thanks for the review Will. I did not do the original work on this series so do not know the original intent behind this change. I tend to agree with your comment. Will remove this change for v3. > > if (!tsk) > > tsk = current; > > @@ -233,7 +233,7 @@ static int __die(const char *str, int err, struct pt_regs *regs) > > > > print_modules(); > > __show_regs(regs); > > - pr_emerg("Process %.*s (pid: %d, stack limit = 0x%p)\n", > > + pr_emerg("Process %.*s (pid: %d, stack limit = 0x%pP)\n", > > TASK_COMM_LEN, tsk->comm, task_pid_nr(tsk), > > end_of_stack(tsk)); > > > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > > index 512f7c2..af0bc8e 100644 > > --- a/kernel/printk/printk.c > > +++ b/kernel/printk/printk.c > > It probably makes sense to keep this separate from arch/ changes Point noted. thanks, Tobin.
On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote: > On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote: > > Use the %pP functionality to explicitly allow kernel > > pointers to be logged for stack traces. > > > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > > --- > > arch/arm64/kernel/traps.c | 4 ++-- > > kernel/printk/printk.c | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > index 5ea4b85..fe09660 100644 > > --- a/arch/arm64/kernel/traps.c > > +++ b/arch/arm64/kernel/traps.c > > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) > > struct stackframe frame; > > int skip; > > > > - pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); > > + pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk); > > Why do we care for pr_debug? Because you really want the real value? Seems to make sense to me... thanks, greg k-h
On Wed, Oct 04, 2017 at 10:56:57AM +0200, Greg KH wrote: > On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote: > > On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote: > > > Use the %pP functionality to explicitly allow kernel > > > pointers to be logged for stack traces. > > > > > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > > > --- > > > arch/arm64/kernel/traps.c | 4 ++-- > > > kernel/printk/printk.c | 2 +- > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > > index 5ea4b85..fe09660 100644 > > > --- a/arch/arm64/kernel/traps.c > > > +++ b/arch/arm64/kernel/traps.c > > > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) > > > struct stackframe frame; > > > int skip; > > > > > > - pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); > > > + pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk); > > > > Why do we care for pr_debug? > > Because you really want the real value? Seems to make sense to me... Just seems like anybody debugging the kernel using pr_debug can probably change /proc/sys/kernel/kptr_restrict... Will
On Wed, Oct 04, 2017 at 09:58:10AM +0100, Will Deacon wrote: > On Wed, Oct 04, 2017 at 10:56:57AM +0200, Greg KH wrote: > > On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote: > > > On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote: > > > > Use the %pP functionality to explicitly allow kernel > > > > pointers to be logged for stack traces. > > > > > > > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > > > > --- > > > > arch/arm64/kernel/traps.c | 4 ++-- > > > > kernel/printk/printk.c | 2 +- > > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > > > index 5ea4b85..fe09660 100644 > > > > --- a/arch/arm64/kernel/traps.c > > > > +++ b/arch/arm64/kernel/traps.c > > > > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) > > > > struct stackframe frame; > > > > int skip; > > > > > > > > - pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); > > > > + pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk); > > > > > > Why do we care for pr_debug? > > > > Because you really want the real value? Seems to make sense to me... > > Just seems like anybody debugging the kernel using pr_debug can probably > change /proc/sys/kernel/kptr_restrict... Ok, fair enough :)
On Wed, Oct 04, 2017 at 11:02:55AM +0200, Greg KH wrote: > On Wed, Oct 04, 2017 at 09:58:10AM +0100, Will Deacon wrote: > > On Wed, Oct 04, 2017 at 10:56:57AM +0200, Greg KH wrote: > > > On Mon, Oct 02, 2017 at 11:42:05AM +0100, Will Deacon wrote: > > > > On Sun, Oct 01, 2017 at 11:06:46AM +1100, Tobin C. Harding wrote: > > > > > Use the %pP functionality to explicitly allow kernel > > > > > pointers to be logged for stack traces. > > > > > > > > > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > > > > > --- > > > > > arch/arm64/kernel/traps.c | 4 ++-- > > > > > kernel/printk/printk.c | 2 +- > > > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > > > > index 5ea4b85..fe09660 100644 > > > > > --- a/arch/arm64/kernel/traps.c > > > > > +++ b/arch/arm64/kernel/traps.c > > > > > @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) > > > > > struct stackframe frame; > > > > > int skip; > > > > > > > > > > - pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); > > > > > + pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk); > > > > > > > > Why do we care for pr_debug? > > > > > > Because you really want the real value? Seems to make sense to me... > > > > Just seems like anybody debugging the kernel using pr_debug can probably > > change /proc/sys/kernel/kptr_restrict... > > Ok, fair enough :) For what its worth I agree with Will.
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index 5ea4b85..fe09660 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -147,7 +147,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) struct stackframe frame; int skip; - pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk); + pr_debug("%s(regs = %pP tsk = %pP)\n", __func__, regs, tsk); if (!tsk) tsk = current; @@ -233,7 +233,7 @@ static int __die(const char *str, int err, struct pt_regs *regs) print_modules(); __show_regs(regs); - pr_emerg("Process %.*s (pid: %d, stack limit = 0x%p)\n", + pr_emerg("Process %.*s (pid: %d, stack limit = 0x%pP)\n", TASK_COMM_LEN, tsk->comm, task_pid_nr(tsk), end_of_stack(tsk)); diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 512f7c2..af0bc8e 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -3142,7 +3142,7 @@ void show_regs_print_info(const char *log_lvl) { dump_stack_print_info(log_lvl); - printk("%stask: %p task.stack: %p\n", + printk("%stask: %pP task.stack: %pP\n", log_lvl, current, task_stack_page(current)); }
Use the %pP functionality to explicitly allow kernel pointers to be logged for stack traces. Signed-off-by: Tobin C. Harding <me@tobin.cc> --- arch/arm64/kernel/traps.c | 4 ++-- kernel/printk/printk.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)