Re: [PATCH] arm64: set __exception_irq_entry with __irq_entry as a default

[Youngmin Nam <youngmin.nam@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 13769 bytes --]

On Mon, Apr 24, 2023 at 02:08:14PM +0100, Mark Rutland wrote:
> On Mon, Apr 24, 2023 at 02:09:05PM +0200, Dmitry Vyukov wrote:
> > On Mon, 24 Apr 2023 at 13:01, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Mon, Apr 24, 2023 at 10:04:36AM +0900, Youngmin Nam wrote:
> > > > filter_irq_stacks() is supposed to cut entries which are related irq entries
> > > > from its call stack.
> > > > And in_irqentry_text() which is called by filter_irq_stacks()
> > > > uses __irqentry_text_start/end symbol to find irq entries in callstack.
> > > >
> > > > But it doesn't work correctly as without "CONFIG_FUNCTION_GRAPH_TRACER",
> > > > arm64 kernel doesn't include gic_handle_irq which is entry point of arm64 irq
> > > > between __irqentry_text_start and __irqentry_text_end as we discussed in below link.
> > >
> > > TBH, the __irqentry_text annotations don't make much sense, and I'd love to
> > > remove them.
> > >
> > > The irqchip handlers are not the actual exception entry points, and we invoke a
> > > fair amount of code between those and the actual IRQ handlers (e.g. to map from
> > > the irq domain to the actual hander, which might involve poking chained irqchip
> > > handlers), so it doesn't make much sense for the irqchip handlers to be
> > > special.
> > >
> > > > https://lore.kernel.org/all/CACT4Y+aReMGLYua2rCLHgFpS9io5cZC04Q8GLs-uNmrn1ezxYQ@mail.gmail.com/#t
> > > >
> > > > This problem can makes unintentional deep call stack entries especially
> > > > in KASAN enabled situation as below.
> > >
> > > What exactly does KASAN need here? Is this just to limit the depth of the
> > > trace?
> > 
> > No, it's not just depth. Any uses of stack depot need stable
> > repeatable traces, so that they are deduplicated well. For irq stacks
> > it means removing the random part where the interrupt is delivered.
> > Otherwise stack depot grows without limits and overflows.

Hi Dmitry Vyukov.
Thanks for your additional comments.

> 
> Sure -- you want to filter out the non-deterministic context that the interrupt
> was taken *from*.
> 
> > We don't need the exact entry point for this. A frame "close enough"
> > may work well if there are no memory allocations/frees skipped.
> 
> With that in mind, I think what we should do is cut this at the instant we
> enter the exception; for the trace below that would be el1h_64_irq. I've added
> some line spacing there to make it stand out.
> 
> That would mean that we'd have three entry points that an interrupt trace might
> start from:
> 
> * el1h_64_irq()
> * el0t_64_irq()
> * el0t_32_irq()
>

Hi Mark.
Thanks for your kind review.

If I understand your intention corretly, I should add "__irq_entry"
to C function of irq_handler as below.

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
-asmlinkage void el1h_64_irq_handler(struct pt_regs *regs);
+asmlinkage void __irq_entry el1h_64_irq_handler(struct pt_regs *regs);

-asmlinkage void el0t_64_irq_handler(struct pt_regs *regs);
+asmlinkage void __irq_entry el0t_64_irq_handler(struct pt_regs *regs);

-asmlinkage void el0t_32_irq_handler(struct pt_regs *regs);
+asmlinkage void __irq_entry el0t_32_irq_handler(struct pt_regs *regs);

But these irq handlers are marked with "noinstr" already so that we can't put them into
irqentry section.

arch/arm64/kernel/entry-common.c:492:asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
arch/arm64/kernel/entry-common.c:730:asmlinkage void noinstr el0t_64_irq_handler(struct pt_regs *regs)
arch/arm64/kernel/entry-common.c:824:asmlinkage void noinstr el0t_32_irq_handler(struct pt_regs *regs)

Could you tell me that I am doing right ?

