Messages in this thread Patch in this message | | | Date | Wed, 2 Sep 2020 15:15:32 +0200 | From | Christian Brauner <> | Subject | Re: [PATCH] kernel/pid: Rename `find_ge_pid` to `find_get_vpid`. |
| |
On Wed, Sep 02, 2020 at 12:23:10PM +0000, Harley Paterson wrote: > Most PID functions in `kernel/pid.c` are named in the format > [do_something]_pid, and functions with a VPID counterpart have > the corresponding name [do_something]_vpid. > > `find_get_pid` and its VPID counterpart, `find_ge_pid` do not share > this convention. > > This patch renames `find_ge_pid` to `find_get_vpid` to conform to the > existing PID function naming idioms. I believe the proposed name > makes the purpose of the function clearer. > > I have built and tested this patch on an x64 virtual machine, running > Ubuntu 20.04 LTS. My tests involved building the modified kernel, > installing it, and booting into a Ubuntu GUI session. > > This patch is applied on top of the linux-5.9-rc3 kernel > > This patch effects four lines, one in each of four files. > > Signed-off-by: H Paterson <harley.paterson@hotmail.co.nz> > --- > fs/proc/base.c | 2 +- > include/linux/pid.h | 2 +- > kernel/bpf/task_iter.c | 2 +- > kernel/pid.c | 2 +- > 4 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 617db4e0f..e2ff20311 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -3390,7 +3390,7 @@ static struct tgid_iter next_tgid(struct pid_namespace *ns, struct tgid_iter ite > rcu_read_lock(); > retry: > iter.task = NULL; > - pid = find_ge_pid(iter.tgid, ns); > + pid = find_get_vpid(iter.tgid, ns); > if (pid) { > iter.tgid = pid_nr_ns(pid, ns); > iter.task = pid_task(pid, PIDTYPE_TGID); > diff --git a/include/linux/pid.h b/include/linux/pid.h > index 176d6cf80..2875b4189 100644 > --- a/include/linux/pid.h > +++ b/include/linux/pid.h > @@ -128,7 +128,7 @@ extern struct pid *find_vpid(int nr); > * Lookup a PID in the hash table, and return with it's count elevated. > */ > extern struct pid *find_get_pid(int nr); > -extern struct pid *find_ge_pid(int nr, struct pid_namespace *); > +extern struct pid *find_get_vpid(int nr, struct pid_namespace *); > > extern struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid, > size_t set_tid_size); > diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c > index 99af4cea1..334cb8831 100644 > --- a/kernel/bpf/task_iter.c > +++ b/kernel/bpf/task_iter.c > @@ -29,7 +29,7 @@ static struct task_struct *task_seq_get_next(struct pid_namespace *ns, > > rcu_read_lock(); > retry: > - pid = find_ge_pid(*tid, ns); > + pid = find_get_vpid(*tid, ns); > if (pid) { > *tid = pid_nr_ns(pid, ns); > task = get_pid_task(pid, PIDTYPE_PID); > diff --git a/kernel/pid.c b/kernel/pid.c > index b2562a7ce..0c77fef72 100644 > --- a/kernel/pid.c > +++ b/kernel/pid.c > @@ -514,7 +514,7 @@ EXPORT_SYMBOL_GPL(task_active_pid_ns); > * > * If there is a pid at nr this function is exactly the same as find_pid_ns. > */ > -struct pid *find_ge_pid(int nr, struct pid_namespace *ns) > +struct pid *find_get_vpid(int nr, struct pid_namespace *ns)
Hey Harley,
Thanks for the patch.
I think we shouldn't rename this function but I see why you got confused. find_ge_pid() gives you the struct pid associated with the next greatest or equal pid number in the provided pid namespace.
A few notes why this patch is problematic:
- The "_vpid" prefix is usually used in contexts where a (virtual) pid nr is looked up in the caller's pid namespace and some object associated with it returned. But this takes a pid namespace argument and so can be used to look up a pid based on a pid nr in any pid namespace not just in the caller's. - The "ge" here is actually a meaningful abbreviation afaict for "greater than or equal" so replacing it with "get" would lose this important piece of information.
- "get" is usually used - and especially here - to indicate that a reference count is increased which isn't happening here so this would be potentially dangerous as it would signal callers that a reference count is incremented when it really isn't.
Thanks! Christian
| |