Re: [PATCH 2/9] ARM: traps: use get_kernel_nofault instead of set_fs()

[Arnd Bergmann]

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