Skip to content

Commit 2ad62d1

Browse files
mairacanalmairacanal
committed
drm/v3d: Free the job and assign it to NULL if initialization fails
Currently, if `v3d_job_init()` fails (e.g. in the IGT test "bad-in-sync", where we submit an invalid in-sync to the IOCTL), then we end up with the following NULL pointer dereference: [ 34.146279] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000078 [ 34.146301] Mem abort info: [ 34.146306] ESR = 0x0000000096000005 [ 34.146315] EC = 0x25: DABT (current EL), IL = 32 bits [ 34.146322] SET = 0, FnV = 0 [ 34.146328] EA = 0, S1PTW = 0 [ 34.146334] FSC = 0x05: level 1 translation fault [ 34.146340] Data abort info: [ 34.146345] ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000 [ 34.146351] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 34.146357] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 34.146366] user pgtable: 4k pages, 39-bit VAs, pgdp=00000001232e6000 [ 34.146375] [0000000000000078] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000 [ 34.146399] Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP [ 34.146406] Modules linked in: rfcomm snd_seq_dummy snd_hrtimer snd_seq snd_seq_device algif_hash aes_neon_bs aes_neon_blk algif_skcipher af_alg bnep hid_logitech_hidpp brcmfmac_wcc brcmfmac brcmutil hci_uart vc4 btbcm cfg80211 bluetooth bcm2835_v4l2(C) snd_soc_hdmi_codec binfmt_misc cec drm_display_helper hid_logitech_dj bcm2835_mmal_vchiq(C) drm_dma_helper drm_kms_helper videobuf2_v4l2 raspberrypi_hwmon ecdh_generic videobuf2_vmalloc videobuf2_memops ecc videobuf2_common rfkill videodev libaes snd_soc_core dwc2 i2c_brcmstb snd_pcm_dmaengine snd_bcm2835(C) i2c_bcm2835 pwm_bcm2835 snd_pcm mc v3d snd_timer snd gpu_sched drm_shmem_helper nvmem_rmem uio_pdrv_genirq uio i2c_dev drm fuse dm_mod drm_panel_orientation_quirks backlight configfs ip_tables x_tables ipv6 [ 34.146556] CPU: 1 PID: 1890 Comm: v3d_submit_csd Tainted: G C 6.7.0-rc3-g49ddab089611 torvalds#68 [ 34.146563] Hardware name: Raspberry Pi 4 Model B Rev 1.5 (DT) [ 34.146569] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 34.146575] pc : drm_sched_job_cleanup+0x3c/0x190 [gpu_sched] [ 34.146611] lr : v3d_submit_csd_ioctl+0x1b4/0x460 [v3d] [ 34.146653] sp : ffffffc083cbbb80 [ 34.146658] x29: ffffffc083cbbb90 x28: ffffff81035afc00 x27: ffffffe77a641168 [ 34.146668] x26: ffffff81056a8000 x25: 0000000000000058 x24: 0000000000000000 [ 34.146677] x23: ffffff81065e2000 x22: ffffff81035afe00 x21: ffffffc083cbbcf0 [ 34.146686] x20: ffffff81035afe00 x19: 00000000ffffffea x18: 0000000000000000 [ 34.146694] x17: 0000000000000000 x16: ffffffe7989e34b0 x15: 0000000000000000 [ 34.146703] x14: 0000000004000004 x13: ffffff81035afe80 x12: ffffffc083cb8000 [ 34.146711] x11: cc57e05dfbe5ef00 x10: cc57e05dfbe5ef00 x9 : ffffffe77a64131c [ 34.146719] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f [ 34.146727] x5 : 0000000000000040 x4 : ffffff81fefb03f0 x3 : ffffffc083cbba40 [ 34.146736] x2 : ffffff81056a8000 x1 : ffffffe7989e35e8 x0 : 0000000000000000 [ 34.146745] Call trace: [ 34.146748] drm_sched_job_cleanup+0x3c/0x190 [gpu_sched] [ 34.146768] v3d_submit_csd_ioctl+0x1b4/0x460 [v3d] [ 34.146791] drm_ioctl_kernel+0xe0/0x120 [drm] [ 34.147029] drm_ioctl+0x264/0x408 [drm] [ 34.147135] __arm64_sys_ioctl+0x9c/0xe0 [ 34.147152] invoke_syscall+0x4c/0x118 [ 34.147162] el0_svc_common+0xb8/0xf0 [ 34.147168] do_el0_svc+0x28/0x40 [ 34.147174] el0_svc+0x38/0x88 [ 34.147184] el0t_64_sync_handler+0x84/0x100 [ 34.147191] el0t_64_sync+0x190/0x198 [ 34.147201] Code: aa0003f4 f90007e8 f9401008 aa0803e0 (b8478c09) [ 34.147210] ---[ end trace 0000000000000000 ]--- This happens because we are calling `drm_sched_job_cleanup()` twice: once at `v3d_job_init()` and again when we call `v3d_job_cleanup()`. To mitigate this issue, we can return to the same approach that we used to use before 464c61e: deallocate the job after `v3d_job_init()` fails and assign it to NULL. Then, when we call `v3d_job_cleanup()`, job is NULL and the function returns. Fixes: 464c61e ("drm/v3d: Decouple job allocation from job initiation") Signed-off-by: Maíra Canal <mcanal@igalia.com> Reviewed-by: Iago Toral Quiroga <itoral@igalia.com> Link: https://patchwork.freedesktop.org/patch/msgid/20240109142857.1122704-1-mcanal@igalia.com
1 parent 89fe460 commit 2ad62d1

