Re: [PATCH 13/13] kdb: Add mdi, mdiW / mdiWcN commands to show iomapped memory

[Daniel Thompson <daniel.thompson@linaro.org>]

On Tue, Jun 18, 2024 at 12:33:05PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Jun 18, 2024 at 8:59 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> >
> > On Mon, Jun 17, 2024 at 05:34:47PM -0700, Douglas Anderson wrote:
> > > Add commands that are like the other "md" commands but that allow you
> > > to read memory that's in the IO space.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >
> > Sorry to be the bearer of bad news but...
> >
> >
> > > ---
> > > <snip>
> > > +/*
> > > + * kdb_getioword
> > > + * Inputs:
> > > + *   word    Pointer to the word to receive the result.
> > > + *   addr    Address of the area to copy.
> > > + *   size    Size of the area.
> > > + * Returns:
> > > + *   0 for success, < 0 for error.
> > > + */
> > > +int kdb_getioword(unsigned long *word, unsigned long addr, size_t size)
> > > +{
> > > +     void __iomem *mapped = ioremap(addr, size);
> >
> > ioremap() is a might_sleep() function. It's unsafe to call it from the
> > debug trap handler.
>
> Hmmmm. Do you have a pointer to documentation or code showing that
> it's a might_sleep() function. I was worried about this initially but
> I couldn't find any documentation or code indicating it to be so. I
> also got no warnings when I ran my prototype code and then the code
> worked fine, so I assumed that it must somehow work. Sigh...
>
> Looking more closely, maybe this is:
>
> ioremap() -> ioremap_prot() -> generic_ioremap_prot() ->
> __get_vm_area_caller() -> __get_vm_area_node() -> __get_vm_area_node()
>
> ...and that has a kernel allocation with GFP_KERNEL?

To be honest there were a lot of problems, I just simplified.

__get_vm_area_node() already commences with a BUG_ON(in_interrupt())
before it ends up doing a GFP_KERNEL memory allocation.

I think there are multiple calls to might_sleep() (for example from
__get_vm_area_node() -> alloc_vmap_area() ).

However even if we had preallocated some vmap addresses for peek/poke
there are still problems in ioremap_page_range() too. For example:

generic_ioremap_prot()
  -> ioremap_page_range()
    -> find_vm_area()
      -> find_vmap_area()
        -> spin_lock()

We could go further down the rabbit hole and pre-lookup the VM area too
but then we hit.

generic_ioremap_prot()
  -> ioremap_page_range()
    -> vmap_page_range()
      -> vmap_range_noflush()
        -> might_sleep()

It is remotely possible that the only lock vmap_page_range() takes is
init_mm->page_table_lock but I doubt we can be sure of that.


> I guess it also then calls alloc_vmap_area()  which has might_sleep()...
>
> I'll have to track down why no warnings triggered...
>
> > I'm afraid I don't know a safe alternative either. Machinary such as
> > kmap_atomic() needs a page and iomem won't have one.
>
> Ugh. It would be nice to come up with something since it's not
> uncommon to need to look at the state of hardware registers when a
> crash happens. In the past I've managed to get into gdb, track down a
> global variable where someone has already mapped the memory, and then
> use gdb to look at the memory. It's always a big pain, though...
>
> ...even if I could just look up the virtual address where someone else
> had already mapped it that might be enough. At least I wouldn't need
> to go track down the globals myself...
>
> ...anyway, I guess I'll ponder on it and poke if I have time...

I've often thought about implementing longjmp-on-spin-wait for kgdb for
these kind of reasons. For example I have long wanted to be able to let
the user see /proc/interrupts before the usespace comes up but the spin
locks get in the way.

This approach wouldn't make calling ioremap() safe (since we could end
up bailing out halfway through a non-atomic operation) but it could at
least give control back to kdb and let the user know they have ruined
their system.

I know... there is a *reason* I've never quite got round to writing
this!


Daniel.