I have some revisions to the commit tracking that fixes the problem you described above. In short it was a mistake to track the allocations based on when the handles are added and removed, they should have been tracked based on when handle ref's were added and removed from clients. This avoids the problem you described above as a client can't go away while it's holding some number of handle_refs. The owner field is no longer needed and I agree shouldn't exist, the only other thing it's used for is permissions tracking that could be better handled another way. On Wed, Dec 1, 2010 at 1:25 PM, <rmorell@xxxxxxxxxx> wrote: > 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. The short answer is that the functionality was added to support the case where the system has run out of carveout and would like to start reclaiming it by killing applications. For this to work well, you need the ability to determine how much carveout is in use by each process -- a rough mapping to client. It might be possible to just walk the allocations lists at that point, but it will be somewhat more expensive. > > > Thanks, > Robert > Rebecca -- 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
