On Tue, Sep 8, 2020 at 8:15 AM Christoph Hellwig <hch@xxxxxx> wrote:
>
> > +static void dump_mem(const char *, const char *, unsigned long, unsigned long, bool kernel_mode);
>
> This adds a pointlessly long line.
Fixed.
> And looking at the code I don't see why the argument is even needed.
>
> dump_mem() currently does an unconditional set_fs(KERNEL_DS), so it
> should always use get_kernel_nofault.
I had looked at
if (!user_mode(regs) || in_interrupt()) {
dump_mem(KERN_EMERG, "Stack: ", regs->ARM_sp,
THREAD_SIZE + (unsigned
long)task_stack_page(tsk), kernel_mode);
which told me that there should be at least some code path ending up in
__die() that has in_interrupt() set but comes from user mode.
In this case, the memory to print would be a user pointer and cannot be
accessed by get_kernel_nofault() (but could be accessed with
"set_fs(KERNEL_DS); __get_user()").
I looked through the history now and the only code path I could
find that would arrive here this way is from bad_mode(), indicating
that there is probably a hardware bug or the contents of *regs are
corrupted.
Russell might have a better explanation for this, but I would assume
now that you are right in that we don't ever need to care about
dumping user space addresses here.
> > +static void dump_instr(const char *lvl, struct pt_regs *regs)
> > {
> > unsigned long addr = instruction_pointer(regs);
> > const int thumb = thumb_mode(regs);
> > @@ -173,10 +169,20 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
> > for (i = -4; i < 1 + !!thumb; i++) {
> > unsigned int val, bad;
> >
> > - if (thumb)
> > - bad = get_user(val, &((u16 *)addr)[i]);
> > - else
> > - bad = get_user(val, &((u32 *)addr)[i]);
> > + if (!user_mode(regs)) {
> > + if (thumb) {
> > + u16 val16;
> > + bad = get_kernel_nofault(val16, &((u16 *)addr)[i]);
> > + val = val16;
> > + } else {
> > + bad = get_kernel_nofault(val, &((u32 *)addr)[i]);
> > + }
> > + } else {
> > + if (thumb)
> > + bad = get_user(val, &((u16 *)addr)[i]);
> > + else
> > + bad = get_user(val, &((u32 *)addr)[i]);
> > + }
>
> When I looked at this earlier I just added a little helper to make
> this a little easier to read. Here is my patch from an old tree:
>
> http://git.infradead.org/users/hch/misc.git/commitdiff/67413030ccb7a64a7eb828e13ff0795f4eadfeb7
I think your version was broken for the in-kernel thumb version
because get_kernel_nofault does not widen the result
from 16 to 32 bits. I tried fixing this in your version, but the
result ended up more complex than my version here, so I
decided to keep what I had.
Arnd
