Re: [PATCH v2] drm/atomic-helpers: Invoke end_fb_access while owning plane state

[Alyssa Ross <hi@alyssa.is>]

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

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Invoke drm_plane_helper_funcs.end_fb_access before
> drm_atomic_helper_commit_hw_done(). The latter function hands over
> ownership of the plane state to the following commit, which might
> free it. Releasing resources in end_fb_access then operates on undefined
> state. This bug has been observed with non-blocking commits when they
> are being queued up quickly.
>
> Here is an example stack trace from the bug report. The plane state has
> been free'd already, so the pages for drm_gem_fb_vunmap() are gone.
>
> Unable to handle kernel paging request at virtual address 0000000100000049
> [...]
>  drm_gem_fb_vunmap+0x18/0x74
>  drm_gem_end_shadow_fb_access+0x1c/0x2c
>  drm_atomic_helper_cleanup_planes+0x58/0xd8
>  drm_atomic_helper_commit_tail+0x90/0xa0
>  commit_tail+0x15c/0x188
>  commit_work+0x14/0x20
>
> For aborted commits, it is still ok to run end_fb_access as part of the
> plane's cleanup. Add a test to drm_atomic_helper_cleanup_planes().
>
> v2:
> 	* fix test in drm_atomic_helper_cleanup_planes()
>
> Reported-by: Alyssa Ross <hi@alyssa.is>
> Closes: https://lore.kernel.org/dri-devel/87leazm0ya.fsf@alyssa.is/
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Fixes: 94d879eaf7fb ("drm/atomic-helper: Add {begin,end}_fb_access to plane helpers")
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: <stable@vger.kernel.org> # v6.2+
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Got this basically immediately. :(

simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_init] Allocated atomic state 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_plane_state] Added [PLANE:31:plane-0] 000000004935bdca state to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_crtc_state] Added [CRTC:33:crtc-0] 00000000d25f613d state to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_set_fb_for_plane] Set [FB:38] for [PLANE:31:plane-0] state 000000004935bdca
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:35:Unknown-1] 0000000020d19f10 state to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_check_only] checking 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] Updating routing for [CONNECTOR:35:Unknown-1]
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] [CONNECTOR:35:Unknown-1] keeps [ENCODER:34:None-34], now on [CRTC:33:crtc-0]
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_nonblocking_commit] committing 00000000cfb3f1f2 nonblocking
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_default_clear] Clearing atomic state 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:__drm_atomic_state_free] Freeing atomic state 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_init] Allocated atomic state 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_plane_state] Added [PLANE:31:plane-0] 0000000083f22dc6 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_crtc_state] Added [CRTC:33:crtc-0] 00000000eec339c5 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_set_fb_for_plane] Set [FB:37] for [PLANE:31:plane-0] state 0000000083f22dc6
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:35:Unknown-1] 0000000022495ce9 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_check_only] checking 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] Updating routing for [CONNECTOR:35:Unknown-1]
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] [CONNECTOR:35:Unknown-1] keeps [ENCODER:34:None-34], now on [CRTC:33:crtc-0]
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_default_clear] Clearing atomic state 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:__drm_atomic_state_free] Freeing atomic state 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_init] Allocated atomic state 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_plane_state] Added [PLANE:31:plane-0] 0000000083f22dc6 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_crtc_state] Added [CRTC:33:crtc-0] 00000000eec339c5 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_set_fb_for_plane] Set [FB:37] for [PLANE:31:plane-0] state 0000000083f22dc6
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:35:Unknown-1] 0000000022495ce9 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_check_only] checking 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] Updating routing for [CONNECTOR:35:Unknown-1]
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] [CONNECTOR:35:Unknown-1] keeps [ENCODER:34:None-34], now on [CRTC:33:crtc-0]
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_nonblocking_commit] committing 0000000003dc0c0b nonblocking
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_default_clear] Clearing atomic state 000000000a78a23c
simple-framebuffer dd53a4000.framebuffer: [drm:__drm_atomic_state_free] Freeing atomic state 000000000a78a23c
Unable to handle kernel paging request at virtual address ffff80009033c000
Mem abort info:
  ESR = 0x0000000096000007
  EC = 0x25: DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
  FSC = 0x07: level 3 translation fault
