Re: [PATCH] drm/msm: Fix WARN_ON() splat in _free_object()

[Iskren Chernev]

On 12/10/20 7:40 PM, Rob Clark wrote:
> From: Rob Clark <robdclark@xxxxxxxxxxxx>
>
> [  192.062000] ------------[ cut here ]------------
> [  192.062498] WARNING: CPU: 3 PID: 2039 at 
drivers/gpu/drm/msm/msm_gem.c:381 put_iova_vmas+0x94/0xa0 [msm]
> [  192.062870] Modules linked in: snd_hrtimer snd_seq snd_seq_device 
rfcomm algif_hash algif_skcipher af_alg bnep xt_CHECKSUM nft_chain_nat 
xt_MASQUERADE nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 
nft_counter xt_tcpudp nft_compat cpufreq_powersave cpufreq_conservative 
q6asm_dai q6routing q6afe_dai q6adm bridge q6afe q6asm q6dsp_common 
q6core stp llc nf_tables libcrc32c nfnetlink snd_soc_wsa881x regmap_sdw 
soundwire_qcom gpio_wcd934x snd_soc_wcd934x wcd934x regmap_slimbus 
venus_enc venus_dec apr videobuf2_dma_sg qrtr_smd uvcvideo 
videobuf2_vmalloc videobuf2_memops ath10k_snoc ath10k_core hci_uart 
btqca btbcm mac80211 bluetooth snd_soc_sdm845 ath snd_soc_rt5663 
snd_soc_qcom_common snd_soc_rl6231 soundwire_bus ecdh_generic ecc 
qcom_spmi_adc5 venus_core qcom_pon qcom_spmi_temp_alarm qcom_vadc_common 
v4l2_mem2mem videobuf2_v4l2 cfg80211 videobuf2_common hid_multitouch 
reset_qcom_pdc qcrypto qcom_rng rfkill qcom_q6v5_mss libarc4 libdes qrtr 
ns qcom_wdt socinfo slim_qcom_ngd_ctrl
> [  192.065739]  pdr_interface qcom_q6v5_pas slimbus qcom_pil_info 
qcom_q6v5 qcom_sysmon qcom_common qcom_glink_smem qmi_helpers rmtfs_mem 
tcp_bbr sch_fq fuse ip_tables x_tables ipv6 crc_ccitt ti_sn65dsi86 
i2c_hid msm mdt_loader llcc_qcom rtc_pm8xxx ocmem drm_kms_helper 
crct10dif_ce phy_qcom_qusb2 i2c_qcom_geni panel_simple drm pwm_bl
> [  192.066066] CPU: 3 PID: 2039 Comm: gnome-shell Tainted: G        
W         5.10.0-rc7-next-20201208 #1
> [  192.066068] Hardware name: LENOVO 81JL/LNVNB161216, BIOS 
9UCN33WW(V2.06) 06/ 4/2019
> [  192.066072] pstate: 40400005 (nZcv daif +PAN -UAO -TCO BTYPE=--)
> [  192.066099] pc : put_iova_vmas+0x94/0xa0 [msm]
> [  192.066262] lr : put_iova_vmas+0x1c/0xa0 [msm]
> [  192.066403] sp : ffff800019efbbb0
> [  192.066405] x29: ffff800019efbbb0 x28: ffff800019efbd88
> [  192.066411] x27: 0000000000000000 x26: ffff109582efa400
> [  192.066417] x25: 0000000000000009 x24: 000000000000012b
> [  192.066422] x23: ffff109582efa438 x22: ffff109582efa450
> [  192.066427] x21: ffff109582efa528 x20: ffff1095cbd4f200
> [  192.066432] x19: ffff1095cbd4f200 x18: 0000000000000000
> [  192.066438] x17: 0000000000000000 x16: ffffc26c200ca750
> [  192.066727] x15: 0000000000000000 x14: 0000000000000000
> [  192.066741] x13: ffff1096fb8c9100 x12: 0000000000000002
> [  192.066754] x11: ffffffffffffffff x10: 0000000000000002
> [  192.067046] x9 : 0000000000000001 x8 : 0000000000000a36
> [  192.067060] x7 : ffff4e2ad9f11000 x6 : ffffc26c216d4000
> [  192.067212] x5 : ffffc26c2022661c x4 : ffff1095c2b98000
> [  192.067367] x3 : ffff1095cbd4f300 x2 : 0000000000000000
> [  192.067380] x1 : ffff1095c2b98000 x0 : 0000000000000000
> [  192.067667] Call trace:
> [  192.067734]  put_iova_vmas+0x94/0xa0 [msm]
> [  192.068078]  msm_gem_free_object+0xb4/0x110 [msm]
> [  192.068399]  drm_gem_object_free+0x1c/0x30 [drm]
> [  192.068717]  drm_gem_object_handle_put_unlocked+0xf0/0xf8 [drm]
> [  192.069032]  drm_gem_object_release_handle+0x6c/0x88 [drm]
> [  192.069349]  drm_gem_handle_delete+0x68/0xc0 [drm]
> [  192.069666]  drm_gem_close_ioctl+0x30/0x48 [drm]
> [  192.069984]  drm_ioctl_kernel+0xc0/0x110 [drm]
> [  192.070303]  drm_ioctl+0x210/0x440 [drm]
> [  192.070588]  __arm64_sys_ioctl+0xa8/0xf0
> [  192.070599]  el0_svc_common.constprop.0+0x74/0x190
> [  192.070608]  do_el0_svc+0x24/0x90
> [  192.070618]  el0_svc+0x14/0x20
> [  192.070903]  el0_sync_handler+0xb0/0xb8
> [  192.070911]  el0_sync+0x174/0x180
> [  192.070918] ---[ end trace bee6b12a899001a3 ]---
> [  192.072140] ------------[ cut here ]------------
>
> Fixes: 9b73bde39cf2 ("drm/msm: Fix use-after-free in msm_gem with 
carveout")
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/msm_gem.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c 
b/drivers/gpu/drm/msm/msm_gem.c
> index 68a6c7eacc0a..a21be5b910ff 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -990,6 +990,8 @@ void msm_gem_free_object(struct drm_gem_object *obj)
>          if (msm_obj->pages)
>              kvfree(msm_obj->pages);
>
> +        put_iova_vmas(obj);
> +
>          /* dma_buf_detach() grabs resv lock, so we need to unlock
>           * prior to drm_prime_gem_destroy
>           */
> @@ -999,11 +1001,10 @@ void msm_gem_free_object(struct drm_gem_object 
*obj)
>      } else {
>          msm_gem_vunmap(obj);
>          put_pages(obj);
> +        put_iova_vmas(obj);
>          msm_gem_unlock(obj);
>      }
>
> -    put_iova_vmas(obj);
> -
>      drm_gem_object_release(obj);
>
>      kfree(msm_obj);

Ah, the put_iova_vmas needs to happen inside the msm_gem_lock|unlock.
My bad!

Acked-by: Iskren Chernev <iskren.chernev@xxxxxxxxx>