linux-kernel - Re: [PATCH] task_pid_nr_ns() breaks proc_pid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20071118142047.GA360@tv-sign.ru>
Date:	Sun, 18 Nov 2007 17:20:47 +0300
From:	Oleg Nesterov <oleg@...sign.ru>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Pavel Emelyanov <xemul@...nvz.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] task_pid_nr_ns() breaks proc_pid_readdir()

On 11/17, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@...sign.ru> writes:
> 
> > Make sure that task_pid_nr_ns() returns !0 before updating tgid. Note that
> > next_tgid(tgid + 1) can find the same "struct pid" again, but we shouldn't
> > go into the endless loop because pid_task(PIDTYPE_PID) must return NULL in
> > this case, so next_tgid() can't return the same task.
> >
> Oleg I think I would rather update next_tgid to return the tgid (which
> removes the need to call task_pid_nr_ns).  This keeps all of the task
> iteration logic together in next_tgid.

Yes sure, I think your patch is also correct, please use it.

<offtopic>

Personally, I hate the functions which use pointers to return another value.
(yes, yes, I know, my taste is perverted). Why don't we return structure in
this case? We can even make a common helper struct, say,

	struct pair {
		union {
			long val1;
			void *ptr1;
		};
		union {
			long val2;
			void *ptr2;
		};
	};

	#define PAIR(x1, x2)	(struct pair){{ . x1 }, { . x2 }}

Now, next_tgid() can do

	return PAIR(ptr1 = task, val2 = tgid);

With -freg-struct-return the generated code is nice.

Of course, another option is to rewrite the kernle in perl, in that case
proc_pid_readdir() can just do

	(task, tgid) = next_tgid();

</offtopic>

> Although looking at this in more detail, I'm half wondering if
> proc_pid_make_inode() should take a struct pid instead of a task.

Yes, I also thought about this. Needs more changes, and still not perfect.

I am starting to think we need a more generic change. How about the patch
below? With this change the stable task_struct implies we have the stable
pids, this allows us to do a lot of cleanups.

Oleg.

--- kernel/pid.c	2007-10-25 16:22:12.000000000 +0400
+++ -	2007-11-18 16:56:30.682555454 +0300
@@ -323,7 +323,7 @@ int fastcall attach_pid(struct task_stru
 	struct pid_link *link;
 
 	link = &task->pids[type];
-	link->pid = pid;
+	link->pid = get_pid(pid);
 	hlist_add_head_rcu(&link->node, &pid->tasks[type]);
 
 	return 0;
@@ -339,7 +339,6 @@ void fastcall detach_pid(struct task_str
 	pid = link->pid;
 
 	hlist_del_rcu(&link->node);
-	link->pid = NULL;
 
 	for (tmp = PIDTYPE_MAX; --tmp >= 0; )
 		if (!hlist_empty(&pid->tasks[tmp]))
@@ -348,6 +347,14 @@ void fastcall detach_pid(struct task_str
 	free_pid(pid);
 }
 
+void task_put_pids(struct pid_link *pids)
+{
+	int type = PIDTYPE_MAX;
+
+	while (type--)
+		put_pid(pids[type].pid);
+}
+
 /* transfer_pid is an optimization of attach_pid(new), detach_pid(old) */
 void fastcall transfer_pid(struct task_struct *old, struct task_struct *new,
 			   enum pid_type type)
--- kernel/fork.c	2007-11-09 12:57:31.000000000 +0300
+++ -	2007-11-18 16:57:34.037105563 +0300
@@ -121,6 +121,7 @@ void __put_task_struct(struct task_struc
 	WARN_ON(atomic_read(&tsk->usage));
 	WARN_ON(tsk == current);
 
+	task_put_pids(tsk->pids);
 	security_task_free(tsk);
 	free_uid(tsk->user);
 	put_group_info(tsk->group_info);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/