Re: [PATCH] kernel/pid: Rename `find_ge_pid` to `find_get_vpid`.

[Christian Brauner]

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@xxxxxxxxxxxxx>
> ---
>  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