lkml.org 
[lkml]   [2012]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v5
    On 12/28, Cyrill Gorcunov wrote:
    >
    > When we do checkpoint of a task we need to know the list of children
    > the task has but there is no easy way to make a reverse parent->children
    > chain from an arbitrary <pid> (while a parent pid is provided in "PPid"
    > field of /proc/<pid>/status).

    Looks correct at first glance... But I'll try to recheck. I guess you need
    to resend anyway, I bet nobody can recall this patch ;)

    However I do not understand the ptrace_may_access() check at all.

    > +static struct pid *
    > +get_children_pid(struct proc_pid_children_iter *iter, struct pid *pid_prev, loff_t pos)
    > +{
    > + struct task_struct *start, *task;
    > + struct pid *pid = NULL;
    > +
    > + read_lock(&tasklist_lock);
    > +
    > + start = pid_task(iter->pid_start, PIDTYPE_PID);
    > + if (!start)
    > + goto out;
    > +
    > + /*
    > + * Lets try to continue searching this would speed
    > + * search significantly.
    > + */
    > + if (pid_prev) {
    > + task = pid_task(pid_prev, PIDTYPE_PID);
    > + if (task && task->real_parent == start &&
    > + !(list_empty(&task->sibling))) {
    > + /*
    > + * OK, ltes try the fastpath, we might
    > + * miss some freshly created children
    > + * here, but it was never promised to be
    > + * accurate.
    > + *
    > + * Also note if we have not enough rights
    > + * to access the next children pid we simply
    > + * fall into slow-search version.
    > + */

    Why we should try the slow-search path if ptrace_may_access() fails?

    > + if (!list_is_last(&task->sibling, &start->children)) {
    > + task = list_first_entry(&task->sibling,
    > + struct task_struct, sibling);
    > + if (ptrace_may_access(task, PTRACE_MODE_READ)) {
    > + pid = get_pid(task_pid(task));
    > + goto out;
    > + }
    > + } else
    > + goto out;
    > + }
    > + }

    Well, this is cosmetic, but imho

    if (list_is_last(...))
    goto out;

    task = list_first_entry(...);
    ...

    looks better.


    > + list_for_each_entry(task, &start->children, sibling) {
    > + if (pos-- == 0) {
    > + if (ptrace_may_access(task, PTRACE_MODE_READ)) {
    > + pid = get_pid(task_pid(task));
    > + goto out;
    > + } else {
    > + /* Maybe we success with the next children */
    > + pos++;

    Again. I simply can't understand what ptrace_may_access() actually
    means. Why do we use the possible child, not parent?

    IOW. I have no idea if we really need any security check at all.
    You can find the children pids without this patch anyway via.
    grep PPid /proc/*/status.

    But if you want ptrace_may_access/whatever, you should check
    ptrace_may_access(start), no?

    Oleg.



    \
     
     \ /
      Last update: 2012-01-15 19:15    [W:0.026 / U:0.416 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site