From 9b59029875a7e4d362834fe2d6017b74a044b2e6 Mon Sep 17 00:00:00 2001 From: David Smith Date: Thu, 28 May 2009 10:58:17 -0500 Subject: [PATCH] Avoid 1 case of holding a semaphore while mmap callbacks are being made. * runtime/task_finder.c (__stp_call_mmap_callbacks_with_addr): Renamed from __stp_call_mmap_callbacks_with_vma(). Also added some code from __stp_utrace_task_finder_target_syscall_exit() that locks the 'mmap_sem' semaphore. This avoids holding the semaphore while the mmap callbacks are made. (__stp_utrace_task_finder_target_syscall_exit): Just calls __stp_call_mmap_callbacks_with_addr() in the mmap case. --- runtime/task_finder.c | 132 ++++++++++++++++++++++++------------------ 1 file changed, 75 insertions(+), 57 deletions(-) diff --git a/runtime/task_finder.c b/runtime/task_finder.c index f5e059caa..b35192a20 100644 --- a/runtime/task_finder.c +++ b/runtime/task_finder.c @@ -580,43 +580,90 @@ __stp_call_mmap_callbacks(struct stap_task_finder_target *tgt, } } + +static struct vm_area_struct * +__stp_find_file_based_vma(struct mm_struct *mm, unsigned long addr) +{ + struct vm_area_struct *vma = find_vma(mm, addr); + + // I'm not positive why the checking for vm_start > addr is + // necessary, but it seems to be (sometimes find_vma() returns + // a vma that addr doesn't belong to). + if (vma && (vma->vm_file == NULL || vma->vm_start > addr)) + vma = NULL; + return vma; +} + + static void -__stp_call_mmap_callbacks_with_vma(struct stap_task_finder_target *tgt, - struct task_struct *tsk, - struct vm_area_struct *vma) +__stp_call_mmap_callbacks_with_addr(struct stap_task_finder_target *tgt, + struct task_struct *tsk, + unsigned long addr) { - char *mmpath_buf; - char *mmpath; - int rc; + struct mm_struct *mm; + struct vm_area_struct *vma; + char *mmpath_buf = NULL; + char *mmpath = NULL; + long err; + unsigned long length; + unsigned long offset; + unsigned long vm_flags; - // Allocate space for a path - mmpath_buf = _stp_kmalloc(PATH_MAX); - if (mmpath_buf == NULL) { - _stp_error("Unable to allocate space for path"); + mm = get_task_mm(tsk); + if (! mm) return; - } - // Grab the path associated with this vma. + down_read(&mm->mmap_sem); + vma = __stp_find_file_based_vma(mm, addr); + if (vma) { + // Cache information we need from the vma + addr = vma->vm_start; + length = vma->vm_end - vma->vm_start; + offset = (vma->vm_pgoff << PAGE_SHIFT); + vm_flags = vma->vm_flags; + + // Allocate space for a path + mmpath_buf = _stp_kmalloc(PATH_MAX); + if (mmpath_buf == NULL) { + _stp_error("Unable to allocate space for path"); + } + else { + // Grab the path associated with this vma. #ifdef STAPCONF_DPATH_PATH - mmpath = d_path(&(vma->vm_file->f_path), mmpath_buf, PATH_MAX); + mmpath = d_path(&(vma->vm_file->f_path), mmpath_buf, + PATH_MAX); #else - mmpath = d_path(vma->vm_file->f_dentry, vma->vm_file->f_vfsmnt, - mmpath_buf, PATH_MAX); + mmpath = d_path(vma->vm_file->f_dentry, + vma->vm_file->f_vfsmnt, mmpath_buf, + PATH_MAX); #endif - if (mmpath == NULL || IS_ERR(mmpath)) { - rc = -PTR_ERR(mmpath); - _stp_error("Unable to get path (error %d) for pid %d", - rc, (int)tsk->pid); - } - else { - __stp_call_mmap_callbacks(tgt, tsk, mmpath, vma->vm_start, - vma->vm_end - vma->vm_start, - (vma->vm_pgoff << PAGE_SHIFT), - vma->vm_flags); + if (mmpath == NULL || IS_ERR(mmpath)) { + long err = ((mmpath == NULL) ? 0 + : -PTR_ERR(mmpath)); + _stp_error("Unable to get path (error %ld) for pid %d", + err, (int)tsk->pid); + mmpath = NULL; + } + } } - _stp_kfree(mmpath_buf); + + // At this point, we're done with the vma (assuming we found + // one). We can't hold the 'mmap_sem' semaphore while making + // callbacks. + up_read(&mm->mmap_sem); + + if (mmpath) + __stp_call_mmap_callbacks(tgt, tsk, mmpath, addr, + length, offset, vm_flags); + + // Cleanup. + if (mmpath_buf) + _stp_kfree(mmpath_buf); + mmput(mm); + return; } + static inline void __stp_call_munmap_callbacks(struct stap_task_finder_target *tgt, struct task_struct *tsk, unsigned long addr, @@ -1055,19 +1102,6 @@ utftq_out: } -static struct vm_area_struct * -__stp_find_file_based_vma(struct mm_struct *mm, unsigned long addr) -{ - struct vm_area_struct *vma = find_vma(mm, addr); - - // I'm not positive why the checking for vm_start > addr is - // necessary, but it seems to be (sometimes find_vma() returns - // a vma that addr doesn't belong to). - if (vma && (vma->vm_file == NULL || vma->vm_start > addr)) - vma = NULL; - return vma; -} - #ifdef UTRACE_ORIG_VERSION static u32 __stp_utrace_task_finder_target_syscall_entry(struct utrace_attached_engine *engine, @@ -1194,24 +1228,8 @@ __stp_utrace_task_finder_target_syscall_exit(enum utrace_resume_action action, } else if (entry->syscall_no == MMAP_SYSCALL_NO(tsk) || entry->syscall_no == MMAP2_SYSCALL_NO(tsk)) { - struct mm_struct *mm; - - mm = get_task_mm(tsk); - if (mm) { - struct vm_area_struct *vma; - - down_read(&mm->mmap_sem); - vma = __stp_find_file_based_vma(mm, rv); - - // Call the callbacks - if (vma) { - __stp_call_mmap_callbacks_with_vma(tgt, tsk, - vma); - } - - up_read(&mm->mmap_sem); - mmput(mm); - } + // Call the callbacks + __stp_call_mmap_callbacks_with_addr(tgt, tsk, rv); } else { // mprotect // Call the callbacks -- 2.43.5