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
[off-list ref] wrote:

On Wed, Oct 16, 2024 at 01:16:15AM -0700, Suren Baghdasaryan wrote:

quoted

On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
[off-list ref] wrote:

quoted

On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
[snip]

quoted

quoted

-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/<pid>, 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.

quoted

quoted

[snip]