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: 6935 bytes --]

On Tue, Apr 25, 2023 at 02:39:51PM +0100, Mark Rutland wrote:
> On Tue, Apr 25, 2023 at 11:31:31AM +0900, Youngmin Nam wrote:
> > 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.
> 
> I'd meant something like the below, marking the assembly (as x86 does) rather
> than the C code. I'll try to sort that out and send a proper patch series after
> -rc1.
> 
> Thanks,
> Mark.
> 
After applying your draft patch,
I checked System.map and could see irq entries we expected were included as below.

ffffffc008000000 T _text
ffffffc008010000 T __irqentry_text_start
ffffffc008010000 T _stext
ffffffc008010000 t el1t_64_irq
ffffffc00801006c t el1t_64_fiq
ffffffc0080100d8 t el1h_64_irq
ffffffc008010144 t el1h_64_fiq
ffffffc0080101b0 t el0t_64_irq
ffffffc008010344 t el0t_64_fiq
ffffffc0080104d8 t el0t_32_irq
ffffffc008010670 t el0t_32_fiq
ffffffc008010928 T __do_softirq
ffffffc008010928 T __irqentry_text_end
ffffffc008010928 T __softirqentry_text_start
ffffffc008010fa0 T __entry_text_start
ffffffc008010fa0 T __softirqentry_text_end

And then, I confirmed callstack was cut correctly as below.

[   89.738326]I[5:NetworkWatchlis: 1084]  kasan_save_stack+0x40/0x70
[   89.738337]I[5:NetworkWatchlis: 1084]  save_stack_info+0x34/0x138
[   89.738348]I[5:NetworkWatchlis: 1084]  kasan_save_free_info+0x18/0x24
[   89.738358]I[5:NetworkWatchlis: 1084]  ____kasan_slab_free+0x16c/0x170
[   89.738369]I[5:NetworkWatchlis: 1084]  __kasan_slab_free+0x10/0x20
[   89.738379]I[5:NetworkWatchlis: 1084]  kmem_cache_free+0x238/0x53c
[   89.738388]I[5:NetworkWatchlis: 1084]  mempool_free_slab+0x1c/0x28
[   89.738397]I[5:NetworkWatchlis: 1084]  mempool_free+0x7c/0x1a0
[   89.738405]I[5:NetworkWatchlis: 1084]  bvec_free+0x34/0x80
[   89.738417]I[5:NetworkWatchlis: 1084]  bio_free+0x60/0x98
[   89.738426]I[5:NetworkWatchlis: 1084]  bio_put+0x50/0x21c
[   89.738434]I[5:NetworkWatchlis: 1084]  f2fs_write_end_io+0x4ac/0x4d0
[   89.738444]I[5:NetworkWatchlis: 1084]  bio_endio+0x2dc/0x300
[   89.738453]I[5:NetworkWatchlis: 1084]  __dm_io_complete+0x324/0x37c
[   89.738464]I[5:NetworkWatchlis: 1084]  dm_io_dec_pending+0x60/0xa4
[   89.738474]I[5:NetworkWatchlis: 1084]  clone_endio+0xf8/0x2f0
[   89.738484]I[5:NetworkWatchlis: 1084]  bio_endio+0x2dc/0x300
[   89.738493]I[5:NetworkWatchlis: 1084]  blk_update_request+0x258/0x63c
[   89.738503]I[5:NetworkWatchlis: 1084]  scsi_end_request+0x50/0x304
[   89.738514]I[5:NetworkWatchlis: 1084]  scsi_io_completion+0x88/0x160
[   89.738524]I[5:NetworkWatchlis: 1084]  scsi_finish_command+0x17c/0x194
[   89.738534]I[5:NetworkWatchlis: 1084]  scsi_complete+0xcc/0x158
[   89.738543]I[5:NetworkWatchlis: 1084]  blk_mq_complete_request+0x4c/0x5c
[   89.738553]I[5:NetworkWatchlis: 1084]  scsi_done_internal+0xf4/0x1e0
[   89.738564]I[5:NetworkWatchlis: 1084]  scsi_done+0x14/0x20
[   89.738575]I[5:NetworkWatchlis: 1084]  ufshcd_compl_one_cqe+0x578/0x71c
[   89.738585]I[5:NetworkWatchlis: 1084]  ufshcd_mcq_poll_cqe_nolock+0xc8/0x150
[   89.738594]I[5:NetworkWatchlis: 1084]  exynos_vendor_mcq_irq+0xac/0xc4 [ufs_exynos_core]
[   89.738638]I[5:NetworkWatchlis: 1084]  __handle_irq_event_percpu+0xd0/0x348
[   89.738647]I[5:NetworkWatchlis: 1084]  handle_irq_event_percpu+0x24/0x74
[   89.738656]I[5:NetworkWatchlis: 1084]  handle_irq_event+0x74/0xe0
[   89.738665]I[5:NetworkWatchlis: 1084]  handle_fasteoi_irq+0x174/0x240
[   89.738675]I[5:NetworkWatchlis: 1084]  handle_irq_desc+0x6c/0x2c0
[   89.738686]I[5:NetworkWatchlis: 1084]  generic_handle_domain_irq+0x1c/0x28
[   89.738697]I[5:NetworkWatchlis: 1084]  gic_handle_irq+0x64/0x154
[   89.738707]I[5:NetworkWatchlis: 1084]  call_on_irq_stack+0x2c/0x54
[   89.738717]I[5:NetworkWatchlis: 1084]  do_interrupt_handler+0x70/0xa0
[   89.738726]I[5:NetworkWatchlis: 1084]  el1_interrupt+0x34/0x68
[   89.738737]I[5:NetworkWatchlis: 1084]  el1h_64_irq_handler+0x18/0x24
[   89.738747]I[5:NetworkWatchlis: 1084]  el1h_64_irq+0x68/0x6c

Thanks for your work.
Please add me when you send the final patch so that I can test again.

> ---->8----

[-- 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