Skip to contentSkip to conversation

Attaching PID 0 to a cgroup

PATCH Dhaval Giani 6 participants 10 messages Jul 1, 2008 – Jul 3, 2008 Documentation

lore
Share & tools
lore

Dhaval Giani

author
#0 lore raw

Hi Paul,

Attaching PID 0 to a cgroup caused the current task to be attached to the cgroup. Looking at the code,

        if (pid) {
                rcu_read_lock();
                tsk = find_task_by_vpid(pid);
                if (!tsk || tsk->flags & PF_EXITING) {
                        rcu_read_unlock();
                        return -ESRCH;
                }
                get_task_struct(tsk);
                rcu_read_unlock();

                if ((current->euid) && (current->euid != tsk->uid)
                    && (current->euid != tsk->suid)) {
                        put_task_struct(tsk);
                        return -EACCES;
                }
        } else {
                tsk = current;
                get_task_struct(tsk);
        }

I was wondering, why this was done. It seems to be unexpected behavior. Wouldn't something like the following be a better response? (I've used EINVAL, but I can change it to ESRCH if that is better.)

---
cgroups: Don't allow PID 0 to be attached to a group

Currently when one trys to attach PID 0 to a cgroup, it attaches the current task. That is not expected behavior. It should return an error instead.

Signed-off-by Dhaval Giani <[email protected]>
kernel/cgroup.c +1 -2
Index: linux-2.6/kernel/cgroup.c
===================================================================
@@ -1309,8 +1309,7 @@ static int attach_task_by_pid(struct cgr
 			return -EACCES;
 		}
 	} else {
-		tsk = current;
-		get_task_struct(tsk);
+			return -EINVAL;
 	}
 
 	ret = cgroup_attach_task(cgrp, tsk);
Signature · 2 lines
regards,
Dhaval
author
#1 lore raw

[put in the wrong alias for containers list correcting it.]

Quoted 55 lines · Dhaval Giani

Hi Paul,

Attaching PID 0 to a cgroup caused the current task to be attached to the cgroup. Looking at the code,

        if (pid) {
                rcu_read_lock();
                tsk = find_task_by_vpid(pid);
                if (!tsk || tsk->flags & PF_EXITING) {
                        rcu_read_unlock();
                        return -ESRCH;
                }
                get_task_struct(tsk);
                rcu_read_unlock();

                if ((current->euid) && (current->euid != tsk->uid)
                    && (current->euid != tsk->suid)) {
                        put_task_struct(tsk);
                        return -EACCES;
                }
        } else {
                tsk = current;
                get_task_struct(tsk);
        }

I was wondering, why this was done. It seems to be unexpected behavior. Wouldn't something like the following be a better response? (I've used EINVAL, but I can change it to ESRCH if that is better.)

---
cgroups: Don't allow PID 0 to be attached to a group

Currently when one trys to attach PID 0 to a cgroup, it attaches the current task. That is not expected behavior. It should return an error instead.

Signed-off-by Dhaval Giani <[email protected]>

Index: linux-2.6/kernel/cgroup.c
===================================================================

kernel/cgroup.c +1 -2
@@ -1309,8 +1309,7 @@ static int attach_task_by_pid(struct cgr
 			return -EACCES;
 		}
 	} else {
-		tsk = current;
-		get_task_struct(tsk);
+			return -EINVAL;
 	}
 
 	ret = cgroup_attach_task(cgrp, tsk);
Signature · 2 lines
regards,
Dhaval
Signature · 2 lines
regards,
Dhaval
#2 lore raw
CC Paul Jackson <[email protected]>
Quoted 8 lines · Dhaval Giani

[put in the wrong alias for containers list correcting it.]

Quoted 5 lines · Dhaval Giani

Hi Paul,

Attaching PID 0 to a cgroup caused the current task to be attached to the cgroup. Looking at the code,

[...]

Quoted 5 lines
Quoted 5 lines

I was wondering, why this was done. It seems to be unexpected behavior. Wouldn't something like the following be a better response? (I've used EINVAL, but I can change it to ESRCH if that is better.)

Why is it unexpected? it follows the behavior of cpuset, so this patch will break backward compatibility of cpuset.

But it's better to document this.

-----------------------------------------

Document the following cgroup usage:

 # echo 0 > /dev/cgroup/tasks
Signed-off-by Li Zefan <[email protected]>
---
 cgroups.txt |    4 ++++
 1 file changed, 4 insertions(+)
Documentation/cgroups.txt +4 -0
index 824fc02..213f533 100644
@@ -390,6 +390,10 @@ If you have several tasks to attach, you have to do it one after another:
 	...
 # /bin/echo PIDn > tasks
 
+You can attach the current task by echoing 0:
+
+# /bin/echo 0 > tasks
+
 3. Kernel API
 =============
author Acked-by
#3 lore raw
Quoted 15 lines · Li Zefan
CC Paul Jackson <[email protected]>
Quoted 8 lines · Dhaval Giani