Data abort info:
  ISV = 0, ISS = 0x00000007, ISS2 = 0x00000000
  CM = 0, WnR = 0, TnD = 0, TagAccess = 0
  GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
swapper pgtable: 16k pages, 48-bit VAs, pgdp=0000000dc5c44000
[ffff80009033c000] pgd=1000000dce9a0003, p4d=1000000dce9a0003, pud=1000000dce99c003, pmd=10000008105c8003, pte=0000000000000000
Internal error: Oops: 0000000096000007 [#1] PREEMPT SMP
Modules linked in: rfcomm snd_seq_dummy snd_hrtimer snd_seq snd_seq_device bnep des_generic libdes md4 brcmfmac_wcc joydev brcmfmac hci_bcm4377 brcmutil bluetooth ecdh_generic hid_magicmouse cfg80211 ecc rfkill snd_soc_macaudio macsmc_power macsmc_reboot macsmc_hid xt_conntrack apple_isp videobuf2_dma_sg videobuf2_memops videobuf2_v4l2 nf_conntrack snd_soc_cs42l84 nf_defrag_ipv6 videodev nf_defrag_ipv4 videobuf2_common clk_apple_nco ofpart snd_soc_tas2764 spi_nor snd_soc_apple_mca mc apple_admac pwm_apple apple_soc_cpufreq leds_pwm ip6t_rpfilter hid_apple ipt_rpfilter xt_pkttype xt_LOG nf_log_syslog nft_compat nf_tables nfnetlink loop tun tap macvlan bridge stp llc fuse zstd zram dm_crypt xhci_plat_hcd xhci_hcd nvmem_spmi_mfd rtc_macsmc gpio_macsmc pcie_apple simple_mfd_spmi tps6598x dockchannel_hid regmap_spmi dwc3 phy_apple_atc pci_host_common udc_core typec nvme_apple macsmc_rtkit apple_sart apple_rtkit_helper apple_dockchannel macsmc apple_rtkit mfd_core spmi_apple_controller nvmem_apple_efuses
 pinctrl_apple_gpio spi_apple i2c_apple apple_dart apple_mailbox btrfs xor xor_neon raid6_pq
CPU: 2 PID: 507 Comm: kworker/u16:10 Tainted: G S                 6.5.0-asahi #1-NixOS
Hardware name: Apple MacBook Pro (13-inch, M2, 2022) (DT)
Workqueue: events_unbound commit_work
pstate: 21400009 (nzCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
pc : __memcpy+0x15c/0x230
lr : __drm_fb_xfrm_toio.isra.0+0xcc/0x15c
sp : ffff800082bf3b90
x29: ffff800082bf3b90 x28: ffff8000807773f4 x27: ffff80009033a800
x26: 0000000000000012 x25: 0000000000000a00 x24: 0000000000002800
x23: ffff000035604700 x22: 0000000000000640 x21: ffff000128070000
x20: ffff000128072800 x19: ffff80008402a800 x18: ffffffffffffffff
x17: 746174735f63696d x16: 6f74615f6d72645f x15: ff090f19ff090f19
x14: 0000000000000000 x13: ff0a1320ff0a1320 x12: ff0a1320ff0b1321
x11: ff0a1320ff0b1321 x10: ff0b1321ff0b1321 x9 : ff0b1321ff0a1320
x8 : ff0a1320ff0a1320 x7 : ff0a1320ff0a1320 x6 : ff0a1320ff0a1320
x5 : ffff000128075000 x4 : ffff80009033d000 x3 : ffff000128073fc0
x2 : 0000000000000ff0 x1 : ffff80009033bfc0 x0 : ffff000128072800
Call trace:
 __memcpy+0x15c/0x230
 drm_fb_xfrm.isra.0+0x44/0x60
 drm_fb_blit+0x234/0x2ec
 simpledrm_primary_plane_helper_atomic_update+0x12c/0x164
 drm_atomic_helper_commit_planes+0xe4/0x2d0
 drm_atomic_helper_commit_tail+0x54/0xa0
 commit_tail+0x15c/0x188
 commit_work+0x14/0x20
 process_one_work+0x1e0/0x344
 worker_thread+0x68/0x424
 kthread+0xf4/0x100
 ret_from_fork+0x10/0x20
Code: a9422428 a9032c6a a9432c2a a984346c (a9c4342c) 
---[ end trace 0000000000000000 ]---

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]