Hi Rebecca,
I found a bug in nvmap's carveout tracking, but I'm not sure what the right fix
is.
The bug is a deref of a poisoned pointer (really, access to a freed struct
nvmap_client) that manifests itself in the following sequence of events:
1. Userspace nvmap client A opens /dev/nvmap
2. Client A creates handle H
Internally, this creates handle H and a client ref R. H's owner field is
to a pointer to client A's struct nvmap_client.
3. Client A allocates memory for handle H, and gets unique ID I for it
5. Client A opens /dev/fb0, sets fb0's nvmap handle to its own
6. Client A tells fb0 (dc driver) to flip to buffID I
At this point, the dc driver will create its own client ref R' to handle H,
which bumps H's refcount to make sure that the underlying memory doesn't go
away.
7. Client A's process dies.
destroy_client() removes client ref R, but client ref R' remains so handle
H also remains; destroy_client() also deletes the carveout_commit list and
frees the nvmap_client struct for client A
8. Client B repeats the same or similar process as A above, resulting in a flip
in the dc driver to a new surface
When the dc driver releases its ref R' on the old handle H, handle H is
freed. _nvmap_handle_free() calls nvmap_carveout_commit_subtract(), and
specifies a pointer to the old (freed) nvmap_client struct that it pulls
out of H's owner field:
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 5]
0xc02739ec in __list_del (client=0xc4de2d20, node=0xd792ddac, len=16384) at include/linux/list.h:89
89 next->prev = prev;
(gdb) bt
#0 0xc02739ec in __list_del (client=0xc4de2d20, node=0xd792ddac, len=16384) at include/linux/list.h:89
#1 list_del_init (client=0xc4de2d20, node=0xd792ddac, len=16384) at include/linux/list.h:139
#2 nvmap_carveout_commit_subtract (client=0xc4de2d20, node=0xd792ddac, len=16384)
at drivers/video/tegra/nvmap/nvmap_dev.c:307
#3 0xc02766b0 in _nvmap_handle_free (h=0xc4de2c00) at drivers/video/tegra/nvmap/nvmap_handle.c:85
#4 0xc0277830 in nvmap_handle_put (client=0xd7a7dae0, id=3302894592) at drivers/video/tegra/nvmap/nvmap.h:230
#5 nvmap_free_handle_id (client=0xd7a7dae0, id=3302894592) at drivers/video/tegra/nvmap/nvmap_handle.c:383
#6 0xc0272bf4 in nvmap_free (client=0xd7a7dae0, r=0xc6a6bc80) at drivers/video/tegra/nvmap/nvmap.c:716
#7 0xc026e940 in tegra_fb_flip_worker (work=<value optimized out>) at drivers/video/tegra/fb.c:450
#8 0xc00934b0 in process_one_work (worker=0xd7804380, work=0xc69d1a00) at kernel/workqueue.c:1822
#9 0xc0093aa8 in worker_thread (__worker=<value optimized out>) at kernel/workqueue.c:1933
#10 0xc0099930 in kthread (_create=0xd783dec8) at kernel/kthread.c:95
#11 0xc003d878 in kernel_thread_helper ()
#12 0xc003d878 in kernel_thread_helper ()
The trivial fix for this particular crash is to clear the handle's owner field
when that client no longer references that handle (I'll send that patch out
shortly).
However, the notion of carveout_commit being per-client (and, more generally, a
handle having an "owner") seems wrong. Rather, clients hold references to
handles, and handles may reserve carveout. Rebecca, it looks like you added
the carveout_commit tracking in commit 179ad421, and as far as I can tell, it's
only used for debugfs printouts. Is there some reason
nvmap_debug_allocations_show() and nvmap_debug_clients_show() couldn't be
implemented by walking the client's handle_refs tree and inspecting the handles
themselves? This should prevent "hidden" carveout being consumed by duped
handles whose "owner" has been destroyed.
Thanks,
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