[put in the wrong alias for containers list correcting it.]

Quoted 5 lines · Dhaval Giani

Hi Paul,

Attaching PID 0 to a cgroup caused the current task to be attached to the cgroup. Looking at the code,

[...]

Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines

I was wondering, why this was done. It seems to be unexpected behavior. Wouldn't something like the following be a better response? (I've used EINVAL, but I can change it to ESRCH if that is better.)

Why is it unexpected? it follows the behavior of cpuset, so this patch will break backward compatibility of cpuset.

Ah, I was not aware of that. Thanks!

Quoted 3 lines

But it's better to document this.

Yes please.

Quoted 6 lines

-----------------------------------------

Document the following cgroup usage:

 # echo 0 > /dev/cgroup/tasks
Signed-off-by Li Zefan <[email protected]>
Acked-by Dhaval Giani <[email protected]>
Quoted 20 lines
---
 cgroups.txt |    4 ++++
 1 file changed, 4 insertions(+)
Documentation/cgroups.txt +4 -0
index 824fc02..213f533 100644
@@ -390,6 +390,10 @@ If you have several tasks to attach, you have to do it one after another:
...
 # /bin/echo PIDn > tasks
 
+You can attach the current task by echoing 0:
+
+# /bin/echo 0 > tasks
+
 3. Kernel API
 =============
Signature · 2 lines
regards,
Dhaval
Acked-by Acked-by Dhaval Giani Acked-by
#4 lore raw
In reply to Li Zefan
Quoted 1 line

But it's better to document this.

Good idea.

Acked-by Paul Jackson <[email protected]>

You (Li Zefan) might want to resend this as a patch, in case Andrew doesn't happen to see this embedded here.

Something like the following:

Subject: [PATCH] cgroup: document zero pid means current task

From: Li Zefan <[email protected]>

Document that a pid of zero(0) can be used to refer to the current task when attaching a task to a cgroup, as in the following usage:

    # echo 0 > /dev/cgroup/tasks

This is consistent with existing cpuset behavior.

Signed-off-by Li Zefan <[email protected]>
Acked-by Dhaval Giani <[email protected]>
Acked-by Paul Jackson <[email protected]>

---

   cgroups.txt |    4 ++++
   1 file changed, 4 insertions(+)

diff --git a/Documentation/cgroups.txt b/Documentation/cgroups.txt
index 824fc02..213f533 100644
--- a/Documentation/cgroups.txt
+++ b/Documentation/cgroups.txt
@@ -390,6 +390,10 @@ If you have several tasks to attach, you have to do it one after another:
...

   # /bin/echo PIDn > tasks

+You can attach the current task by echoing 0:
+
+# /bin/echo 0 > tasks
+
3. Kernel API
=============

Signature · 3 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214
#5 lore raw
In reply to Li Zefan
Quoted 3 lines · Li Zefan

Why is it unexpected? it follows the behavior of cpuset, so this patch will break backward compatibility of cpuset.

Agreed. I think we want to keep this behaviour.

Paul

#6 lore raw
In reply to Li Zefan
Quoted 39 lines · Li Zefan
CC Paul Jackson <[email protected]>
Quoted 8 lines · Dhaval Giani

[put in the wrong alias for containers list correcting it.]

Quoted 5 lines · Dhaval Giani

Hi Paul,

Attaching PID 0 to a cgroup caused the current task to be attached to the cgroup. Looking at the code,

[...]

Quoted 4 lines
Quoted 4 lines

I was wondering, why this was done. It seems to be unexpected behavior. Wouldn't something like the following be a better response? (I've used EINVAL, but I can change it to ESRCH if that is better.)

Why is it unexpected? it follows the behavior of cpuset, so this patch will break backward compatibility of cpuset.

But it's better to document this.

-----------------------------------------

Document the following cgroup usage:

 # echo 0 > /dev/cgroup/tasks
Signed-off-by Li Zefan <[email protected]>
---
 cgroups.txt |    4 ++++
 1 file changed, 4 insertions(+)
Documentation/cgroups.txt +0 -0
index 824fc02..213f533 100644

Documentation/cgroups.txt If you have several tasks to attach, you have to do it one after another: · line 398

Quoted 9 lines
Documentation/cgroups.txt +4 -0
@@ -390,6 +390,10 @@ If you have several tasks to attach, you have to do it one after another:
...
 # /bin/echo PIDn > tasks
 
+You can attach the current task by echoing 0:
+
+# /bin/echo 0 > tasks
+
 3. Kernel API
 =============

Wouldn't be more meaningful to specify the bash's builtin echo here even if it doesn't opportunely handle write() errors?

Using /bin/echo would attach /bin/echo itself to the cgroup, that just exists, so it seems like a kind of noop, isn't it?

-Andrea

author
#7 lore raw
Quoted 52 lines · Andrea Righi
Quoted 39 lines · Li Zefan
CC Paul Jackson <[email protected]>
Quoted 8 lines · Dhaval Giani

