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    [from the cache]
©2003-2011 Jasper Spaans