> ... so we might have three traces for a given interrupt, but the portion
> between that and the irqchip handler would be deterministic, so deduplication
> would only end up with three traces.
> 
> It may be useful to distinguish the three cases, since some IRQ handlers do
> different things when user_mode(regs) and/or compat_user_mode(regs) are true.
> 
> > > If so, we could easily add an API to get a stacktrace up to an IRQ exception
> > > boundary. IIRC we'd been asked for that in the past, and it's relatively simple
> > > to implement that regardless of CONFIG_FUNCTION_GRAPH_TRACER.
> > >
> > > > [ 2479.383395]I[0:launcher-loader: 1719] Stack depot reached limit capacity
> > > > [ 2479.383538]I[0:launcher-loader: 1719] WARNING: CPU: 0 PID: 1719 at lib/stackdepot.c:129 __stack_depot_save+0x464/0x46c
> > > > [ 2479.385693]I[0:launcher-loader: 1719] pstate: 624000c5 (nZCv daIF +PAN -UAO +TCO -DIT -SSBS BTYPE=--)
> > > > [ 2479.385724]I[0:launcher-loader: 1719] pc : __stack_depot_save+0x464/0x46c
> > > > [ 2479.385751]I[0:launcher-loader: 1719] lr : __stack_depot_save+0x460/0x46c
> > > > [ 2479.385774]I[0:launcher-loader: 1719] sp : ffffffc0080073c0
> > > > [ 2479.385793]I[0:launcher-loader: 1719] x29: ffffffc0080073e0 x28: ffffffd00b78a000 x27: 0000000000000000
> > > > [ 2479.385839]I[0:launcher-loader: 1719] x26: 000000000004d1dd x25: ffffff891474f000 x24: 00000000ca64d1dd
> > > > [ 2479.385882]I[0:launcher-loader: 1719] x23: 0000000000000200 x22: 0000000000000220 x21: 0000000000000040
> > > > [ 2479.385925]I[0:launcher-loader: 1719] x20: ffffffc008007440 x19: 0000000000000000 x18: 0000000000000000
> > > > [ 2479.385969]I[0:launcher-loader: 1719] x17: 2065726568207475 x16: 000000000000005e x15: 2d2d2d2d2d2d2d20
> > > > [ 2479.386013]I[0:launcher-loader: 1719] x14: 5d39313731203a72 x13: 00000000002f6b30 x12: 00000000002f6af8
> > > > [ 2479.386057]I[0:launcher-loader: 1719] x11: 00000000ffffffff x10: ffffffb90aacf000 x9 : e8a74a6c16008800
> > > > [ 2479.386101]I[0:launcher-loader: 1719] x8 : e8a74a6c16008800 x7 : 00000000002f6b30 x6 : 00000000002f6af8
> > > > [ 2479.386145]I[0:launcher-loader: 1719] x5 : ffffffc0080070c8 x4 : ffffffd00b192380 x3 : ffffffd0092b313c
> > > > [ 2479.386189]I[0:launcher-loader: 1719] x2 : 0000000000000001 x1 : 0000000000000004 x0 : 0000000000000022
> > > > [ 2479.386231]I[0:launcher-loader: 1719] Call trace:
> > > > [ 2479.386248]I[0:launcher-loader: 1719]  __stack_depot_save+0x464/0x46c
> > > > [ 2479.386273]I[0:launcher-loader: 1719]  kasan_save_stack+0x58/0x70
> > > > [ 2479.386303]I[0:launcher-loader: 1719]  save_stack_info+0x34/0x138
> > > > [ 2479.386331]I[0:launcher-loader: 1719]  kasan_save_free_info+0x18/0x24
> > > > [ 2479.386358]I[0:launcher-loader: 1719]  ____kasan_slab_free+0x16c/0x170
> > > > [ 2479.386385]I[0:launcher-loader: 1719]  __kasan_slab_free+0x10/0x20
> > > > [ 2479.386410]I[0:launcher-loader: 1719]  kmem_cache_free+0x238/0x53c
> > > > [ 2479.386435]I[0:launcher-loader: 1719]  mempool_free_slab+0x1c/0x28
> > > > [ 2479.386460]I[0:launcher-loader: 1719]  mempool_free+0x7c/0x1a0
> > > > [ 2479.386484]I[0:launcher-loader: 1719]  bvec_free+0x34/0x80
> > > > [ 2479.386514]I[0:launcher-loader: 1719]  bio_free+0x60/0x98
> > > > [ 2479.386540]I[0:launcher-loader: 1719]  bio_put+0x50/0x21c
> > > > [ 2479.386567]I[0:launcher-loader: 1719]  f2fs_write_end_io+0x4ac/0x4d0
> > > > [ 2479.386594]I[0:launcher-loader: 1719]  bio_endio+0x2dc/0x300
> > > > [ 2479.386622]I[0:launcher-loader: 1719]  __dm_io_complete+0x324/0x37c
> > > > [ 2479.386650]I[0:launcher-loader: 1719]  dm_io_dec_pending+0x60/0xa4
> > > > [ 2479.386676]I[0:launcher-loader: 1719]  clone_endio+0xf8/0x2f0
> > > > [ 2479.386700]I[0:launcher-loader: 1719]  bio_endio+0x2dc/0x300
> > > > [ 2479.386727]I[0:launcher-loader: 1719]  blk_update_request+0x258/0x63c
> > > > [ 2479.386754]I[0:launcher-loader: 1719]  scsi_end_request+0x50/0x304
> > > > [ 2479.386782]I[0:launcher-loader: 1719]  scsi_io_completion+0x88/0x160
> > > > [ 2479.386808]I[0:launcher-loader: 1719]  scsi_finish_command+0x17c/0x194
> > > > [ 2479.386833]I[0:launcher-loader: 1719]  scsi_complete+0xcc/0x158
> > > > [ 2479.386859]I[0:launcher-loader: 1719]  blk_mq_complete_request+0x4c/0x5c
> > > > [ 2479.386885]I[0:launcher-loader: 1719]  scsi_done_internal+0xf4/0x1e0
> > > > [ 2479.386910]I[0:launcher-loader: 1719]  scsi_done+0x14/0x20
> > > > [ 2479.386935]I[0:launcher-loader: 1719]  ufshcd_compl_one_cqe+0x578/0x71c
> > > > [ 2479.386963]I[0:launcher-loader: 1719]  ufshcd_mcq_poll_cqe_nolock+0xc8/0x150
> > > > [ 2479.386991]I[0:launcher-loader: 1719]  ufshcd_intr+0x868/0xc0c
> > > > [ 2479.387017]I[0:launcher-loader: 1719]  __handle_irq_event_percpu+0xd0/0x348
> > > > [ 2479.387044]I[0:launcher-loader: 1719]  handle_irq_event_percpu+0x24/0x74
> > > > [ 2479.387068]I[0:launcher-loader: 1719]  handle_irq_event+0x74/0xe0
> > > > [ 2479.387091]I[0:launcher-loader: 1719]  handle_fasteoi_irq+0x174/0x240
> > > > [ 2479.387118]I[0:launcher-loader: 1719]  handle_irq_desc+0x7c/0x2c0
> > > > [ 2479.387147]I[0:launcher-loader: 1719]  generic_handle_domain_irq+0x1c/0x28
> > > > [ 2479.387174]I[0:launcher-loader: 1719]  gic_handle_irq+0x64/0x158
> > > > [ 2479.387204]I[0:launcher-loader: 1719]  call_on_irq_stack+0x2c/0x54
> > > > [ 2479.387231]I[0:launcher-loader: 1719]  do_interrupt_handler+0x70/0xa0
> > > > [ 2479.387258]I[0:launcher-loader: 1719]  el1_interrupt+0x34/0x68
> > > > [ 2479.387283]I[0:launcher-loader: 1719]  el1h_64_irq_handler+0x18/0x24
> > > > [ 2479.387308]I[0:launcher-loader: 1719]  el1h_64_irq+0x68/0x6c
> 
> This is where we'd cut the trace with my suggestion.
> 
> > > > [ 2479.387332]I[0:launcher-loader: 1719]  blk_attempt_bio_merge+0x8/0x170
> > > > [ 2479.387356]I[0:launcher-loader: 1719]  blk_mq_attempt_bio_merge+0x78/0x98
> > > > [ 2479.387383]I[0:launcher-loader: 1719]  blk_mq_submit_bio+0x324/0xa40
> > > > [ 2479.387409]I[0:launcher-loader: 1719]  __submit_bio+0x104/0x138
> > > > [ 2479.387436]I[0:launcher-loader: 1719]  submit_bio_noacct_nocheck+0x1d0/0x4a0
> > > > [ 2479.387462]I[0:launcher-loader: 1719]  submit_bio_noacct+0x618/0x804
> > > > [ 2479.387487]I[0:launcher-loader: 1719]  submit_bio+0x164/0x180
> > > > [ 2479.387511]I[0:launcher-loader: 1719]  f2fs_submit_read_bio+0xe4/0x1c4
> > > > [ 2479.387537]I[0:launcher-loader: 1719]  f2fs_mpage_readpages+0x888/0xa4c
> > > > [ 2479.387563]I[0:launcher-loader: 1719]  f2fs_readahead+0xd4/0x19c
> > > > [ 2479.387587]I[0:launcher-loader: 1719]  read_pages+0xb0/0x4ac
> > > > [ 2479.387614]I[0:launcher-loader: 1719]  page_cache_ra_unbounded+0x238/0x288
> > > > [ 2479.387642]I[0:launcher-loader: 1719]  do_page_cache_ra+0x60/0x6c
> > > > [ 2479.387669]I[0:launcher-loader: 1719]  page_cache_ra_order+0x318/0x364
> > > > [ 2479.387695]I[0:launcher-loader: 1719]  ondemand_readahead+0x30c/0x3d8
> > > > [ 2479.387722]I[0:launcher-loader: 1719]  page_cache_sync_ra+0xb4/0xc8
> > > > [ 2479.387749]I[0:launcher-loader: 1719]  filemap_read+0x268/0xd24
> > > > [ 2479.387777]I[0:launcher-loader: 1719]  f2fs_file_read_iter+0x1a0/0x62c
> > > > [ 2479.387806]I[0:launcher-loader: 1719]  vfs_read+0x258/0x34c
> > > > [ 2479.387831]I[0:launcher-loader: 1719]  ksys_pread64+0x8c/0xd0
> > > > [ 2479.387857]I[0:launcher-loader: 1719]  __arm64_sys_pread64+0x48/0x54
> > > > [ 2479.387881]I[0:launcher-loader: 1719]  invoke_syscall+0x58/0x158
> > > > [ 2479.387909]I[0:launcher-loader: 1719]  el0_svc_common+0xf0/0x134
> > > > [ 2479.387935]I[0:launcher-loader: 1719]  do_el0_svc+0x44/0x114
> > > > [ 2479.387961]I[0:launcher-loader: 1719]  el0_svc+0x2c/0x80
> > > > [ 2479.387985]I[0:launcher-loader: 1719]  el0t_64_sync_handler+0x48/0x114
> > > > [ 2479.388010]I[0:launcher-loader: 1719]  el0t_64_sync+0x190/0x194
> > > > [ 2479.388038]I[0:launcher-loader: 1719] Kernel panic - not syncing: kernel: panic_on_warn set ...
> 
> Thanks,
> Mark.
> 
> > > >
> > > > So let's set __exception_irq_entry with __irq_entry as a default.
> > > > Applying this patch, we can see gic_hande_irq is included in Systemp.map as below.
> > > >
> > > > * Before
> > > > ffffffc008010000 T __do_softirq
> > > > ffffffc008010000 T __irqentry_text_end
> > > > ffffffc008010000 T __irqentry_text_start
> > > > ffffffc008010000 T __softirqentry_text_start
> > > > ffffffc008010000 T _stext
> > > > ffffffc00801066c T __softirqentry_text_end
> > > > ffffffc008010670 T __entry_text_start
> > > >
> > > > * After
> > > > ffffffc008010000 T __irqentry_text_start
> > > > ffffffc008010000 T _stext
> > > > ffffffc008010000 t gic_handle_irq
> > > > ffffffc00801013c t gic_handle_irq
> > > > ffffffc008010294 T __irqentry_text_end
> > > > ffffffc008010298 T __do_softirq
> > > > ffffffc008010298 T __softirqentry_text_start
> > > > ffffffc008010904 T __softirqentry_text_end
> > > > ffffffc008010908 T __entry_text_start
> > > >
> > > > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> > > > Signed-off-by: SEO HOYOUNG <hy50.seo@samsung.com>
> > > > Change-Id: Iea7ff528be1c72cf50ab6aabafa77215ddb55eb2
> > >
> > > This change-id is meaningless upstream.
> > >
> > > > ---
> > > >  arch/arm64/include/asm/exception.h | 5 -----
> > > >  1 file changed, 5 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> > > > index 19713d0f013b..18dbb35a337f 100644
> > > > --- a/arch/arm64/include/asm/exception.h
> > > > +++ b/arch/arm64/include/asm/exception.h
> > > > @@ -8,16 +8,11 @@
> > > >  #define __ASM_EXCEPTION_H
> > > >
> > > >  #include <asm/esr.h>
> > > > -#include <asm/kprobes.h>
> > > >  #include <asm/ptrace.h>
> > > >
> > > >  #include <linux/interrupt.h>
> > > >
> > > > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > >  #define __exception_irq_entry        __irq_entry
> > > > -#else
> > > > -#define __exception_irq_entry        __kprobes
> > > > -#endif
> > >
> > > How does this affect ftrace and kprobes? The commit message never explained why
> > > this change is safe.
> > >
> > > Thanks,
> > > Mark.
> > >
> > > >
> > > >  static inline unsigned long disr_to_esr(u64 disr)
> > > >  {
> > > > --
> > > > 2.39.2
> > > >
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

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