File tree

1 file changed

+28
-7
lines changed

1 file changed

+28
-7
lines changed

‎drivers/gpu/drm/v3d/v3d_submit.c

+28-7
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,13 @@ v3d_job_allocate(void **container, size_t size)
147147
return 0;
148148
}
149149

150+
static void
151+
v3d_job_deallocate(void **container)
152+
{
153+
kfree(*container);
154+
*container = NULL;
155+
}
156+
150157
static int
151158
v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
152159
struct v3d_job *job, void (*free)(struct kref *ref),
@@ -273,17 +280,21 @@ v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
273280

274281
ret = v3d_job_init(v3d, file_priv, &(*job)->base,
275282
v3d_job_free, args->in_sync, se, V3D_CSD);
276-
if (ret)
283+
if (ret) {
284+
v3d_job_deallocate((void *)job);
277285
return ret;
286+
}
278287

279288
ret = v3d_job_allocate((void *)clean_job, sizeof(**clean_job));
280289
if (ret)
281290
return ret;
282291

283292
ret = v3d_job_init(v3d, file_priv, *clean_job,
284293
v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
285-
if (ret)
294+
if (ret) {
295+
v3d_job_deallocate((void *)clean_job);
286296
return ret;
297+
}
287298

288299
(*job)->args = *args;
289300

@@ -860,8 +871,10 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
860871

861872
ret = v3d_job_init(v3d, file_priv, &render->base,
862873
v3d_render_job_free, args->in_sync_rcl, &se, V3D_RENDER);
863-
if (ret)
874+
if (ret) {
875+
v3d_job_deallocate((void *)&render);
864876
goto fail;
877+
}
865878

866879
render->start = args->rcl_start;
867880
render->end = args->rcl_end;
@@ -874,8 +887,10 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
874887

875888
ret = v3d_job_init(v3d, file_priv, &bin->base,
876889
v3d_job_free, args->in_sync_bcl, &se, V3D_BIN);
877-
if (ret)
890+
if (ret) {
891+
v3d_job_deallocate((void *)&bin);
878892
goto fail;
893+
}
879894

880895
bin->start = args->bcl_start;
881896
bin->end = args->bcl_end;
@@ -892,8 +907,10 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
892907

893908
ret = v3d_job_init(v3d, file_priv, clean_job,
894909
v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
895-
if (ret)
910+
if (ret) {
911+
v3d_job_deallocate((void *)&clean_job);
896912
goto fail;
913+
}
897914

898915
last_job = clean_job;
899916
} else {
@@ -1015,8 +1032,10 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
10151032

10161033
ret = v3d_job_init(v3d, file_priv, &job->base,
10171034
v3d_job_free, args->in_sync, &se, V3D_TFU);
1018-
if (ret)
1035+
if (ret) {
1036+
v3d_job_deallocate((void *)&job);
10191037
goto fail;
1038+
}
10201039

10211040
job->base.bo = kcalloc(ARRAY_SIZE(args->bo_handles),
10221041
sizeof(*job->base.bo), GFP_KERNEL);
@@ -1233,8 +1252,10 @@ v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
12331252

12341253
ret = v3d_job_init(v3d, file_priv, &cpu_job->base,
12351254
v3d_job_free, 0, &se, V3D_CPU);
1236-
if (ret)
1255+
if (ret) {
1256+
v3d_job_deallocate((void *)&cpu_job);
12371257
goto fail;
1258+
}
12381259

12391260
clean_job = cpu_job->indirect_csd.clean_job;
12401261
csd_job = cpu_job->indirect_csd.job;

0 commit comments

Comments
 (0)