From df67a3c1031963b13621ae4ff853e6f4fbd17622 Mon Sep 17 00:00:00 2001 From: Vyacheslav Cherkashin Date: Mon, 11 Apr 2016 15:19:17 +0300 Subject: [PATCH] [FIX] race condition when use sspt_proc Change-Id: I74f18ff7631c521dceebf137c19d5269c2e2339c Signed-off-by: Vyacheslav Cherkashin --- preload/preload_module.c | 6 ++- us_manager/helper.c | 28 ++++++++--- us_manager/pf/pf_group.c | 24 ++++++---- us_manager/sspt/sspt_proc.c | 114 +++++++++++++++++++++++++++----------------- us_manager/sspt/sspt_proc.h | 9 ++-- us_manager/us_manager.c | 2 +- 6 files changed, 118 insertions(+), 65 deletions(-) diff --git a/preload/preload_module.c b/preload/preload_module.c index 650e7bb..b614042 100644 --- a/preload/preload_module.c +++ b/preload/preload_module.c @@ -266,9 +266,11 @@ static bool __is_proc_mmap_mappable(struct task_struct *task) return false; r_debug_addr += r_state_offset; - proc = sspt_proc_by_task(task); - if (proc) + proc = sspt_proc_get_by_task(task); + if (proc) { proc->r_state_addr = r_debug_addr; + sspt_proc_put(proc); + } if (get_user(state, (unsigned long *)r_debug_addr)) return false; diff --git a/us_manager/helper.c b/us_manager/helper.c index 88749e2..1bdfecf 100644 --- a/us_manager/helper.c +++ b/us_manager/helper.c @@ -69,11 +69,20 @@ static int entry_handler_pf(struct kretprobe_instance *ri, struct pt_regs *regs) #endif /* CONFIG_arch */ if (data->addr) { - struct sspt_proc *proc = sspt_proc_by_task(current); + int ret = 0; + struct sspt_proc *proc; + + proc = sspt_proc_get_by_task(current); + if (proc) { + if (proc->r_state_addr == data->addr) { + /* skip ret_handler_pf() for current task */ + ret = 1; + } + + sspt_proc_put(proc); + } - if (proc && (proc->r_state_addr == data->addr)) - /* skip ret_handler_pf() for current task */ - return 1; + return ret; } return 0; @@ -165,7 +174,7 @@ static unsigned long cb_clean_child(void *data) struct task_struct *parent = current; struct sspt_proc *proc; - proc = sspt_proc_by_task(parent); + proc = sspt_proc_get_by_task(parent); if (proc) { struct task_struct *child = *(struct task_struct **)data; @@ -174,6 +183,8 @@ static unsigned long cb_clean_child(void *data) /* disarm urp for child */ swap_uretprobe_free_task(parent, child, false); + + sspt_proc_put(proc); } atomic_dec(&rm_uprobes_child_cnt); @@ -460,7 +471,7 @@ static void remove_unmap_probes(struct task_struct *task, sspt_proc_write_lock(); - proc = sspt_proc_by_task(task); + proc = sspt_proc_get_by_task(task); if (proc) { struct msg_unmap_data msg_data = { .start = umd->start, @@ -471,6 +482,8 @@ static void remove_unmap_probes(struct task_struct *task, /* send unmap region */ sspt_proc_on_each_filter(proc, msg_unmap, (void *)&msg_data); + + sspt_proc_put(proc); } sspt_proc_write_unlock(); @@ -572,7 +585,7 @@ static int ret_handler_mmap(struct kretprobe_instance *ri, if (IS_ERR_VALUE(start_addr)) return 0; - proc = sspt_proc_by_task(task); + proc = sspt_proc_get_by_task(task); if (proc == NULL) return 0; @@ -580,6 +593,7 @@ static int ret_handler_mmap(struct kretprobe_instance *ri, if (vma && check_vma(vma)) sspt_proc_on_each_filter(proc, msg_map, (void *)vma); + sspt_proc_put(proc); return 0; } diff --git a/us_manager/pf/pf_group.c b/us_manager/pf/pf_group.c index 3665b09..a8fad77 100644 --- a/us_manager/pf/pf_group.c +++ b/us_manager/pf/pf_group.c @@ -512,7 +512,7 @@ static enum pf_inst_flag pfg_check_task(struct task_struct *task) continue; if (proc == NULL) - proc = sspt_proc_by_task(task); + proc = sspt_proc_get_by_task(task); if (proc) { flag = flag == PIF_NONE ? PIF_SECOND : flag; @@ -534,6 +534,7 @@ static enum pf_inst_flag pfg_check_task(struct task_struct *task) flag = flag == PIF_FIRST ? flag : PIF_ADD_PFG; } mutex_unlock(&proc->filters.mtx); + sspt_proc_put(proc); } } pfg_list_runlock(); @@ -566,9 +567,11 @@ void check_task_and_install(struct task_struct *task) switch (flag) { case PIF_FIRST: case PIF_ADD_PFG: - proc = sspt_proc_by_task(task); - if (proc) + proc = sspt_proc_get_by_task(task); + if (proc) { first_install(task, proc); + sspt_proc_put(proc); + } break; case PIF_NONE: @@ -593,15 +596,19 @@ void call_page_fault(struct task_struct *task, unsigned long page_addr) switch (flag) { case PIF_FIRST: case PIF_ADD_PFG: - proc = sspt_proc_by_task(task); - if (proc) + proc = sspt_proc_get_by_task(task); + if (proc) { first_install(task, proc); + sspt_proc_put(proc); + } break; case PIF_SECOND: - proc = sspt_proc_by_task(task); - if (proc) + proc = sspt_proc_get_by_task(task); + if (proc) { subsequent_install(task, proc, page_addr); + sspt_proc_put(proc); + } break; case PIF_NONE: @@ -661,12 +668,13 @@ void call_mm_release(struct task_struct *task) { struct sspt_proc *proc; - proc = sspt_proc_by_task(task); + proc = sspt_proc_get_by_task(task); if (proc) { if (task->flags & PF_EXITING) mmr_from_exit(proc); else mmr_from_exec(proc); + sspt_proc_put(proc); } } diff --git a/us_manager/sspt/sspt_proc.c b/us_manager/sspt/sspt_proc.c index 0f70788..28239ac 100644 --- a/us_manager/sspt/sspt_proc.c +++ b/us_manager/sspt/sspt_proc.c @@ -83,30 +83,35 @@ void sspt_proc_write_unlock(void) write_unlock(&sspt_proc_rwlock); } +struct ktd_proc { + struct sspt_proc *proc; + spinlock_t lock; +}; static void ktd_init(struct task_struct *task, void *data) { - struct sspt_proc **pproc = (struct sspt_proc **)data; + struct ktd_proc *kproc = (struct ktd_proc *)data; - *pproc = NULL; + kproc->proc = NULL; + spin_lock_init(&kproc->lock); } static void ktd_exit(struct task_struct *task, void *data) { - struct sspt_proc **pproc = (struct sspt_proc **)data; + struct ktd_proc *kproc = (struct ktd_proc *)data; - WARN_ON(*pproc); + WARN_ON(kproc->proc); } struct ktask_data ktd = { .init = ktd_init, .exit = ktd_exit, - .size = sizeof(struct sspt_proc *), + .size = sizeof(struct ktd_proc), }; -static struct sspt_proc **pproc_by_task(struct task_struct *task) +static struct ktd_proc *kproc_by_task(struct task_struct *task) { - return (struct sspt_proc **)swap_ktd(&ktd, task); + return (struct ktd_proc *)swap_ktd(&ktd, task); } int sspt_proc_init(void) @@ -121,33 +126,40 @@ void sspt_proc_uninit(void) void sspt_change_leader(struct task_struct *prev, struct task_struct *next) { - struct sspt_proc **prev_pproc; + struct ktd_proc *prev_kproc; - prev_pproc = pproc_by_task(prev); - if (*prev_pproc) { - struct sspt_proc **next_pproc; + prev_kproc = kproc_by_task(prev); + spin_lock(&prev_kproc->lock); + if (prev_kproc->proc) { + struct ktd_proc *next_kproc; - next_pproc = pproc_by_task(next); + next_kproc = kproc_by_task(next); get_task_struct(next); /* Change the keeper sspt_proc */ - BUG_ON(*next_pproc); - *next_pproc = *prev_pproc; - *prev_pproc = NULL; + BUG_ON(next_kproc->proc); + + spin_lock(&next_kproc->lock); + next_kproc->proc = prev_kproc->proc; + prev_kproc->proc = NULL; + spin_unlock(&next_kproc->lock); /* Set new the task leader to sspt_proc */ - (*next_pproc)->leader = next; + next_kproc->proc->leader = next; put_task_struct(prev); } + spin_unlock(&prev_kproc->lock); } void sspt_reset_proc(struct task_struct *task) { - struct sspt_proc **pproc; + struct ktd_proc *kproc; - pproc = pproc_by_task(task->group_leader); - *pproc = NULL; + kproc = kproc_by_task(task->group_leader); + spin_lock(&kproc->lock); + kproc->proc = NULL; + spin_unlock(&kproc->lock); } @@ -232,13 +244,29 @@ void sspt_proc_put(struct sspt_proc *proc) kfree(proc); } } +EXPORT_SYMBOL_GPL(sspt_proc_put); struct sspt_proc *sspt_proc_by_task(struct task_struct *task) { - return *pproc_by_task(task->group_leader); + return kproc_by_task(task->group_leader)->proc; } EXPORT_SYMBOL_GPL(sspt_proc_by_task); +struct sspt_proc *sspt_proc_get_by_task(struct task_struct *task) +{ + struct ktd_proc *kproc = kproc_by_task(task->group_leader); + struct sspt_proc *proc; + + spin_lock(&kproc->lock); + proc = kproc->proc; + if (proc) + sspt_proc_get(proc); + spin_unlock(&kproc->lock); + + return proc; +} +EXPORT_SYMBOL_GPL(sspt_proc_get_by_task); + /** * @brief Call func() on each proc (no lock) * @@ -280,43 +308,43 @@ EXPORT_SYMBOL_GPL(on_each_proc); struct sspt_proc *sspt_proc_get_by_task_or_new(struct task_struct *task) { static DEFINE_MUTEX(local_mutex); - struct sspt_proc **pproc; + struct ktd_proc *kproc; + struct sspt_proc *proc; struct task_struct *leader = task->group_leader; - pproc = pproc_by_task(leader); - if (*pproc) + kproc = kproc_by_task(leader); + if (kproc->proc) goto out; - /* This lock for synchronizing to create sspt_proc */ - mutex_lock(&local_mutex); - pproc = pproc_by_task(leader); - if (*pproc == NULL) { - *pproc = sspt_proc_create(leader); - if (*pproc) { - sspt_proc_write_lock(); - list_add(&(*pproc)->list, &proc_probes_list); - sspt_proc_write_unlock(); - } + proc = sspt_proc_create(leader); + + spin_lock(&kproc->lock); + if (kproc->proc == NULL) { + sspt_proc_get(proc); + kproc->proc = proc; + proc = NULL; + + sspt_proc_write_lock(); + list_add(&kproc->proc->list, &proc_probes_list); + sspt_proc_write_unlock(); } - mutex_unlock(&local_mutex); + spin_unlock(&kproc->lock); + + if (proc) + sspt_proc_cleanup(proc); out: - return *pproc; + return kproc->proc; } /** - * @brief Free all sspt_proc + * @brief Check sspt_proc on empty * * @return Pointer on the sspt_proc struct */ -void sspt_proc_free_all(void) +void sspt_proc_check_empty(void) { - struct sspt_proc *proc, *n; - - list_for_each_entry_safe(proc, n, &proc_probes_list, list) { - list_del(&proc->list); - sspt_proc_cleanup(proc); - } + WARN_ON(!list_empty(&proc_probes_list)); } static void sspt_proc_add_file(struct sspt_proc *proc, struct sspt_file *file) diff --git a/us_manager/sspt/sspt_proc.h b/us_manager/sspt/sspt_proc.h index 54eb80d..835b9cb 100644 --- a/us_manager/sspt/sspt_proc.h +++ b/us_manager/sspt/sspt_proc.h @@ -84,17 +84,18 @@ struct sspt_proc_cb { struct list_head *sspt_proc_list(void); -void sspt_proc_cleanup(struct sspt_proc *proc); +struct sspt_proc *sspt_proc_by_task(struct task_struct *task); +struct sspt_proc *sspt_proc_get_by_task(struct task_struct *task); +struct sspt_proc *sspt_proc_get_by_task_or_new(struct task_struct *task); struct sspt_proc *sspt_proc_get(struct sspt_proc *proc); void sspt_proc_put(struct sspt_proc *proc); +void sspt_proc_cleanup(struct sspt_proc *proc); void on_each_proc_no_lock(void (*func)(struct sspt_proc *, void *), void *data); void on_each_proc(void (*func)(struct sspt_proc *, void *), void *data); -struct sspt_proc *sspt_proc_by_task(struct task_struct *task); -struct sspt_proc *sspt_proc_get_by_task_or_new(struct task_struct *task); -void sspt_proc_free_all(void); +void sspt_proc_check_empty(void); struct sspt_file *sspt_proc_find_file(struct sspt_proc *proc, struct dentry *dentry); diff --git a/us_manager/us_manager.c b/us_manager/us_manager.c index 9b66b6e..8ea1041 100644 --- a/us_manager/us_manager.c +++ b/us_manager/us_manager.c @@ -60,7 +60,7 @@ static void do_usm_stop(void) uninstall_all(); unregister_helper_bottom(); - sspt_proc_free_all(); + sspt_proc_check_empty(); } static int do_usm_start(void) -- 2.7.4