Skip to contentSkip to conversation

kernel/pid.c: Masking the flag out to get the actual value.

PATCH In review Raphael S. Carvalho 3 participants 5 messages Jun 10, 2013 – Jun 12, 2013

lore
Share & tools
lore

Raphael S. Carvalho

author
#0 lore raw

PATCH 1/1 kernel/pid.c: Masking the flag out to get the actual value.

This patch shouldn't be applied if those branches must only be taken when the pid_allocation(PIDNS_HASH_ADDING) flag was turned off. Otherwise, we must turn the PIDNS_HASH_ADDING flag (1U << 31) off before getting into the switch-cases.

Signed-off-by Raphael S. Carvalho <[email protected]>
---
 kernel/pid.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
kernel/pid.c +4 -1
index 0db3e79..6bda527 100644
@@ -263,7 +263,10 @@ void free_pid(struct pid *pid)
 		struct upid *upid = pid->numbers + i;
 		struct pid_namespace *ns = upid->ns;
 		hlist_del_rcu(&upid->pid_chain);
-		switch(--ns->nr_hashed) {
+
+		/* We must turn the PIDNS_HASH_ADDING flag off to
+		   get the actual value of nr_hashed */
+		switch ((--ns->nr_hashed) & ~(PIDNS_HASH_ADDING)) {
 		case 1:
 			/* When all that is left in the pid namespace
 			 * is the reaper wake up the reaper.  The reaper
Signature · 1 line
1.7.2.5
#1 lore raw
Quoted 2 lines · On Mon, 10 Jun 2013 18:56:38 -0300 "Raphael S. Carvalho"…

This patch shouldn't be applied if those branches must only be taken when the pid_allocation(PIDNS_HASH_ADDING) flag was turned off.

Well yes - hopefully this is the case. Otherwise we're missing some rather important-looking wakeups.

Quoted 9 lines

Otherwise, we must turn the PIDNS_HASH_ADDING flag (1U << 31) off before getting into the switch-cases.

...

kernel/pid.c +0 -0
@@ -263,7 +263,10 @@ void free_pid(struct pid *pid)
 		struct upid *upid = pid->numbers + i;

kernel/pid.c void free_pid(struct pid *pid) · line 272

Quoted 10 lines
kernel/pid.c +4 -1
 		struct pid_namespace *ns = upid->ns;
 		hlist_del_rcu(&upid->pid_chain);
-		switch(--ns->nr_hashed) {
+
+		/* We must turn the PIDNS_HASH_ADDING flag off to
+		   get the actual value of nr_hashed */
+		switch ((--ns->nr_hashed) & ~(PIDNS_HASH_ADDING)) {
 		case 1:
 			/* When all that is left in the pid namespace
 			 * is the reaper wake up the reaper.  The reaper

Eric, can you please take a look? Presumably and hopefully PIDNS_HASH_ADDING cannot be set here, but what guarantees this?

Hopefully we can fix this one by adding the missing comment.

#2 lore raw
Quoted 4 lines · Andrew Morton
Quoted 2 lines · On Mon, 10 Jun 2013 18:56:38 -0300 "Raphael S. Carvalho"…

This patch shouldn't be applied if those branches must only be taken when the pid_allocation(PIDNS_HASH_ADDING) flag was turned off.

That is correct. We should not encounter the last pid in the pid namespace while still allowing processes to be created in the pid namespace.

So the patch I am seeing quoted below is unnecessary.

Quoted 18 lines

Well yes - hopefully this is the case. Otherwise we're missing some rather important-looking wakeups.

Quoted 14 lines

Otherwise, we must turn the PIDNS_HASH_ADDING flag (1U << 31) off before getting into the switch-cases.

...

kernel/pid.c +2 -1
@@ -263,7 +263,10 @@ void free_pid(struct pid *pid)
 		struct upid *upid = pid->numbers + i;
 		struct pid_namespace *ns = upid->ns;
 		hlist_del_rcu(&upid->pid_chain);
-		switch(--ns->nr_hashed) {
+
+		/* We must turn the PIDNS_HASH_ADDING flag off to
Quoted 8 lines

kernel/pid.c void free_pid(struct pid *pid) · line 272

Quoted 5 lines
kernel/pid.c +2 -0
+		   get the actual value of nr_hashed */
+		switch ((--ns->nr_hashed) & ~(PIDNS_HASH_ADDING)) {
 		case 1:
 			/* When all that is left in the pid namespace
 			 * is the reaper wake up the reaper.  The reaper

Eric, can you please take a look? Presumably and hopefully PIDNS_HASH_ADDING cannot be set here, but what guarantees this?

The init process has not exited, and called zap_pid_ns_processes.

In fact there is a case where nr_hashed can be 0 | PIDNS_HASH_ADDING
where we absolutely don't want to do these things. Of course there
are no pids allocated in that case to free so we can't possible get to the switch in free pid either.

Quoted 1 line

Hopefully we can fix this one by adding the missing comment.

Perhaps we can fix this one by having people who care read the code and think about what it means? Seriously if we are adding pids/processes in the pid namespace why would to clean up the pid namespace?

Eric

#3 lore raw
Quoted 5 lines · On Tue, 11 Jun 2013 18:16:50 -0700 [email protected] (Eric W. Biederman)…
Quoted 2 lines

Hopefully we can fix this one by adding the missing comment.

Perhaps we can fix this one by having people who care read the code and think about what it means?

As is obvious from this thread, that approach isn't working.

Quoted 2 lines

Seriously if we are adding pids/processes in
the pid namespace why would to clean up the pid namespace?

A good way to communicate the design would be to describe the semantics of PIDNS_HASH_ADDING, at its definition site.

[idly wonders what the heck pid_namespace.level and pid.level do, sigh]

#4 lore raw
Quoted 10 lines · Andrew Morton
Quoted 5 lines · On Tue, 11 Jun 2013 18:16:50 -0700 [email protected] (Eric W. Biederman)…
Quoted 2 lines

Hopefully we can fix this one by adding the missing comment.

Perhaps we can fix this one by having people who care read the code and think about what it means?

As is obvious from this thread, that approach isn't working.

Quoted 8 lines
Quoted 2 lines

Seriously if we are adding pids/processes in
the pid namespace why would to clean up the pid namespace?

A good way to communicate the design would be to describe the semantics of PIDNS_HASH_ADDING, at its definition site.

[idly wonders what the heck pid_namespace.level and pid.level do, sigh]

Explaining the semantics a bit more seems reasonable.

Something like:

unsigned int level; /* How deeply nested is this pid namespace */

#define PIDNS_HASH_ADDING (1U << 31) /* Process are still entering the pid namespace */

Sorry I don't have the focus to make that into a proper patch.

Eric