On Tue, Sep 8, 2020 at 8:15 AM Christoph Hellwig [off-list ref] wrote:

quoted

+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.

quoted

+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel