On Tue, 11 Apr 2023 13:16:18 +0100 Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > On Mon, Apr 10, 2023 at 09:45:02AM +0800, xiaosong.ma wrote: > > perform the check in dump_mapping() to print warning info and avoid crash with invalid non-NULL page->mapping. > > For example, a panic with following backtraces show dump_page will show wrong info and panic when the bad page > > is non-NULL mapping and page->mapping is 0x80000000000. > > > > crash_arm64> bt > > PID: 232 TASK: ffffff80e8c2c340 CPU: 0 COMMAND: "Binder:232_2" > > #0 [ffffffc013e5b080] sysdump_panic_event$b2bce43a479f4f7762201bfee02d7889 at ffffffc0108d7c2c > > #1 [ffffffc013e5b0c0] atomic_notifier_call_chain at ffffffc010300228 > > #2 [ffffffc013e5b2c0] panic at ffffffc0102c926c > > #3 [ffffffc013e5b370] die at ffffffc010267670 > > #4 [ffffffc013e5b3a0] die_kernel_fault at ffffffc0102808a4 > > #5 [ffffffc013e5b3d0] __do_kernel_fault at ffffffc010280820 > > #6 [ffffffc013e5b410] do_bad_area at ffffffc01028059c > > #7 [ffffffc013e5b440] do_translation_fault$4df5decbea5d08a63349aa36f07426b2 at ffffffc0111149c8 > > #8 [ffffffc013e5b470] do_mem_abort at ffffffc0100a4488 > > #9 [ffffffc013e5b5e0] el1_ia at ffffffc0100a6c00 > > #10 [ffffffc013e5b5f0] __dump_page at ffffffc0104beecc > > This doesn't show a crash in dump_mapping(), it shows a crash in > __dump_page(). um, yes. But if page->mapping is corrupted, where does __dump_page() dereference it? The initial patch (https://lkml.kernel.org/r/1680587425-4683-1-git-send-email-Xiaosong.Ma@xxxxxxxxxx) prevented __dump_page() from calling dump_mapping() if page->mapping is bad, and that presumably fixed things. > > diff --git a/fs/inode.c b/fs/inode.c > > index f453eb5..c9021e5 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -564,7 +564,8 @@ void dump_mapping(const struct address_space *mapping) > > * If mapping is an invalid pointer, we don't want to crash > > * accessing it, so probe everything depending on it carefully. > > */ > > - if (get_kernel_nofault(host, &mapping->host) || > > + if (get_kernel_nofault(mapping, &mapping) || > > + get_kernel_nofault(host, &mapping->host) || > > This patch makes no sense. Essentially, you're saying > mapping = &mapping > which is obviously wrong. We're checking for mapping==junk, so this could be get_kernel_nofault(tmp, mapping) or go direct to copy_from_kernel_nofault(). We used to have a probe_kernel_address() for this... So confusion reigns. I think making dump_mapping() tolerant of a wild mapping pointer makes sense, but I don't think we actually know why the reporter's kernel crashed.
