lkml.org 
[lkml]   [2009]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [patch 1/9] sys: Fix missing rcu protection for __task_cred()access
    From
    Date
    Thomas Gleixner wrote:
    > On Fri, 11 Dec 2009, Tetsuo Handa wrote:
    > > > Usually tasklist gives enough protection, but if copy_process() fails
    > > > it calls free_pid() lockless and does call_rcu(delayed_put_pid().
    > > > This means, without rcu lock find_pid_ns() can't scan the hash table
    > > > safely.
    > >
    > > So, we need to change below comment from "or" to "and" ?
    >
    > No, both functions must be called with rcu_read_lock()
    >
    > tasklist_lock read-held is not protecting the rcu lists and does not
    > protect against a concurrent update. It merily protects against tasks
    > going away or being added while we look up the lists.

    So, rcu_read_lock() is mandatory.
    But I couldn't understand why tasklist_lock being held is not mandatory.

    Caller of these functions will use "struct task_struct" and expect that values
    and pointers retrieved by dereferencing returned pointer are valid.

    If "struct task_struct" was removed from the list due to missing tasklist_lock
    between returning from find_task_by_pid_ns() and dereferencing, I worry that
    values and pointers retrieved by dereferencing are invalid.

    rcu_read_lock() being held will prevent the returned "struct task_struct" from
    being kfree()d, but I think rcu_read_lock() being held does not prevent values
    and pointers of "struct task_struct" from being invalidated.



    Anyway, I browsed 2.6.32 for find_task_by_vpid() / find_task_by_pid_ns() users
    and found below users which lacks read_lock(&tasklist_lock) or rcu_read_lock().

    Users missing read_lock(&tasklist_lock) when calling find_task_by_vpid():

    get_net_ns_by_pid() in net/core/net_namespace.c
    seq_print_userip_objs() in kernel/trace/trace_output.c
    fill_pid() in kernel/taskstats.c
    fill_tgid() in kernel/taskstats.c
    futex_find_get_task() in kernel/futex.c
    SYSCALL_DEFINE3(get_robust_list) in kernel/futex.c
    compat_sys_get_robust_list() in kernel/futex_compat.c
    ptrace_get_task_struct() in kernel/ptrace.c
    do_send_specific() in kernel/signal.c
    find_get_context() in kernel/perf_event.c
    posix_cpu_clock_get() in kernel/posix-cpu-timers.c
    good_sigevent() in kernel/posix-timers.c
    attach_task_by_pid() in kernel/cgroup.c
    SYSCALL_DEFINE1(getpgid) in kernel/sys.c
    SYSCALL_DEFINE1(getsid) in kernel/sys.c
    do_sched_setscheduler() in kernel/sched.c

    Users missing rcu_read_lock() when calling find_task_by_vpid():

    SYSCALL_DEFINE3(ioprio_set) in fs/ioprio.c
    SYSCALL_DEFINE2(ioprio_get) in fs/ioprio.c
    cap_get_target_pid() in kernel/capability.c
    audit_prepare_user_tty() in kernel/audit.c
    audit_receive_msg() in kernel/audit.c
    check_clock() in kernel/posix-cpu-timers.c
    posix_cpu_timer_create() in kernel/posix-cpu-timers.c
    SYSCALL_DEFINE3(setpriority) in kernel/sys.c
    SYSCALL_DEFINE2(getpriority) in kernel/sys.c
    SYSCALL_DEFINE2(setpgid) in kernel/sys.c
    SYSCALL_DEFINE1(sched_getscheduler) in kernel/sched.c
    SYSCALL_DEFINE2(sched_getparam) in kernel/sched.c
    sched_setaffinity() in kernel/sched.c
    sched_getaffinity() in kernel/sched.c
    SYSCALL_DEFINE2(sched_rr_get_interval) in kernel/sched.c
    tomoyo_is_select_one() in security/tomoyo/common.c
    tomoyo_read_pid() in security/tomoyo/common.c
    SYSCALL_DEFINE6(move_pages) in mm/migrate.c
    SYSCALL_DEFINE4(migrate_pages) in mm/mempolicy.c
    find_process_by_pid() in arch/mips/kernel/mips-mt-fpaff.c
    pfm_get_task() in arch/ia64/kernel/perfmon.c
    cxn_pin_by_pid() in arch/frv/mm/mmu-context.c

    Users missing read_lock(&tasklist_lock) when calling find_task_by_pid_ns():

    rest_init() in init/main.c
    proc_pid_lookup() in fs/proc/base.c
    proc_task_lookup() in fs/proc/base.c
    first_tid() in fs/proc/base.c
    getthread() in kernel/kgdb.c
    mconsole_stack() in arch/um/drivers/mconsole_kern.c

    Users missing rcu_read_lock() when calling find_task_by_pid_ns():

    rest_init() in init/main.c
    getthread() in kernel/kgdb.c
    mconsole_stack() in arch/um/drivers/mconsole_kern.c

    Regarding users which lack rcu_read_lock(), users call
    read_lock(&tasklist_lock) in order to access "struct task_struct" returned
    by find_task_by_pid_ns() but they do not want to be bothered by internal pid
    structure. Thus, the fix would be to add rcu_read_lock() and rcu_read_unlock()
    inside find_task_by_pid_ns().

    But how to check users which lack read_lock(&tasklist_lock) but expecting that
    it is safe to access "struct task_struct" returned by find_task_by_pid_ns() ?


    \
     
     \ /
      Last update: 2009-12-11 04:29    [W:0.038 / U:29.428 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site