Start using struct pid.
Subsystems (9)
In the last round of cleaning up the pid hash table a more general struct pid was introduced, that can be referenced counted.
With the more general struct pid most if not all places where we store a pid_t we can now store a struct pid * and remove the need for a hash table lookup, and avoid any possible problems with pid roll over.
Looking forward to the pid namespaces struct pid * gives us an absolute form a pid so we can compare and use them without caring which pid namespace we are in.
This patchset introduces the infrastructure needed to use struct pid instead of pid_t, and then it goes on to convert two different kernel users that currently store a pid_t value.
There are a lot more places to go but this is enough to get the basic idea.
Before we can merge a pid namespace patch all of the kernel pid_t users need to be examined. Those that deal with user space processes need to be converted to using a struct pid *. Those that deal with kernel processes need to converted to using the kthread api. A rare few that only use their current processes pid values get to be left alone.
Eric
PATCH 1/7 pid: Implement access helpers for a tacks various process groups.
task_session returns the struct pid of a tasks session.
task_pgrp returns the struct pid of a tasks process group. task_tgid returns the struct pid of a tasks thread group. task_pid returns the struct pid of a tasks process id.
These can be used to avoid unnecessary hash table lookups, and to implement safe pid comparisions in the face of a pid namespace.
--- include/linux/sched.h | 20 ++++++++++++++++++++ 1 files changed, 20 insertions(+), 0 deletions(-)
include/linux/sched.h +20 -0
@@ -1042,6 +1042,26 @@ static inline pid_t process_group(structreturn tsk->signal->pgrp; } +static inline struct pid *task_pid (struct task_struct *task) +{ +return task->pids[PIDTYPE_PID ].pid; +} + +static inline struct pid *task_tgid (struct task_struct *task) +{ +return task->group_leader->pids[PIDTYPE_PID ].pid; +} + +static inline struct pid *task_pgrp (struct task_struct *task) +{ +return task->group_leader->pids[PIDTYPE_PGID ].pid; +} + +static inline struct pid *task_session (struct task_struct *task) +{ +return task->group_leader->pids[PIDTYPE_SID ].pid; +} +/** * pid_alive - check that a task structure is not stale * @p: Task structure to be checked.
Signature · 1 line
1.4.2.rc3.g7e18e-dirty
Re: [PATCH 1/7] pid: Implement access helpers for a tacks various process groups.
include/linux/sched.h static inline pid_t process_group(struct · line 1048
Quoted 4 lines · Eric W. Biederman
include/linux/sched.h +4
-0+static inline struct pid *task_pid (struct task_struct *task) +{ +return task->pids[PIDTYPE_PID ].pid; +}
Does this mean we can start to deprecate the use of tsk->pid?
-- Dave
Re: [PATCH 1/7] pid: Implement access helpers for a tacks various process groups.
Quoted 7 lines · Dave Hansen
include/linux/sched.h static inline pid_t process_group(struct · line 1048
Quoted 4 lines · Eric W. Biederman
include/linux/sched.h +4
-0+static inline struct pid *task_pid (struct task_struct *task) +{ +return task->pids[PIDTYPE_PID ].pid; +}Does this mean we can start to deprecate the use of
tsk->pid?
Good question. I think there are enough users in the same process case that it might not make sense to get rid of tsk->pid. Some of tsk->pids cousins though like tgid are definitely up for grabs.
I haven't gotten far enough to be able to say for certain.
Eric
Re: [Containers] [PATCH 1/7] pid: Implement access helpers for a tacks various process groups.
In reply to Dave HansenQuoted 9 lines · Eric W. Biederman
include/linux/sched.h static inline pid_t process_group(struct · line 1048
Quoted 5 lines
include/linux/sched.h +4
-0+static inline struct pid *task_pid (struct task_struct *task) +{ +return task->pids[PIDTYPE_PID ].pid; +}Does this mean we can start to deprecate the use of
tsk->pid?
Thats the final goal imho :)))
Kirill
PATCH 2/7 pid: Add do_each_pid_task
In reply to Eric W. BiedermanTo avoid pid rollover confusion the kernel needs to work with struct pid * instead of pid_t. Currently there is not an iterator that walks through all of the tasks of a given pid type starting with a struct pid. This prevents us replacing some pid_t instances with struct pid. So this patch adds do_each_pid_task which walks through the set of task for a given pid type starting with a struct pid.
--- include/linux/pid.h | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
include/linux/pid.h +13 -0
@@ -118,4 +118,17 @@ #define while_each_task_pid(who, type, t 1; }) ); \ } +#define do_each_pid_task (pid, type, task) \ + if ((task = pid_task(pid, type))) { \ + prefetch(pid_next(task, type)); \ + do { + +#define while_each_pid_task (pid, type, task) \ + } while (pid_next(task, type) && ({ \ + task = pid_next_task(task, type); \ + rcu_dereference(task); \ + prefetch(pid_next(task, type)); \ + 1; }) ); \ + } +#endif /* _LINUX_PID_H */
Signature · 1 line
1.4.2.rc3.g7e18e-dirty
Re: [Containers] [PATCH 2/7] pid: Add do_each_pid_task
Quoted 17 lines · Eric W. Biederman
To avoid pid rollover confusion the kernel needs to work with struct pid * instead of
pid_t. Currently there is not an iterator that walks through all of the tasks of a given pid type starting with a struct pid. This prevents us replacing somepid_tinstances with struct pid. So this patch addsdo_each_pid_taskwhich walks through the set of task for a given pid type starting with a struct pid.Signed-off-by Eric W. Biederman <[email protected]>--- include/linux/pid.h | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)include/linux/pid.h +0
-0
include/linux/pid.h #define while_each_task_pid(who, type, t · line 121
Quoted 4 lines
include/linux/pid.h +1
-0@@ -118,4 +118,17 @@ #define while_each_task_pid(who, type, t 1; }) ); \ } +#define do_each_pid_task (pid, type, task) \
Hmm, defining do_each_pid_task right after do_each_task_pid could result in some frustration :)
Though not sure of a better name - do_each_task_structpid?
-serge
Re: [Containers] [PATCH 2/7] pid: Add do_each_pid_task
Quoted 21 lines · On Tue, 15 Aug 2006 22:10:43 -0500 "Serge E. Hallyn" <[email protected]> wrote:
Quoted 20 lines · Eric W. Biederman
To avoid pid rollover confusion the kernel needs to work with struct pid * instead of
pid_t. Currently there is not an iterator that walks through all of the tasks of a given pid type starting with a struct pid. This prevents us replacing somepid_tinstances with struct pid. So this patch addsdo_each_pid_taskwhich walks through the set of task for a given pid type starting with a struct pid.Signed-off-by Eric W. Biederman <[email protected]>--- include/linux/pid.h | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)include/linux/pid.h +0
-0@@ -118,4 +118,17 @@ #define while_each_task_pid(who, type, t 1; }) ); \ }
Quoted 4 lines
include/linux/pid.h #define while_each_task_pid(who, type, t · line 121
Quoted 1 line
include/linux/pid.h +1
-0+#define do_each_pid_task (pid, type, task) \Hmm, defining
do_each_pid_taskright afterdo_each_task_pidcould result in some frustration :)
That's all right - developers can read the comments to work out what each one does.
<seems I'm having a sarcastic day>
Re: [Containers] [PATCH 2/7] pid: Add do_each_pid_task
Quoted 29 lines · Andrew Morton
Quoted 21 lines · On Tue, 15 Aug 2006 22:10:43 -0500 "Serge E. Hallyn" <[email protected]> wrote:
Quoted 20 lines · Eric W. Biederman
To avoid pid rollover confusion the kernel needs to work with struct pid * instead of
pid_t. Currently there is not an iterator that walks through all of the tasks of a given pid type starting with a struct pid. This prevents us replacing somepid_tinstances with struct pid. So this patch addsdo_each_pid_taskwhich walks through the set of task for a given pid type starting with a struct pid.Signed-off-by Eric W. Biederman <[email protected]>--- include/linux/pid.h | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)include/linux/pid.h +0
-0@@ -118,4 +118,17 @@ #define while_each_task_pid(who, type, t 1; }) ); \ }Quoted 4 lines
include/linux/pid.h #define while_each_task_pid(who, type, t · line 121
Quoted 1 line
include/linux/pid.h +1
-0+#define do_each_pid_task (pid, type, task) \Hmm, defining
do_each_pid_taskright afterdo_each_task_pidcould result in some frustration :)
Quoted 4 lines
That's all right - developers can read the comments to work out what each one does.
<seems I'm having a sarcastic day>
The plan is to that having both of them in the same kernel should be a short lived thing. There are few enough of these perhaps I should just replace all of them at once.
Doing this gradually and reviewably seems to require that I have both versions of the API simultaneously. The core problem here is that there aren't many good names.
Eric
Re: [Containers] [PATCH 2/7] pid: Add do_each_pid_task
In reply to Serge E. HallynQuoted 21 lines · Serge E. Hallyn
Quoted 20 lines · Eric W. Biederman
To avoid pid rollover confusion the kernel needs to work with struct pid * instead of
pid_t. Currently there is not an iterator that walks through all of the tasks of a given pid type starting with a struct pid. This prevents us replacing somepid_tinstances with struct pid. So this patch addsdo_each_pid_taskwhich walks through the set of task for a given pid type starting with a struct pid.Signed-off-by Eric W. Biederman <[email protected]>--- include/linux/pid.h | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)include/linux/pid.h +0
-0@@ -118,4 +118,17 @@ #define while_each_task_pid(who, type, t 1; }) ); \ }
Quoted 6 lines
include/linux/pid.h #define while_each_task_pid(who, type, t · line 121
Quoted 1 line
include/linux/pid.h +1
-0+#define do_each_pid_task (pid, type, task) \Hmm, defining
do_each_pid_taskright afterdo_each_task_pidcould result in some frustration :)Though not sure of a better name -
do_each_task_structpid?
A couple of things.
- I think do_each_pid_task is probably the most descriptive name I can come up with. As these are tasks of a pid. I don't quite understand where the old name came from.
- Whatever we pick for the new function is the name we are going to use for a long time so I don't want it to be clumsy. The existing function will go away after everything is updated.
- If you pick the wrong one you will get a compile error, as integers and pointers are not very interchangeable.
I hate the temporary confusion but I don't see a better alternative.
Eric
Re: [Containers] [PATCH 2/7] pid: Add do_each_pid_task
Quoted 30 lines · Eric W. Biederman
Quoted 21 lines · Serge E. Hallyn
Quoted 20 lines · Eric W. Biederman
To avoid pid rollover confusion the kernel needs to work with struct pid * instead of
pid_t. Currently there is not an iterator that walks through all of the tasks of a given pid type starting with a struct pid. This prevents us replacing somepid_tinstances with struct pid. So this patch addsdo_each_pid_taskwhich walks through the set of task for a given pid type starting with a struct pid.Signed-off-by Eric W. Biederman <[email protected]>--- include/linux/pid.h | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)include/linux/pid.h +0
-0@@ -118,4 +118,17 @@ #define while_each_task_pid(who, type, t 1; }) ); \ }Quoted 6 lines
include/linux/pid.h #define while_each_task_pid(who, type, t · line 121
Quoted 1 line
include/linux/pid.h +1
-0+#define do_each_pid_task (pid, type, task) \Hmm, defining
do_each_pid_taskright afterdo_each_task_pidcould result in some frustration :)Though not sure of a better name -
do_each_task_structpid?
Quoted 8 lines
A couple of things.
- I thinkdo_each_pid_taskis probably the most descriptive name I can come up with. As these are tasks of a pid. I don't quite understand where the old name came from.- Whatever we pick for the new function is the name we are going to use for a long time so I don't want it to be clumsy. The existing function will go away after everything is updated.
Ok. If the old iterator is going away that's fine.
I was under the impression we'd still need to be able to do the loop based on pid_t as well.
thanks,
-serge
Re: [PATCH 2/7] pid: Add do_each_pid_task
In reply to Eric W. Biedermaninclude/linux/pid.h #define while_each_task_pid(who, type, t · line 132
Quoted 13 lines · Eric W. Biederman
include/linux/pid.h +12
-0+#define do_each_pid_task (pid, type, task) \ + if ((task = pid_task(pid, type))) { \ + prefetch(pid_next(task, type)); \ + do { + +#define while_each_pid_task (pid, type, task) \ + } while (pid_next(task, type) && ({ \ + task = pid_next_task(task, type); \ + rcu_dereference(task); \ + prefetch(pid_next(task, type)); \ + 1; }) ); \ + }
A small nit. Suppose a non-leader thread blocks a signal, and does exec. There is a window when we have 2 tasks with the same pid in PIDTYPE_PID namespace. If send_sigio() send the signal in that window, it will be delivered twice to process.
Oleg.
PATCH 1/1 simplify pid iterators
In reply to Eric W. BiedermanOn top of Eric's recent include/linux/pid.h changes.
I think it is hardly possible to read the current do_each_task_pid(). The new version is much simpler and makes the code smaller.
Only the do_each_task_pid change is tested, the do_each_pid_task isn't. Eric, could you take a hard look at this patch?
include/linux/pid.h renamed from include/linux/pid.h~ +23 -36
@@ -99,42 +99,29 @@ static inline pid_t pid_nr(struct pid *p return nr; } -#define pid_next (task, type) \ - ((task)->pids[(type)].node.next) -#define pid_next_task (task, type) \ - hlist_entry(pid_next(task, type), struct task_struct, \ - pids[(type)].node) - - -/* We could use hlist_for_each_entry_rcu here but it takes more arguments -* than the do_each_task_pid/while_each_task_pid. So we roll our own -* to preserve the existing interface. -*/ -#define do_each_task_pid (who, type, task) \ - if ((task = find_task_by_pid_type(type, who))) { \ - prefetch(pid_next(task, type)); \ - do { - -#define while_each_task_pid (who, type, task) \ - } while (pid_next(task, type) && ({ \ - task = pid_next_task(task, type); \ - rcu_dereference(task); \ - prefetch(pid_next(task, type)); \ - 1; }) ); \ - } - -#define do_each_pid_task (pid, type, task) \ - if ((task = pid_task(pid, type))) { \ - prefetch(pid_next(task, type)); \ - do { - -#define while_each_pid_task (pid, type, task) \ - } while (pid_next(task, type) && ({ \ - task = pid_next_task(task, type); \ - rcu_dereference(task); \ - prefetch(pid_next(task, type)); \ - 1; }) ); \ - } +#define do_each_task_pid (who, type, task) \ + do { \ + struct hlist_node *pos___; \ + struct pid *pid___ = find_pid(who); \ + if (pid___ != NULL) \ + hlist_for_each_entry_rcu((task), pos___, \ + &pid___->tasks[type], pids[type].node) { + +#define while_each_task_pid (who, type, task) \ + } \ + } while (0) + + +#define do_each_pid_task (pid, type, task) \ + do { \ + struct hlist_node *pos___; \ + if (pid != NULL) \ + hlist_for_each_entry_rcu((task), pos___, \ + &pid->tasks[type], pids[type].node) { + +#define while_each_pid_task (pid, type, task) \ + } \ + } while (0)#endif /* _LINUX_PID_H */
PATCH 3/7 pid: Implement signal functions that take a struct pid *
In reply to Eric W. BiedermanCurrently the signal functions all either take a task or a pid_t argument. This patch implements variants that take a struct pid *. After all of the users have been update it is my intention to remove the variants that take a pid_t as using pid_t can be more work (an extra hash table lookup) and difficult to get right in the presence of multiple pid namespaces.
There are two kinds of functions introduced in this patch. The are the general use functions kill_pgrp and kill_pid which take a priv argument that is ultimately used to create the appropriate siginfo information, Then there are _kill_pgrp_info, kill_pgrp_info, kill_pid_info the internal implementation helpers that take an explicit siginfo.
The distinction is made because filling out an explcit siginfo is tricky, and will be even more tricky when pid namespaces are introduced.
--- include/linux/sched.h | 5 ++++ kernel/signal.c | 57 ++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 52 insertions(+), 10 deletions(-)
include/linux/sched.h +5 -0
@@ -1290,6 +1290,11 @@ extern int send_sig_info(int, struct sigextern int send_group_sig_info (int ,struct siginfo *,struct task_struct *);extern int force_sigsegv (int ,struct task_struct *);extern int force_sig_info (int ,struct siginfo *,struct task_struct *); +extern int __kill_pgrp_info (int sig,struct siginfo *info,struct pid *pgrp); +extern int kill_pgrp_info (int sig,struct siginfo *info,struct pid *pgrp); +extern int kill_pid_info (int sig,struct siginfo *info,struct pid *pid); +extern int kill_pgrp (struct pid *pid,int sig,int priv); +extern int kill_pid (struct pid *pid,int sig,int priv);extern int __kill_pg_info (int sig,struct siginfo *info,pid_t pgrp);extern int kill_pg_info (int ,struct siginfo *,pid_t );extern int kill_proc_info (int ,struct siginfo *,pid_t );
kernel/signal.c +47 -10
102 lines not on this page · raw message
Signature · 1 line
1.4.2.rc3.g7e18e-dirty
PATCH 4/7 pid: Export the symbols needed to use struct pid *
In reply to Eric W. Biedermanpids aren't something that drivers should care about. However there are a lot of helper layers in the kernel that do care, and are built as modules. Before I can convert them to using struct pid instead of pid_t I need to export the appropriate symbols so they can continue to be built.
--- kernel/pid.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
kernel/pid.c +3 -0
@@ -153,6 +153,7 @@ fastcall void put_pid(struct pid *pid) atomic_dec_and_test(&pid->count))kmem_cache_free (pid_cachep, pid); } +EXPORT_SYMBOL_GPL (put_pid);static void delayed_put_pid (struct rcu_head *rhp) {
@@ -218,6 +219,7 @@ struct pid * fastcall find_pid(int nr) }return NULL ; } +EXPORT_SYMBOL_GPL (find_pid); int fastcall attach_pid(struct task_struct *task, enum pid_type type, int nr) {
@@ -302,6 +304,7 @@ struct pid *find_get_pid(pid_t nr)return pid; } +EXPORT_SYMBOL_GPL (find_get_pid);/* * The pid hash table is scaled according to the amount of memory in the
Signature · 1 line
1.4.2.rc3.g7e18e-dirty
PATCH 5/7 pid: Implement pid_nr
In reply to Eric W. BiedermanAs we stop storing pid_t's and move to storing struct pid *. We need a way to get the pid_t from the struct pid to report to user space what we have stored.
Having a clean well defined way to do this is especially important as we move to multiple pid spaces as may need to report a different value to the caller depending on which pid space the caller is in.
--- include/linux/pid.h | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
include/linux/pid.h +8 -0
@@ -93,6 +93,14 @@ extern struct pid *find_get_pid(int nr);extern struct pid *alloc_pid (void );extern void FASTCALL (free_pid (struct pid *pid)); +static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +} +#define pid_next (task, type) \ ((task)->pids[(type)].node.next)
Signature · 1 line
1.4.2.rc3.g7e18e-dirty
Re: [PATCH 5/7] pid: Implement pid_nr
include/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 7 lines · Eric W. Biederman
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}
When is it valid to be passing around a NULL 'struct pid *'?
-- Dave
Re: [PATCH 5/7] pid: Implement pid_nr
Quoted 10 lines · Dave Hansen
include/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 7 lines · Eric W. Biederman
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}When is it valid to be passing around a NULL 'struct pid *'?
When you don't have one at all. Look at the fcntl case a few patches later, or even the spawnpid case. It simplifies things to just cope with the fact that sometimes the users just have a NULL pointers.
Then of course there is the later chaos when we get to pid spaces where depending on the pid namespace you are in when you call this on a given struct pid sometimes you will get a pid value and sometimes you won't.
Eric
Re: [Containers] [PATCH 5/7] pid: Implement pid_nr
Quoted 13 lines · Eric W. Biederman
Quoted 10 lines · Dave Hansen
include/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 7 lines · Eric W. Biederman
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}When is it valid to be passing around a NULL 'struct pid *'?
Quoted 2 lines
When you don't have one at all. Look at the fcntl case a few patches later, or even the spawnpid case.
Does the fcntl() one originate from anywhere other than find_pid() in f_setown()? It seems like, perhaps, the error checking is being done at the wrong level.
Quoted 4 lines
Then of course there is the later chaos when we get to pid spaces where depending on the pid namespace you are in when you call this on a given struct pid sometimes you will get a pid value and sometimes you won't.
OK, I think it is makes sense to me to say 'get_pid(tsk, pidspace)' and get back a NULL 'struct pid' if that task isn't visible in that namespace. However, I don't get how it is handy to be able to defer the fact that the pid wasn't found until you go do a pid_nr() on that NULL.
-- Dave
Re: [Containers] [PATCH 5/7] pid: Implement pid_nr
Quoted 14 lines · Dave Hansen
Quoted 13 lines · Eric W. Biederman
Quoted 10 lines · Dave Hansen
include/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 7 lines · Eric W. Biederman
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}When is it valid to be passing around a NULL 'struct pid *'?
Quoted 6 lines
Quoted 2 lines
When you don't have one at all. Look at the fcntl case a few patches later, or even the spawnpid case.
Does the
fcntl()one originate from anywhere other thanfind_pid()inf_setown()? It seems like, perhaps, the error checking is being done at the wrong level.
Yes. It is normally NULL as file handles don't usually have a process to send a signal to.
Quoted 9 lines
Quoted 4 lines
Then of course there is the later chaos when we get to pid spaces where depending on the pid namespace you are in when you call this on a given struct pid sometimes you will get a pid value and sometimes you won't.
OK, I think it is makes sense to me to say '
get_pid(tsk, pidspace)' and get back a NULL 'struct pid' if that task isn't visible in that namespace. However, I don't get how it is handy to be able to defer the fact that the pid wasn't found until you go do apid_nr()on that NULL.
If having a pid to signal is optional, which it almost always is, being able to handle that case easily and return a 0 to user space is helpful.
So far it is my assumption that everything that looks up a pid and converts a pid to a number will do it with respect to the current processes pidspace. A very reasonable assumption and only with siginfo have I found a case where it looks like I might not be able to implement it that way.
Eric
Re: [PATCH 5/7] pid: Implement pid_nr
In reply to Dave HansenQuoted 10 lines
include/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 8 lines
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}When is it valid to be passing around a NULL 'struct pid *'?
Is 0 even the right thing to return in the rare case that pid == NULL? -1 maybe?
Jan Engelhardt
Re: [PATCH 5/7] pid: Implement pid_nr
Quoted 10 lines · Jan Engelhardt
Quoted 9 lines
include/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 7 lines
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}When is it valid to be passing around a NULL 'struct pid *'?
Quoted 2 lines
Is 0 even the right thing to return in the rare case that pid == NULL? -1 maybe?
I believe 0 is what we have used elsewhere.
A negative value for a pid is likely to be taken as the group of all processes, or -EPERM, and it does really confusing things when fed through the current f_getown implementation, with PIDTYPE_PGRP.
The only other possible interpretations of 0 are the idle process and yourself. There is still some potential there, but largely 0 is much less likely to be confused than -1. Especially as it doesn't change when you negate it.
Eric
Re: [PATCH 5/7] pid: Implement pid_nr
In reply to Eric W. Biedermaninclude/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 8 lines · Eric W. Biederman
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}
I think this is not safe, you need rcu locks here or the caller should do some locking.
Let's look at f_getown() (PATCH 7/7). What if original task which was pointed by ->f_owner.pid has gone, another thread does fcntl(F_SETOWN), and pid_nr() takes a preemtion after 'if (pid)'? In this case 'pid->nr' may follow a freed memory.
Oleg.
Re: [PATCH 5/7] pid: Implement pid_nr
Quoted 13 lines · Oleg Nesterov
include/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 8 lines · Eric W. Biederman
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}I think this is not safe, you need rcu locks here or the caller should do some locking.
Quoted 4 lines
Let's look at
f_getown()(PATCH 7/7). What if original task which was pointed by ->f_owner.pid has gone, another thread doesfcntl(F_SETOWN), andpid_nr()takes a preemtion after 'if (pid)'? In this case 'pid->nr' may follow a freed memory.
This isn't an rcu reference. I hold a hard reference count on the pid entry. So this should be safe.
What is an rcu reference is going from struct pid to the task it points to.
Eric
Re: [PATCH 5/7] pid: Implement pid_nr
Quoted 15 lines · Eric W. Biederman
Quoted 13 lines · Oleg Nesterov
include/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 8 lines · Eric W. Biederman
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}I think this is not safe, you need rcu locks here or the caller should do some locking.
Quoted 7 lines
Quoted 4 lines
Let's look at
f_getown()(PATCH 7/7). What if original task which was pointed by ->f_owner.pid has gone, another thread doesfcntl(F_SETOWN), andpid_nr()takes a preemtion after 'if (pid)'? In this case 'pid->nr' may follow a freed memory.This isn't an rcu reference. I hold a hard reference count on the pid entry. So this should be safe.
-static void f_modown(struct file *filp, unsigned long pid, +static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
uid_t uid, uid_t euid, int force)
{
write_lock_irq(&filp->f_owner.lock);
if (force || !filp->f_owner.pid) {
- filp->f_owner.pid = pid;
+ put_pid(filp->f_owner.pid);
This 'put_pid()' can actually free 'struct pid' if the task/group has already gone away. Another thread doing f_getown() can access a freed memory, no?
Quoted 2 lines
What is an rcu reference is going from struct pid to the task it points to.
Yes, you are right... But I'd say it is going form task to pid :)
Oleg.
Re: [PATCH 5/7] pid: Implement pid_nr
Quoted 34 lines · Oleg Nesterov
Quoted 15 lines · Eric W. Biederman
Quoted 13 lines · Oleg Nesterov
include/linux/pid.h extern struct pid *find_get_pid(int nr); · line 102
Quoted 8 lines · Eric W. Biederman
include/linux/pid.h +7
-0+static inline pid_t pid_nr (struct pid *pid) +{ +pid_t nr =0 ; +if (pid) + nr = pid->nr; +return nr; +}I think this is not safe, you need rcu locks here or the caller should do some locking.
Quoted 7 lines
Quoted 4 lines
Let's look at
f_getown()(PATCH 7/7). What if original task which was pointed by ->f_owner.pid has gone, another thread doesfcntl(F_SETOWN), andpid_nr()takes a preemtion after 'if (pid)'? In this case 'pid->nr' may follow a freed memory.This isn't an rcu reference. I hold a hard reference count on the pid entry. So this should be safe.
-static void f_modown(struct file *filp, unsigned long pid, +static void f_modown(struct file *filp, struct pid *pid, enum
pid_type
type,uid_t uid, uid_t euid, int force) { write_lock_irq(&filp->f_owner.lock); if (force || !filp->f_owner.pid) { - filp->f_owner.pid = pid; + put_pid(filp->f_owner.pid);
Quoted 3 lines
This '
put_pid()' can actually free 'struct pid' if the task/group has already gone away. Another thread doingf_getown()can access a freed memory, no?
Good point. In that case it looks like I need to hold the f_owner.lock. Something needs to serialize that.
Fun. I touch the code and find a place where we didn't take a lock and accidentally relied on integer operations being atomic.
I will see about working up a fix for that.
Eric
PATCH 6/7 vt: Update spawnpid to be a struct pid_t
In reply to Eric W. BiedermanThis keeps the wrong process from being notified if the daemon to spawn a new console dies.
--- drivers/char/keyboard.c | 9 ++++++--- drivers/char/vt_ioctl.c | 5 +++-- 2 files changed, 9 insertions(+), 5 deletions(-)
drivers/char/keyboard.c +6 -3
@@ -108,7 +108,8 @@ const int NR_TYPES = ARRAY_SIZE(max_valsstruct kbd_struct kbd_table[MAX_NR_CONSOLES ];static struct kbd_struct *kbd = kbd_table; -int spawnpid, spawnsig; +struct pid *spawnpid; +int spawnsig;/* * Variables exported for vt.c
@@ -579,8 +580,10 @@ static void fn_compose(struct vc_data *vstatic void fn_spawn_con (struct vc_data *vc,struct pt_regs *regs) {if (spawnpid) -if (kill_proc (spawnpid, spawnsig,1 )) - spawnpid =0 ; +if (kill_pid (spawnpid, spawnsig,1 )) { +put_pid (spawnpid); + spawnpid =NULL ; + } }static void fn_SAK (struct vc_data *vc,struct pt_regs *regs)
drivers/char/vt_ioctl.c +3 -2
@@ -645,12 +645,13 @@ #endif*/ case KDSIGACCEPT : { -extern int spawnpid, spawnsig; +struct pid *spawnpid; +extern int spawnsig;if (!perm || !capable (CAP_KILL ))return -EPERM ;if (!valid_signal (arg) || arg <1 || arg ==SIGKILL )return -EINVAL ; - spawnpid = current->pid; + spawnpid =get_pid (task_pid (current)); spawnsig = arg;return 0 ; }
Signature · 1 line
1.4.2.rc3.g7e18e-dirty
Re: [PATCH 6/7] vt: Update spawnpid to be a struct pid_t
Ar Maw, 2006-08-15 am 12:23 -0600, ysgrifennodd Eric W. Biederman:
Quoted 2 lines
This keeps the wrong process from being notified if the daemon to spawn a new console dies.
Not sure why we count pids not task structs but within the proposed implementation this appears correct so
Re: [PATCH 6/7] vt: Update spawnpid to be a struct pid_t
Quoted 6 lines · Alan Cox
Ar Maw, 2006-08-15 am 12:23 -0600, ysgrifennodd Eric W. Biederman:
Quoted 2 lines
This keeps the wrong process from being notified if the daemon to spawn a new console dies.
Not sure why we count pids not task structs but within the proposed implementation this appears correct so
Basically struct pid is relatively cheap, 64bytes or so. struct task is expensive 10K or so, when all of the stacks and everything are included.
Counting pids allows the task to exit in user space and free up all of it's memory.
When /proc used to count the task struct it was fairly easy to deliberately oom a 32bit machine just by open up directories in /proc and then having the process exit. rlimits didn't help because we don't count processes that have exited.
Quoted 2 lines
Acked-by Alan Cox <[email protected]>
Thanks. When I get to the tty portion of this I think I am going to have to synchronize with you as last I looked you were working in this area as well.
Eric
Re: [Containers] [PATCH 6/7] vt: Update spawnpid to be a struct pid_t
Quoted 16 lines
Quoted 7 lines · Alan Cox
Ar Maw, 2006-08-15 am 12:23 -0600, ysgrifennodd Eric W. Biederman:
Quoted 2 lines
This keeps the wrong process from being notified if the daemon to spawn a new console dies.
Not sure why we count pids not task structs but within the proposed implementation this appears correct so
Basically struct pid is relatively cheap, 64bytes or so. struct task is expensive 10K or so, when all of the stacks and everything are included.
Quoted 7 lines
Counting pids allows the task to exit in user space and free up all of it's memory.
When /proc used to count the task struct it was fairly easy to deliberately oom a 32bit machine just by open up directories in /proc and then having the process exit. rlimits didn't help because we don't count processes that have exited.
hey, hey. your patch doesn't help in this situation (much)! inodes and dentries can still consume much memory. it just makes the situation a bit better.
I wonder whether it is easy to have the following done with your implementation:
container tasks visible from host. theoretically having 2 pids (vpid, pid) it should be implementable. Do you see any obstacles?
in other regards patches look pretty good. Good job!
Kirill
Re: [Containers] [PATCH 6/7] vt: Update spawnpid to be a struct pid_t
Quoted 18 lines · Kirill Korotaev
Quoted 9 lines
Quoted 7 lines · Alan Cox
Ar Maw, 2006-08-15 am 12:23 -0600, ysgrifennodd Eric W. Biederman:
Quoted 2 lines
This keeps the wrong process from being notified if the daemon to spawn a new console dies.
Not sure why we count pids not task structs but within the proposed implementation this appears correct so
Quoted 9 lines
Basically struct pid is relatively cheap, 64bytes or so. struct task is expensive 10K or so, when all of the stacks and everything are included.
Counting pids allows the task to exit in user space and free up all of it's memory.
When /proc used to count the task struct it was fairly easy to deliberately oom a 32bit machine just by open up directories in /proc and then having the process exit. rlimits didn't help because we don't count processes that have exited.
Quoted 3 lines
hey, hey. your patch doesn't help in this situation (much)! inodes and dentries can still consume much memory. it just makes the situation a bit better.
It does help. The size of the locked memory is better and we account for the used inodes and dentries. As I recall the inode+dentry is less than 1k. The number of open file handle rlimit at least lets the existing limits work. The core improvement is not allowing an escape from our accounting mechanism for one of our biggest data structures.
Quoted 3 lines
I wonder whether it is easy to have the following done with your implementation:
container tasks visible from host. theoretically having 2 pids (vpid, pid) it should be implementable. Do you see any obstacles?
I seen no problem for having tasks have N pid numbers. The trick is to create a hash table entry (struct pid) that when has a pointer to the real struct pid. It won't be the classic pid,vpid. All pids will be equally real.
My intention is to give a process a pid in each namespace that it has an ancestor in.
Quoted 1 line
in other regards patches look pretty good. Good job!
Thanks.
Eric
Re: [PATCH 6/7] vt: Update spawnpid to be a struct pid_t
In reply to Eric W. BiedermanQuoted 41 lines · Eric W. Biederman
This keeps the wrong process from being notified if the daemon to spawn a new console dies.
Signed-off-by Eric W. Biederman <[email protected]>--- drivers/char/keyboard.c | 9 ++++++--- drivers/char/vt_ioctl.c | 5 +++-- 2 files changed, 9 insertions(+), 5 deletions(-)drivers/char/keyboard.c +6
-3@@ -108,7 +108,8 @@ const int NR_TYPES = ARRAY_SIZE(max_valsstruct kbd_struct kbd_table[MAX_NR_CONSOLES ];static struct kbd_struct *kbd = kbd_table; -int spawnpid, spawnsig; +struct pid *spawnpid; +int spawnsig;/* * Variables exported for vt.c @@ -579,8 +580,10 @@ static void fn_compose(struct vc_data *vstatic void fn_spawn_con (struct vc_data *vc,struct pt_regs *regs) {if (spawnpid) -if (kill_proc (spawnpid, spawnsig,1 )) - spawnpid =0 ; +if (kill_pid (spawnpid, spawnsig,1 )) { +put_pid (spawnpid); + spawnpid =NULL ; + } }static void fn_SAK (struct vc_data *vc,struct pt_regs *regs)drivers/char/vt_ioctl.c +0
-0
drivers/char/vt_ioctl.c #endif · line 649
Quoted 6 lines
drivers/char/vt_ioctl.c +2
-1@@ -645,12 +645,13 @@ #endif*/ case KDSIGACCEPT : { -extern int spawnpid, spawnsig; +struct pid *spawnpid; +extern int spawnsig;
shouldn't spawnpid also be declared 'extern' here?
Quoted 9 lines
drivers/char/vt_ioctl.c +1
-1if (!perm || !capable (CAP_KILL ))return -EPERM ;if (!valid_signal (arg) || arg <1 || arg ==SIGKILL )return -EINVAL ; - spawnpid = current->pid; + spawnpid =get_pid (task_pid (current)); spawnsig = arg;return 0 ; }
Re: [PATCH 6/7] vt: Update spawnpid to be a struct pid_t
In reply to Eric W. Biedermandrivers/char/vt_ioctl.c #endif · line 648
Quoted 11 lines · Eric W. Biederman
drivers/char/vt_ioctl.c +1
-1@@ -645,12 +645,13 @@ #endif*/ case KDSIGACCEPT : { -extern int spawnpid, spawnsig; +struct pid *spawnpid;
^^^^^^^^^^^^^^^^^^^^
Should be "extern struct pid *spawnpid" ?
Oleg.
Re: [PATCH 6/7] vt: Update spawnpid to be a struct pid_t
Quoted 13 lines · On Wed, 16 Aug 2006 23:35:57 +0400 Oleg Nesterov <[email protected]> wrote:
drivers/char/vt_ioctl.c #endif · line 648
Quoted 11 lines · Eric W. Biederman
drivers/char/vt_ioctl.c +1
-1@@ -645,12 +645,13 @@ #endif*/ case KDSIGACCEPT : { -extern int spawnpid, spawnsig; +struct pid *spawnpid;^^^^^^^^^^^^^^^^^^^^
Quoted 2 lines
Should be "extern struct pid *spawnpid" ?
It was updated thusly: (the identifiers are a bit generic-sounding though)
From: Eric Biederman <[email protected]>
This moves the declaration of spawnpid and spawnsig into, a header file, and makes spawnpid extern. Without being extern the declaration of swanpid was a nice little struct pid leak in the kernel and totally broke the spawnpid functionality :(
---
drivers/char/vt_ioctl.c | 2 -- include/linux/vt_kern.h | 3 +++ 2 files changed, 3 insertions(+), 2 deletions(-)
drivers/char/vt_ioctl.c renamed from drivers/char/vt_ioctl.c~vt-update-spawnpid-to-be-a-struct-pid_t-tidy +0 -2
@@ -645,8 +645,6 @@ int vt_ioctl(struct tty_struct *tty, str*/ case KDSIGACCEPT : { -struct pid *spawnpid; -extern int spawnsig;if (!perm || !capable (CAP_KILL ))return -EPERM ;if (!valid_signal (arg) || arg <1 || arg ==SIGKILL )
include/linux/vt_kern.h renamed from include/linux/vt_kern.h~vt-update-spawnpid-to-be-a-struct-pid_t-tidy +3 -0
@@ -84,4 +84,7 @@ void reset_vc(struct vc_data *vc);extern char con_buf[CON_BUF_SIZE ];extern struct semaphore con_buf_sem; +extern struct pid *spawnpid; +extern int spawnsig; +#endif /* _VT_KERN_H */
_
Re: [PATCH 6/7] vt: Update spawnpid to be a struct pid_t
Quoted 19 lines · Andrew Morton
Quoted 13 lines · On Wed, 16 Aug 2006 23:35:57 +0400 Oleg Nesterov <[email protected]> wrote:
drivers/char/vt_ioctl.c #endif · line 648
Quoted 11 lines · Eric W. Biederman
drivers/char/vt_ioctl.c +1
-1@@ -645,12 +645,13 @@ #endif*/ case KDSIGACCEPT : { -extern int spawnpid, spawnsig; +struct pid *spawnpid;^^^^^^^^^^^^^^^^^^^^Quoted 2 lines
Should be "extern struct pid *spawnpid" ?
Quoted 1 line
It was updated thusly: (the identifiers are a bit generic-sounding though)
I need to relook at this. But I believe Oleg found an idiom bug in f_getown that will also apply here. Basically if the update is not an atomic transaction then we need to hold a lock when updating the struct pid pointer so updates and uses of the pointer don't race.
Assuming I need to address that. I will place spawnpid, spawnsig, and their lock into a structure and see if I can give it a little bit better name.
My only defense. I didn't change the name in the first place. :)
Eric
PATCH 7/7 file: Modify struct fown_struct to use a struct pid.
In reply to Eric W. BiedermanFile handles can be requested to send sigio and sigurg
to processes. By tracking the destination processes
using struct pid instead of pid_t we make the interface safe from all potential pid wrap around problems.
--- drivers/net/tun.c | 2 + fs/dnotify.c | 2 + fs/fcntl.c | 78 +++++++++++++++++++++++++++++++++------------------- fs/file_table.c | 1 + fs/locks.c | 2 + include/linux/fs.h | 5 +++ kernel/futex.c | 2 + net/socket.c | 2 + 8 files changed, 60 insertions(+), 34 deletions(-)
drivers/net/tun.c +1 -1
@@ -697,7 +697,7 @@ static int tun_chr_fasync(int fd, structreturn ret;if (on) { - ret =f_setown (file, current->pid,0 ); + ret =__f_setown (file,task_pid (current),PIDTYPE_PID ,0 );if (ret)return ret; tun->flags |=TUN_FASYNC ;
fs/dnotify.c +1 -1
@@ -92,7 +92,7 @@ int fcntl_dirnotify(int fd, struct file prev = &odn->dn_next; } - error =f_setown (filp, current->pid,0 ); + error =__f_setown (filp,task_pid (current),PIDTYPE_PID ,0 );if (error) goto out_free;
fs/fcntl.c +50 -28
140 lines not on this page · raw message
fs/file_table.c +1 -0
@@ -181,6 +181,7 @@ #endiffops_put (file->f_op);if (file->f_mode &FMODE_WRITE )put_write_access (inode); +put_pid (file->f_owner.pid);file_kill (file); file->f_dentry =NULL ; file->f_vfsmnt =NULL ;
fs/locks.c +1 -1
@@ -1514,7 +1514,7 @@ int fcntl_setlease(unsigned int fd, stru goto out_unlock; } - error =f_setown (filp, current->pid,0 ); + error =__f_setown (filp,task_pid (current),PIDTYPE_PID ,0 ); out_unlock:unlock_kernel ();return error;
include/linux/fs.h +4 -1
@@ -667,7 +667,8 @@ extern struct block_device *I_BDEV(strucstruct fown_struct {rwlock_t lock;/* protects pid, uid, euid fields */ -int pid;/* pid or -pgrp where SIGIO should be sent */ +struct pid *pid;/* pid or -pgrp where SIGIO should be sent */ +enum pid_type pid_type;/* Kind of process group SIGIO should be sent to */ uid_t uid, euid;/* uid/euid of process setting the owner */ int signum;/* posix.1b rt signal to be delivered on IO */ };
@@ -919,8 +920,10 @@ extern void kill_fasync(struct fasync_st/* only for net: no internal synchronization */ extern void __kill_fasync (struct fasync_struct *,int ,int ); +extern int __f_setown (struct file *filp,struct pid *,enum pid_type ,int force);extern int f_setown (struct file *filp,unsigned long arg,int force);extern void f_delown (struct file *filp); +extern pid_t f_getown (struct file *filp);extern int send_sigurg (struct fown_struct *fown);/*
kernel/futex.c +1 -1
@@ -1589,7 +1589,7 @@ static int futex_fd(u32 __user *uaddr, i filp->f_mapping = filp->f_dentry->d_inode->i_mapping;if (signal) { - err =f_setown (filp, current->pid,1 ); + err =__f_setown (filp,task_pid (current),PIDTYPE_PID ,1 );if (err <0 ) { goto error; }
net/socket.c +1 -1
@@ -825,7 +825,7 @@ #endif /* CONFIG_WIRELESS_EXT */break ;case FIOGETOWN :case SIOCGPGRP : - err =put_user (sock->file->f_owner.pid, + err =put_user (f_getown (sock->file), (int __user *)argp);break ;case SIOCGIFBR :
Signature · 1 line
1.4.2.rc3.g7e18e-dirty
Re: [PATCH 7/7] file: Modify struct fown_struct to use a struct pid.
Quoted 5 lines · Eric W. Biederman
File handles can be requested to send sigio and sigurg
to processes. By tracking the destination processes
using struct pid instead ofpid_twe make the interface safe from all potential pid wrap around problems.
As I can see, this patch adds 2 user visible changes. I am not arguing, but probably it makes sense to document this.
Before this patch, fcntl(F_GETOWN) returns the same pid that was given to fcntl(F_SETOWN). Now it returns 0 if there were no process/group with such a pid when F_SETOWN was called.
The second change is good (I'd say this is bugfix). It is not possible anymore to send the signal to not yet created processes via fcntl(F_SETOWN, last_pid + a_little), or hit a problem with pid re-use.
Oleg.
Re: [PATCH 7/7] file: Modify struct fown_struct to use a struct pid.
Quoted 8 lines · Oleg Nesterov
Quoted 5 lines · Eric W. Biederman
File handles can be requested to send sigio and sigurg
to processes. By tracking the destination processes
using struct pid instead ofpid_twe make the interface safe from all potential pid wrap around problems.
Quoted 6 lines
....
The second change is good (I'd say this is bugfix). It is not possible anymore to send the signal to not yet created processes via
fcntl(F_SETOWN, last_pid + a_little), or hit a problem with pid re-use.
Damn, I read the patch but forgot to read the changelog :) It clearly says "safe from all potential pid wrap around problems", sorry.
Oleg.