Oleg Nesterov <[email protected]> writes:
> Eric W. Biederman wrote:
>>
>> +++ devel-akpm/kernel/task_ref.c 2006-02-27 20:28:59.000000000 -0800
>> @@ -0,0 +1,131 @@
>> +#include <linux/sched.h>
>> +#include <linux/task_ref.h>
>> +
>> +struct task_ref init_tref = {
>> + .count = ATOMIC_INIT(1),
>> + .type = PIDTYPE_PID,
>> + .pid = 0,
>> + .task = NULL,
>> +};
>
> Make it static? Actually, I don't understand why init_tref is better
> than NULL. Yes, NULL will add some checks into task_ref.c, but we can
> avoid some costly atoimic ops.
init_tref isn't for task_ref.c so much as it is there to allow the
users of task_refs to avoid the checks. I have enough helper
functions an appropriate helper instead of a well prescribed value
may be an equally valid approach.
>> +void tref_put(struct task_ref *ref)
>> +{
>> + might_sleep();
>> + if (atomic_dec_and_test(&ref->count)) {
>> + struct task_struct *task;
>> + BUG_ON(ref == &init_tref);
>> + /* Carefully serialize against __detach_pid and tref_get_by_pid */
>> + write_lock_irq(&tasklist_lock);
>> + task = ref->task;
>> + if (task)
>> + task->pids[ref->type].ref = NULL;
>> + write_unlock_irq(&tasklist_lock);
>> + kfree(ref);
>> + }
>
> I think this is racy. Suppose ref->count == 1. What if another cpu does
> tref_get_by_task() between atomic_dec_and_test() and write_lock_irq() ?
> It takes tasklist_lock, increments ->count again, and returns the pointer
> to the memory which will be freed soon.
Agreed. Grumble. This needs to get fixed.
>> +struct task_ref *tref_get_by_pid(int pid, enum pid_type type)
>> +{
>> + struct task_struct *task;
>> + struct task_ref *tref;
>> +
>> + /* Lookup the and pin the task */
>> + read_lock(&tasklist_lock);
>> + task = find_task_by_pid_type(type, pid);
>> + if (task)
>> + get_task_struct(task);
>> + read_unlock(&tasklist_lock);
>> +
>> + /* Now get the tref */
>> + if (task) {
>> + tref = tref_get_by_task(task, type);
>> + put_task_struct(task);
>> + }
>> + else
>> + tref = tref_get(&init_tref);
>> + return tref;
>> +}
>
> I beleive this could be simplified, we don't need to get/put task_struct,
>
> rcu_read_lock();
>
> task = find_task_by_pid_type(type, pid);
> if (task)
> tref = tref_get_by_task(task, type);
> else
> tref = tref_get(&init_tref);
>
> rcu_read_unlock();
Nope kmalloc can sleep.
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