[put in the wrong alias for containers list correcting it.]

Quoted 5 lines · Dhaval Giani

Hi Paul,

Attaching PID 0 to a cgroup caused the current task to be attached to the cgroup. Looking at the code,

[...]

Quoted 4 lines
Quoted 4 lines

I was wondering, why this was done. It seems to be unexpected behavior. Wouldn't something like the following be a better response? (I've used EINVAL, but I can change it to ESRCH if that is better.)

Why is it unexpected? it follows the behavior of cpuset, so this patch will break backward compatibility of cpuset.

But it's better to document this.

-----------------------------------------

Document the following cgroup usage:

 # echo 0 > /dev/cgroup/tasks
Signed-off-by Li Zefan <[email protected]>
---
 cgroups.txt |    4 ++++
 1 file changed, 4 insertions(+)
Documentation/cgroups.txt +0 -0
index 824fc02..213f533 100644

Documentation/cgroups.txt If you have several tasks to attach, you have to do it one after another: · line 398

Quoted 8 lines
Documentation/cgroups.txt +3 -0
@@ -390,6 +390,10 @@ If you have several tasks to attach, you have to do it one after another:
...
 # /bin/echo PIDn > tasks
 +You can attach the current task by echoing 0:
+
+# /bin/echo 0 > tasks
+
 3. Kernel API
 =============

Wouldn't be more meaningful to specify the bash's builtin echo here even if it doesn't opportunely handle write() errors?

Quoted 3 lines

Using /bin/echo would attach /bin/echo itself to the cgroup, that just exists, so it seems like a kind of noop, isn't it?

Yes, you are right. this example should use bash's builtin echo.

Signature · 2 lines
regards,
Dhaval
#8 lore raw
Quoted 53 lines · Dhaval Giani
Quoted 52 lines · Andrea Righi
Quoted 39 lines · Li Zefan
CC Paul Jackson <pj-sJ/[email protected]>
Quoted 8 lines · Dhaval Giani

[put in the wrong alias for containers list correcting it.]

Quoted 5 lines · Dhaval Giani

Hi Paul,

Attaching PID 0 to a cgroup caused the current task to be attached to the cgroup. Looking at the code,

[...]

Quoted 4 lines
Quoted 4 lines

I was wondering, why this was done. It seems to be unexpected behavior. Wouldn't something like the following be a better response? (I've used EINVAL, but I can change it to ESRCH if that is better.)

Why is it unexpected? it follows the behavior of cpuset, so this patch will break backward compatibility of cpuset.

But it's better to document this.

-----------------------------------------

Document the following cgroup usage:

 # echo 0 > /dev/cgroup/tasks
Signed-off-by Li Zefan <[email protected]>
---
 cgroups.txt |    4 ++++
 1 file changed, 4 insertions(+)
Documentation/cgroups.txt +0 -0
index 824fc02..213f533 100644

Documentation/cgroups.txt If you have several tasks to attach, you have to do it one after another: · line 398

Quoted 8 lines
Documentation/cgroups.txt +3 -0
@@ -390,6 +390,10 @@ If you have several tasks to attach, you have to do it one after another:
...
 # /bin/echo PIDn > tasks
 +You can attach the current task by echoing 0:
+
+# /bin/echo 0 > tasks
+
 3. Kernel API
 =============

Wouldn't be more meaningful to specify the bash's builtin echo here even if it doesn't opportunely handle write() errors?

Quoted 5 lines
Quoted 3 lines

Using /bin/echo would attach /bin/echo itself to the cgroup, that just exists, so it seems like a kind of noop, isn't it?

Yes, you are right. this example should use bash's builtin echo.

IMHO you need to include this point in the docs verbosely rather than
just switching the docs to bash's builin-in echo. Otherwise it doesn't fully resolve the fundamental confusion you correctly identified.

Or perhaps a snippet of simplified C code will make it clear:
------------

	char buffer[16];
	int fd;

	fd = open("/some/cgroup/tasks", O_WRONLY);

	/* 
	 * These two writes produce the same effect: adding this process
	 * to /some/cgroup.
	 */
	if (the_slightly_shorter_way)
		write(fd, "0", 2);
	else {
		/* The slightly-less-short way */
		snprintf(buffer, 16, "%u", getpid());
		write(fd, buffer, strlen(buffer));
	}

------------

Cheers,
-Matt Helsley

#9 lore raw
Quoted 16 lines · Matt Helsley

------------

       char buffer[16];
       int fd;

       fd = open("/some/cgroup/tasks", O_WRONLY);

       /*
        * These two writes produce the same effect: adding this process
        * to /some/cgroup.
        */
       if (the_slightly_shorter_way)
               write(fd, "0", 2);
       else {
               /* The slightly-less-short way */
               snprintf(buffer, 16, "%u", getpid());
               write(fd, buffer, strlen(buffer));

If it's a threaded application, then you'd need gettid() rather than getpid() for the two to be equivalent.

Paul