Messages in this thread |  | | | Date | Sun, 15 Jan 2012 19:07:21 +0100 | | From | Oleg Nesterov <> | | Subject | Re: [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.
|  |