Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
On Wed, Oct 16, 2024 at 03:00:55PM +0200, Christian Brauner wrote:
> On Fri, Oct 11, 2024 at 12:05:55PM +0100, Lorenzo Stoakes wrote:
> > The means by which a pid is determined from a pidfd is duplicated, with
> > some callers holding a reference to the (pid)fd, and others explicitly
> > pinning the pid.
> >
> > Introduce __pidfd_get_pid() which abstracts both approaches and provide
> > optional output parameters for file->f_flags and the fd (the latter of
> > which, if provided, prevents the function from decrementing the fd's
> > refernce count).
> >
> > Additionally, allow the ability to open a pidfd by opening a /proc/
> > directory, utilised by the pidfd_send_signal() system call, providing a
> > pidfd_get_pid_proc() helper function to do so.
> >
> > Doing this allows us to eliminate open-coded pidfd pid lookup and to
> > consistently handle this in one place.
> >
> > This lays the groundwork for a subsequent patch which adds a new sentinel
> > pidfd to explicitly reference the current process (i.e. thread group
> > leader) without the need for a pidfd.
> >
> > Signed-off-by: Lorenzo Stoakes
> > ---
> > include/linux/pid.h | 42 +++-
> > kernel/pid.c| 58 ++---
> > kernel/signal.c | 22 -
> > 3 files changed, 84 insertions(+), 38 deletions(-)
> >
> > diff --git a/include/linux/pid.h b/include/linux/pid.h
> > index a3aad9b4074c..68b02eab7509 100644
> > --- a/include/linux/pid.h
> > +++ b/include/linux/pid.h
> > @@ -2,6 +2,7 @@
> > #ifndef _LINUX_PID_H
> > #define _LINUX_PID_H
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
> >
> > struct file;
> >
> > +
> > +/**
> > + * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
> > + *
> > + * @pidfd: The pidfd whose pid we want, or the fd of a /proc/
> > file if
> > + * @alloc_proc is also set.
> > + * @pin_pid:If set, then the reference counter of the returned pid is
> > + * incremented. If not set, then @fd should be provided to
> > pin the
> > + * pidfd.
> > + * @allow_proc: If set, then an fd of a /proc/ file can be passed
> > instead
> > + * of a pidfd, and this will be used to determine the pid.
> > + * @flags: Output variable, if non-NULL, then the file->f_flags of the
> > + * pidfd will be set here.
> > + * @fd: Output variable, if non-NULL, then the pidfd reference will
> > + * remain elevated and the caller will need to decrement it
> > + * themselves.
> > + *
> > + * Returns: If successful, the pid associated with the pidfd, otherwise an
> > + * error.
> > + */
> > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > + bool allow_proc, unsigned int *flags,
> > + struct fd *fd);
> > +
> > +static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int
> > *flags)
> > +{
> > + return __pidfd_get_pid(pidfd, /* pin_pid = */ true,
> > + /* allow_proc = */ false,
> > + flags, /* fd = */ NULL);
> > +}
> > +
> > +static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd,
> > + unsigned int *flags,
> > + struct fd *fd)
> > +{
> > + return __pidfd_get_pid(pidfd, /* pin_pid = */ false,
> > + /* allow_proc = */ true,
> > + flags, fd);
> > +}
> > +
> > struct pid *pidfd_pid(const struct file *file);
> > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
> > struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
> > int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
> > void do_notify_pidfd(struct task_struct *task);
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 2715afb77eab..25cc1c36a1b1 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -36,6 +36,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -534,22 +535,46 @@ struct pid *find_ge_pid(int nr, struct pid_namespace
> > *ns)
> > }
> > EXPORT_SYMBOL_GPL(find_ge_pid);
> >
> > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > + bool allow_proc, unsigned int *flags,
> > + struct fd *fd)
>
> Hm, we should never return a struct fd. A struct fd is an inherently
> scoped-bound concept - or at least aims to be. Simply put, we always
> want to have the fdget() and the fdput() in the same scope as the file
> pointer you can access via fd_file() is only valid as long as we're in
> the syscall.
>
> Ideally we mostly use CLASS(fd/fd_raw) and nearly never fdget(). The
> point is that this is
Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
On Fri, Oct 11, 2024 at 12:05:55PM +0100, Lorenzo Stoakes wrote:
> The means by which a pid is determined from a pidfd is duplicated, with
> some callers holding a reference to the (pid)fd, and others explicitly
> pinning the pid.
>
> Introduce __pidfd_get_pid() which abstracts both approaches and provide
> optional output parameters for file->f_flags and the fd (the latter of
> which, if provided, prevents the function from decrementing the fd's
> refernce count).
>
> Additionally, allow the ability to open a pidfd by opening a /proc/
> directory, utilised by the pidfd_send_signal() system call, providing a
> pidfd_get_pid_proc() helper function to do so.
>
> Doing this allows us to eliminate open-coded pidfd pid lookup and to
> consistently handle this in one place.
>
> This lays the groundwork for a subsequent patch which adds a new sentinel
> pidfd to explicitly reference the current process (i.e. thread group
> leader) without the need for a pidfd.
>
> Signed-off-by: Lorenzo Stoakes
> ---
> include/linux/pid.h | 42 +++-
> kernel/pid.c| 58 ++---
> kernel/signal.c | 22 -
> 3 files changed, 84 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index a3aad9b4074c..68b02eab7509 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -2,6 +2,7 @@
> #ifndef _LINUX_PID_H
> #define _LINUX_PID_H
>
> +#include
> #include
> #include
> #include
> @@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
>
> struct file;
>
> +
> +/**
> + * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
> + *
> + * @pidfd: The pidfd whose pid we want, or the fd of a /proc/ file
> if
> + * @alloc_proc is also set.
> + * @pin_pid:If set, then the reference counter of the returned pid is
> + * incremented. If not set, then @fd should be provided to pin
> the
> + * pidfd.
> + * @allow_proc: If set, then an fd of a /proc/ file can be passed
> instead
> + * of a pidfd, and this will be used to determine the pid.
> + * @flags: Output variable, if non-NULL, then the file->f_flags of the
> + * pidfd will be set here.
> + * @fd: Output variable, if non-NULL, then the pidfd reference will
> + * remain elevated and the caller will need to decrement it
> + * themselves.
> + *
> + * Returns: If successful, the pid associated with the pidfd, otherwise an
> + * error.
> + */
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd);
> +
> +static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int
> *flags)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ true,
> +/* allow_proc = */ false,
> +flags, /* fd = */ NULL);
> +}
> +
> +static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd,
> + unsigned int *flags,
> + struct fd *fd)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ false,
> +/* allow_proc = */ true,
> +flags, fd);
> +}
> +
> struct pid *pidfd_pid(const struct file *file);
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
> struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
> int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
> void do_notify_pidfd(struct task_struct *task);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2715afb77eab..25cc1c36a1b1 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -36,6 +36,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -534,22 +535,46 @@ struct pid *find_ge_pid(int nr, struct pid_namespace
> *ns)
> }
> EXPORT_SYMBOL_GPL(find_ge_pid);
>
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd)
Hm, we should never return a struct fd. A struct fd is an inherently
scoped-bound concept - or at least aims to be. Simply put, we always
want to have the fdget() and the fdput() in the same scope as the file
pointer you can access via fd_file() is only valid as long as we're in
the syscall.
Ideally we mostly use CLASS(fd/fd_raw) and nearly never fdget(). The
point is that this is the wrong api to expose.
It would probably be wiser if you added a pidfd based fdget() inspired
primitive.
Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
On Wed, Oct 16, 2024 at 04:50:56PM +0800, kernel test robot wrote:
>
>
> Hello,
>
> kernel test robot noticed "BUG:unable_to_handle_page_fault_for_address" on:
Thanks, see below for analysis.
>
> commit: e65dbb5c9051a4da2305787fd558e1d60de2275a ("[PATCH v2 1/3] pidfd:
> extend pidfd_get_pid() and de-duplicate pid lookup")
> url:
> https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/pidfd-extend-pidfd_get_pid-and-de-duplicate-pid-lookup/20241011-191241
> base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git
> next
> patch link:
> https://lore.kernel.org/all/8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoa...@oracle.com/
> patch subject: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate
> pid lookup
>
> in testcase: trinity
> version: trinity-i386-abe9de86-1_20230429
> with following parameters:
>
> runtime: 600s
>
>
>
> config: x86_64-randconfig-072-20241015
> compiler: gcc-12
> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version
> of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot
> | Closes: https://lore.kernel.org/oe-lkp/[email protected]
>
>
> [ 416.054386][ T1959] BUG: unable to handle page fault for address:
> 8fed9474
> [ 416.055651][ T1959] #PF: supervisor write access in kernel mode
> [ 416.056550][ T1959] #PF: error_code(0x0003) - permissions violation
> [ 416.057502][ T1959] PGD 3e90f5067 P4D 3e90f5067 PUD 3e90f6063 PMD 3e50001a1
> [ 416.058587][ T1959] Oops: Oops: 0003 [#1] PREEMPT SMP KASAN
> [ 416.059414][ T1959] CPU: 1 UID: 65534 PID: 1959 Comm: trinity-c3 Not
> tainted 6.12.0-rc1-4-ge65dbb5c9051 #1
> d7a38916ac9252f968706afc2c77f70fbdabe689
> [ 416.061328][ T1959] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> [ 416.062850][ T1959] RIP: 0010:fput (arch/x86/include/asm/atomic64_64.h:61
> include/linux/atomic/atomic-arch-fallback.h:4404
> include/linux/atomic/atomic-long.h:1571
> include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
> [ 416.063578][ T1959] Code: ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00
> f3 0f 1e fa 55 48 89 e5 41 55 41 54 53 48 89 fb be 08 00 00 00 e8 96 c6 f7 ff
> 48 ff 0b 0f 85 dd 00 00 00 65 4c 8b 25 04 ff 0e 70 4c 8d 6b 48
> All code
>
>0: ff (bad)
>1: ff 66 66jmp*0x66(%rsi)
>4: 2e 0f 1f 84 00 00 00cs nopl 0x0(%rax,%rax,1)
>b: 00 00
>d: 0f 1f 00nopl (%rax)
> 10: f3 0f 1e fa endbr64
> 14: 55 push %rbp
> 15: 48 89 e5mov%rsp,%rbp
> 18: 41 55 push %r13
> 1a: 41 54 push %r12
> 1c: 53 push %rbx
> 1d: 48 89 fbmov%rdi,%rbx
> 20: be 08 00 00 00 mov$0x8,%esi
> 25: e8 96 c6 f7 ff call 0xfff7c6c0
> 2a:*f0 48 ff 0b lock decq (%rbx)<--
> trapping instruction
OK so this looks like the fput() invoking atomic_long_dec_and_test() on an
invalid &file->f_count.
It looks like 0x8fed9474 in RBX is the file...
And that's because I'm not setting f in
SYSCALL_DEFINE4(pidfd_send_signal, ...) at:
pidfd_to_pid_proc(pidfd, &f_flags, &f);
On error and yet then jump to
err:
fdput(f);
return ret;
Which is trying to fdput() (thus fput()) the f, ugh.
OK I will fix this + respin, thanks for the report!
[snip]
Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
On Wed, Oct 16, 2024 at 1:22 AM Lorenzo Stoakes
wrote:
>
> On Wed, Oct 16, 2024 at 01:16:15AM -0700, Suren Baghdasaryan wrote:
> > On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
> > wrote:
> > >
> > > On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
> > > [snip]
> > > > > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > > > > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > > > > + bool allow_proc, unsigned int *flags,
> > > > > + struct fd *fd)
> > > > > {
> > > > > - struct fd f;
> > > > > + struct file *file;
> > > > > struct pid *pid;
> > > > > + struct fd f = fdget(pidfd);
> > > > >
> > > > > - f = fdget(fd);
> > > > > - if (!fd_file(f))
> > > > > + file = fd_file(f);
> > > > > + if (!file)
> > > > > return ERR_PTR(-EBADF);
> > > > >
> > > > > - pid = pidfd_pid(fd_file(f));
> > > > > - if (!IS_ERR(pid)) {
> > > > > - get_pid(pid);
> > > > > - *flags = fd_file(f)->f_flags;
> > > > > + pid = pidfd_pid(file);
> > > > > + /* If we allow opening a pidfd via /proc/, do so. */
> > > > > + if (IS_ERR(pid) && allow_proc)
> > > > > + pid = tgid_pidfd_to_pid(file);
> > > > > +
> > > > > + if (IS_ERR(pid)) {
> > > > > + fdput(f);
> > > > > + return pid;
> > > > > }
> > > > >
> > > > > - fdput(f);
> > > > > + if (pin_pid)
> > > > > + get_pid(pid);
> > > > > + else
> > > > > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd
> > > > > around? */
> > > > > +
> > > > > + if (flags)
> > > > > + *flags = file->f_flags;
> > > > > +
> > > > > + /*
> > > > > +* If the user provides an fd output then it will handle
> > > > > decrementing
> > > > > +* its reference counter.
> > > > > +*/
> > > > > + if (fd)
> > > > > + *fd = f;
> > > > > + else
> > > > > + /* Otherwise we release it. */
> > > > > + fdput(f);
> > > > > +
> > > > > return pid;
> > > > > }
> > > >
> > > > There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> > > > should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> > > > otherwise __pidfd_get_pid() will not be exported. A module calling
> > > > pidfd_get_pid() now inlined in the header file will try to call
> > > > __pidfd_get_pid() and will have trouble resolving this symbol.
> > >
> > > Hmm hang on not there isn't? I don't see that anywhere?
> >
> > Doh! Sorry, I didn't realize the export was an out-of-tree Android
> > change. Never mind...
>
> No probs :P just glad I didn't miss something in this series!
>
> Hey maybe a motivation to upstream some of this? ;)
I wish... Without an upstream user the exports are not accepted
upstream and unfortunately Android vendors often resist upstreaming
their modules.
>
> >
> > >
> > > [snip]
Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
Hello,
kernel test robot noticed "BUG:unable_to_handle_page_fault_for_address" on:
commit: e65dbb5c9051a4da2305787fd558e1d60de2275a ("[PATCH v2 1/3] pidfd: extend
pidfd_get_pid() and de-duplicate pid lookup")
url:
https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/pidfd-extend-pidfd_get_pid-and-de-duplicate-pid-lookup/20241011-191241
base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git
next
patch link:
https://lore.kernel.org/all/8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoa...@oracle.com/
patch subject: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate
pid lookup
in testcase: trinity
version: trinity-i386-abe9de86-1_20230429
with following parameters:
runtime: 600s
config: x86_64-randconfig-072-20241015
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes: https://lore.kernel.org/oe-lkp/[email protected]
[ 416.054386][ T1959] BUG: unable to handle page fault for address:
8fed9474
[ 416.055651][ T1959] #PF: supervisor write access in kernel mode
[ 416.056550][ T1959] #PF: error_code(0x0003) - permissions violation
[ 416.057502][ T1959] PGD 3e90f5067 P4D 3e90f5067 PUD 3e90f6063 PMD 3e50001a1
[ 416.058587][ T1959] Oops: Oops: 0003 [#1] PREEMPT SMP KASAN
[ 416.059414][ T1959] CPU: 1 UID: 65534 PID: 1959 Comm: trinity-c3 Not tainted
6.12.0-rc1-4-ge65dbb5c9051 #1 d7a38916ac9252f968706afc2c77f70fbdabe689
[ 416.061328][ T1959] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 416.062850][ T1959] RIP: 0010:fput (arch/x86/include/asm/atomic64_64.h:61
include/linux/atomic/atomic-arch-fallback.h:4404
include/linux/atomic/atomic-long.h:1571
include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
[ 416.063578][ T1959] Code: ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 f3
0f 1e fa 55 48 89 e5 41 55 41 54 53 48 89 fb be 08 00 00 00 e8 96 c6 f7 ff
48 ff 0b 0f 85 dd 00 00 00 65 4c 8b 25 04 ff 0e 70 4c 8d 6b 48
All code
0: ff (bad)
1: ff 66 66jmp*0x66(%rsi)
4: 2e 0f 1f 84 00 00 00cs nopl 0x0(%rax,%rax,1)
b: 00 00
d: 0f 1f 00nopl (%rax)
10: f3 0f 1e fa endbr64
14: 55 push %rbp
15: 48 89 e5mov%rsp,%rbp
18: 41 55 push %r13
1a: 41 54 push %r12
1c: 53 push %rbx
1d: 48 89 fbmov%rdi,%rbx
20: be 08 00 00 00 mov$0x8,%esi
25: e8 96 c6 f7 ff call 0xfff7c6c0
2a:* f0 48 ff 0b lock decq (%rbx)<-- trapping
instruction
2e: 0f 85 dd 00 00 00 jne0x111
34: 65 4c 8b 25 04 ff 0emov%gs:0x700eff04(%rip),%r12#
0x700eff40
3b: 70
3c: 4c 8d 6b 48 lea0x48(%rbx),%r13
Code starting with the faulting instruction
===
0: f0 48 ff 0b lock decq (%rbx)
4: 0f 85 dd 00 00 00 jne0xe7
a: 65 4c 8b 25 04 ff 0emov%gs:0x700eff04(%rip),%r12#
0x700eff16
11: 70
12: 4c 8d 6b 48 lea0x48(%rbx),%r13
[ 416.066250][ T1959] RSP: 0018:c9000299fa70 EFLAGS: 00010246
[ 416.067156][ T1959] RAX: 0001 RBX: 8fed9474 RCX:
[ 416.068377][ T1959] RDX: RSI: RDI:
[ 416.069091][ T1980] module: module-autoload: duplicate request for module
net-pf-12
[ 416.069532][ T1959] RBP: c9000299fa88 R08: R09:
[ 416.069538][ T1959] R10: R11: R12:
[ 416.069541][ T1959] R13: fff7 R14: c9000299fb70 R15:
dc00
[ 416.078460][ T1959] FS: () GS:8883a850(0063)
knlGS:f7ef8280
[ 416.079775][ T1959] CS: 0010 DS: 002b ES: 002b CR0: 80050033
[ 416.080740][ T1959] CR2: 8fed9474 CR3: 000120fe6000 CR4:
000406f0
[ 416.081938][ T1959] DR0: DR1: DR2:
[ 416.083156][ T1959] DR3: DR6: fffe0ff0 DR7:
0400
[ 416.084359][ T1959] Call Trace:
[ 416.084939][ T1959]
[ 416.085461][ T1959] ? show_regs (arch/x86/kernel/dumpstack.c:479)
[ 416.088241][ T1964] module: module-autoload: duplicate request for module
net-pf-32
[ 416.089149][ T1959] ? __die (arch/x86/kernel/dumpstack.c:421
arch/x86/kernel/dumpstack.c:434)
[ 416.089165][ T1959] ? __kasan_check_read (mm/kasan/sha
Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
On Wed, Oct 16, 2024 at 01:16:15AM -0700, Suren Baghdasaryan wrote:
> On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
> wrote:
> >
> > On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
> > [snip]
> > > > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > > > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > > > + bool allow_proc, unsigned int *flags,
> > > > + struct fd *fd)
> > > > {
> > > > - struct fd f;
> > > > + struct file *file;
> > > > struct pid *pid;
> > > > + struct fd f = fdget(pidfd);
> > > >
> > > > - f = fdget(fd);
> > > > - if (!fd_file(f))
> > > > + file = fd_file(f);
> > > > + if (!file)
> > > > return ERR_PTR(-EBADF);
> > > >
> > > > - pid = pidfd_pid(fd_file(f));
> > > > - if (!IS_ERR(pid)) {
> > > > - get_pid(pid);
> > > > - *flags = fd_file(f)->f_flags;
> > > > + pid = pidfd_pid(file);
> > > > + /* If we allow opening a pidfd via /proc/, do so. */
> > > > + if (IS_ERR(pid) && allow_proc)
> > > > + pid = tgid_pidfd_to_pid(file);
> > > > +
> > > > + if (IS_ERR(pid)) {
> > > > + fdput(f);
> > > > + return pid;
> > > > }
> > > >
> > > > - fdput(f);
> > > > + if (pin_pid)
> > > > + get_pid(pid);
> > > > + else
> > > > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around?
> > > > */
> > > > +
> > > > + if (flags)
> > > > + *flags = file->f_flags;
> > > > +
> > > > + /*
> > > > +* If the user provides an fd output then it will handle
> > > > decrementing
> > > > +* its reference counter.
> > > > +*/
> > > > + if (fd)
> > > > + *fd = f;
> > > > + else
> > > > + /* Otherwise we release it. */
> > > > + fdput(f);
> > > > +
> > > > return pid;
> > > > }
> > >
> > > There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> > > should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> > > otherwise __pidfd_get_pid() will not be exported. A module calling
> > > pidfd_get_pid() now inlined in the header file will try to call
> > > __pidfd_get_pid() and will have trouble resolving this symbol.
> >
> > Hmm hang on not there isn't? I don't see that anywhere?
>
> Doh! Sorry, I didn't realize the export was an out-of-tree Android
> change. Never mind...
No probs :P just glad I didn't miss something in this series!
Hey maybe a motivation to upstream some of this? ;)
>
> >
> > [snip]
Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
wrote:
>
> On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
> [snip]
> > > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > > + bool allow_proc, unsigned int *flags,
> > > + struct fd *fd)
> > > {
> > > - struct fd f;
> > > + struct file *file;
> > > struct pid *pid;
> > > + struct fd f = fdget(pidfd);
> > >
> > > - f = fdget(fd);
> > > - if (!fd_file(f))
> > > + file = fd_file(f);
> > > + if (!file)
> > > return ERR_PTR(-EBADF);
> > >
> > > - pid = pidfd_pid(fd_file(f));
> > > - if (!IS_ERR(pid)) {
> > > - get_pid(pid);
> > > - *flags = fd_file(f)->f_flags;
> > > + pid = pidfd_pid(file);
> > > + /* If we allow opening a pidfd via /proc/, do so. */
> > > + if (IS_ERR(pid) && allow_proc)
> > > + pid = tgid_pidfd_to_pid(file);
> > > +
> > > + if (IS_ERR(pid)) {
> > > + fdput(f);
> > > + return pid;
> > > }
> > >
> > > - fdput(f);
> > > + if (pin_pid)
> > > + get_pid(pid);
> > > + else
> > > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > > +
> > > + if (flags)
> > > + *flags = file->f_flags;
> > > +
> > > + /*
> > > +* If the user provides an fd output then it will handle
> > > decrementing
> > > +* its reference counter.
> > > +*/
> > > + if (fd)
> > > + *fd = f;
> > > + else
> > > + /* Otherwise we release it. */
> > > + fdput(f);
> > > +
> > > return pid;
> > > }
> >
> > There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> > should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> > otherwise __pidfd_get_pid() will not be exported. A module calling
> > pidfd_get_pid() now inlined in the header file will try to call
> > __pidfd_get_pid() and will have trouble resolving this symbol.
>
> Hmm hang on not there isn't? I don't see that anywhere?
Doh! Sorry, I didn't realize the export was an out-of-tree Android
change. Never mind...
>
> [snip]
Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
[snip]
> > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > + bool allow_proc, unsigned int *flags,
> > + struct fd *fd)
> > {
> > - struct fd f;
> > + struct file *file;
> > struct pid *pid;
> > + struct fd f = fdget(pidfd);
> >
> > - f = fdget(fd);
> > - if (!fd_file(f))
> > + file = fd_file(f);
> > + if (!file)
> > return ERR_PTR(-EBADF);
> >
> > - pid = pidfd_pid(fd_file(f));
> > - if (!IS_ERR(pid)) {
> > - get_pid(pid);
> > - *flags = fd_file(f)->f_flags;
> > + pid = pidfd_pid(file);
> > + /* If we allow opening a pidfd via /proc/, do so. */
> > + if (IS_ERR(pid) && allow_proc)
> > + pid = tgid_pidfd_to_pid(file);
> > +
> > + if (IS_ERR(pid)) {
> > + fdput(f);
> > + return pid;
> > }
> >
> > - fdput(f);
> > + if (pin_pid)
> > + get_pid(pid);
> > + else
> > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > +
> > + if (flags)
> > + *flags = file->f_flags;
> > +
> > + /*
> > +* If the user provides an fd output then it will handle
> > decrementing
> > +* its reference counter.
> > +*/
> > + if (fd)
> > + *fd = f;
> > + else
> > + /* Otherwise we release it. */
> > + fdput(f);
> > +
> > return pid;
> > }
>
> There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> otherwise __pidfd_get_pid() will not be exported. A module calling
> pidfd_get_pid() now inlined in the header file will try to call
> __pidfd_get_pid() and will have trouble resolving this symbol.
Hmm hang on not there isn't? I don't see that anywhere?
[snip]
Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
On Fri, Oct 11, 2024 at 4:06 AM Lorenzo Stoakes
wrote:
>
> The means by which a pid is determined from a pidfd is duplicated, with
> some callers holding a reference to the (pid)fd, and others explicitly
> pinning the pid.
>
> Introduce __pidfd_get_pid() which abstracts both approaches and provide
> optional output parameters for file->f_flags and the fd (the latter of
> which, if provided, prevents the function from decrementing the fd's
> refernce count).
>
> Additionally, allow the ability to open a pidfd by opening a /proc/
> directory, utilised by the pidfd_send_signal() system call, providing a
> pidfd_get_pid_proc() helper function to do so.
>
> Doing this allows us to eliminate open-coded pidfd pid lookup and to
> consistently handle this in one place.
>
> This lays the groundwork for a subsequent patch which adds a new sentinel
> pidfd to explicitly reference the current process (i.e. thread group
> leader) without the need for a pidfd.
>
> Signed-off-by: Lorenzo Stoakes
> ---
> include/linux/pid.h | 42 +++-
> kernel/pid.c| 58 ++---
> kernel/signal.c | 22 -
> 3 files changed, 84 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index a3aad9b4074c..68b02eab7509 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -2,6 +2,7 @@
> #ifndef _LINUX_PID_H
> #define _LINUX_PID_H
>
> +#include
> #include
> #include
> #include
> @@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
>
> struct file;
>
> +
> +/**
> + * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
> + *
> + * @pidfd: The pidfd whose pid we want, or the fd of a /proc/ file
> if
> + * @alloc_proc is also set.
> + * @pin_pid:If set, then the reference counter of the returned pid is
> + * incremented. If not set, then @fd should be provided to pin
> the
> + * pidfd.
> + * @allow_proc: If set, then an fd of a /proc/ file can be passed
> instead
> + * of a pidfd, and this will be used to determine the pid.
> + * @flags: Output variable, if non-NULL, then the file->f_flags of the
> + * pidfd will be set here.
> + * @fd: Output variable, if non-NULL, then the pidfd reference will
> + * remain elevated and the caller will need to decrement it
> + * themselves.
> + *
> + * Returns: If successful, the pid associated with the pidfd, otherwise an
> + * error.
> + */
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd);
> +
> +static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int
> *flags)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ true,
> + /* allow_proc = */ false,
> + flags, /* fd = */ NULL);
> +}
> +
> +static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd,
> + unsigned int *flags,
> + struct fd *fd)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ false,
> + /* allow_proc = */ true,
> + flags, fd);
> +}
> +
> struct pid *pidfd_pid(const struct file *file);
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
> struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
> int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
> void do_notify_pidfd(struct task_struct *task);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2715afb77eab..25cc1c36a1b1 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -36,6 +36,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -534,22 +535,46 @@ struct pid *find_ge_pid(int nr, struct pid_namespace
> *ns)
> }
> EXPORT_SYMBOL_GPL(find_ge_pid);
>
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd)
> {
> - struct fd f;
> + struct file *file;
> struct pid *pid;
> + struct fd f = fdget(pidfd);
>
> - f = fdget(fd);
> - if (!fd_file(f))
> + file = fd_file(f);
> + if (!file)
> return ERR_PTR(-EBADF);
>
> - pid = pidfd_pid(fd_file(f));
> - if (!IS_ERR(pid)) {
> - get_pid(pid);
> - *flags = fd_file(f)->f_flags;
> + pid = pidfd_pid(file);
> + /* If we allow opening a pidfd via /proc/, do so. */
> + if (IS_ERR(pid) && allow_proc)
> + pid = tgid_pidfd_to_pid(file);
> +
> + if (IS_ERR(pid)) {
> +
